From f9e7fee47dc220951e2fb3064615ad9f5a30839f Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Fri, 31 Jan 2020 09:48:05 -0600
Subject: [PATCH 1/9] don't mark a source as changed before we've finished
 remapping

Also, check the remap operation for errors, and if an error occurs,
try to remap to nil (which shouldn't be able to fail).
---
 fragment.go | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fragment.go b/fragment.go
index 22efb072e..39e14f1f6 100644
--- a/fragment.go
+++ b/fragment.go
@@ -296,13 +296,22 @@ func (f *fragment) applyStorage(data []byte, file *os.File, newGen generation, m
 	// Tell storage to prefer mapping if and only if we think the data
 	// is mmapped and valid.
 	f.storage.PreferMapping(mapped)
-	f.storage.SetSource(newGen)
 	// RemapRoaringStorage will fix any mapped containers to point either
 	// to the provided data (if PreferMapping was called with true and
 	// data is provided and there's a corresponding container) or to
 	// allocated storage, so when it's done, there's nothing in it that
 	// is mapped to anything *other than* the provided data.
-	return f.storage.RemapRoaringStorage(data)
+	mapped, err := f.storage.RemapRoaringStorage(data)
+	if err != nil {
+		// OOPS! something went wrong, we don't know why, we can't
+		// sanely recover from that.
+		_, _ = f.storage.RemapRoaringStorage(nil)
+		mapped = false
+		f.storage.SetSource(nil)
+	} else {
+		f.storage.SetSource(newGen)
+	}
+	return mapped, err
 }
 
 // openStorage opens the storage bitmap.

From 372389fd30f269ae51f2afaaf3e3c2913d98947e Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Mon, 3 Feb 2020 15:48:16 -0600
Subject: [PATCH 2/9] don't corrupt files when mmap fails

In some cases, after a snapshot, if mmap fails, we could write
a duplicate of the bitmap to the file, creating cryptic "unknown
op type: 60" messages. This doesn't fix those files, but it stops
making them.
---
 fragment.go  | 15 +++++++++-
 mmap_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 mmap_test.go

diff --git a/fragment.go b/fragment.go
index 39e14f1f6..ae689098b 100644
--- a/fragment.go
+++ b/fragment.go
@@ -291,7 +291,20 @@ func (f *fragment) importStorage(data []byte, file *os.File, newGen generation,
 // to use a new storage as backing store.
 func (f *fragment) applyStorage(data []byte, file *os.File, newGen generation, mapped bool) (bool, error) {
 	if len(data) == 0 {
-		return f.emptyStorage(file)
+		if file != nil {
+			fi, err := file.Stat()
+			if err == nil && fi != nil && fi.Size() == 0 {
+				return f.emptyStorage(file)
+			}
+		}
+		// if we can't be sure of that, we assume data is 0 because
+		// we couldn't mmap it, and since all we'd be doing is remapping
+		// our containers to use that storage *to take advantage of
+		// mmap*, we'll just make sure our containers aren't pointing to
+		// old storage and say "nope".
+		f.storage.RemapRoaringStorage(nil)
+		f.storage.SetSource(nil)
+		return false, nil
 	}
 	// Tell storage to prefer mapping if and only if we think the data
 	// is mmapped and valid.
diff --git a/mmap_test.go b/mmap_test.go
new file mode 100644
index 000000000..542c35763
--- /dev/null
+++ b/mmap_test.go
@@ -0,0 +1,83 @@
+// Copyright 2020 Pilosa Corp.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package pilosa
+
+import (
+	"math/rand"
+	"sync/atomic"
+	"testing"
+)
+
+type cv struct {
+	cols []uint64
+	vals []int64
+}
+
+// This test should basically never fail, but it might if you were running
+// out of available mmaps. Which you can fake up by adding '&& false' to the test
+// in newGeneration in generation.go. So this is probably useless but it's
+// a failure mode we've been bitten by once...
+func TestMmapBehavior(t *testing.T) {
+	depth := uint(6)
+	var done int64
+	f := mustOpenBSIFragment("i", "f", viewStandard, 0)
+	defer f.Clean(t)
+
+	ch := make(chan struct{})
+
+	for i := 0; i < f.MaxOpN; i++ {
+		_, _ = f.setBit(0, uint64(i*32))
+	}
+	// force snapshot so we get a mmapped row...
+	_ = f.Snapshot()
+	row := f.row(0)
+	segment := row.Segments()[0]
+	bitmap := segment.data
+
+	// request information from the frozen bitmap we got back
+	go func() {
+		for atomic.LoadInt64(&done) == 0 {
+			for i := 0; i < f.MaxOpN; i++ {
+				_ = bitmap.Contains(uint64(i * 32))
+			}
+		}
+		close(ch)
+	}()
+
+	values := make([]cv, 1024)
+	for i := range values {
+		cols := make([]uint64, 512)
+		vals := make([]int64, 512)
+		for j := range cols {
+			cols[j] = uint64(rand.Int63n(ShardWidth))
+			vals[j] = int64(rand.Int63n(1 << depth))
+		}
+		values[i] = cv{cols, vals}
+	}
+
+	// modify the original bitmap, until it causes a snapshot, which
+	// then invalidates the other map...
+	for j := 0; j < 5; j++ {
+		for i := 0; i < f.MaxOpN/int(depth+1); i++ {
+			cv := values[i%len(values)]
+			err := f.importValue(cv.cols, cv.vals, depth, (i%3 == 1))
+			if err != nil {
+				t.Fatalf("importValue[%d][%d]: %v", j, i, err)
+			}
+		}
+	}
+	atomic.StoreInt64(&done, 1)
+	<-ch
+}

From 63fb2f8539cdf6d542924a704e8830294b41f959 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Wed, 5 Feb 2020 14:16:07 -0600
Subject: [PATCH 3/9] generation testing and paranoia features

We might have a problem with a stale mmap, and to try to narrow it down
a bit, we add some sanity-checking features and panic recovery to the
generation Transaction code.

This is pretty experimental.
---
 fragment.go        | 49 ++++++++++++++++++++++++++++++++++--
 gendebug_test.go   | 39 +++++++++++++++++++++++++++++
 generation.go      | 32 ++++++++++++++++++++++++
 generation_test.go | 62 +++++++++++++++++++++++++++++++++-------------
 logger/logger.go   | 24 ++++++++++++++++++
 mmap_test.go       | 54 ++++++++++++++++++----------------------
 roaring/roaring.go | 27 ++++++++++++++++++++
 7 files changed, 238 insertions(+), 49 deletions(-)
 create mode 100644 gendebug_test.go

diff --git a/fragment.go b/fragment.go
index ae689098b..dfc0e759c 100644
--- a/fragment.go
+++ b/fragment.go
@@ -28,6 +28,7 @@ import (
 	"math"
 	"math/bits"
 	"os"
+	"runtime/debug"
 	"sort"
 	"strings"
 	"sync"
@@ -105,6 +106,8 @@ type fragment struct {
 	field string
 	view  string
 	shard uint64
+	// debugging tool: addresses of current and previous maps
+	prevdata, currdata struct{ from, to uintptr }
 
 	// File-backed storage
 	path            string
@@ -352,6 +355,16 @@ func (f *fragment) openStorage(unmarshalData bool) error {
 	}
 	var err error
 	f.gen, err = newGeneration(f.gen, f.path, unmarshalData, storageOp, f.Logger)
+	if f.gen != nil {
+		scratchData := f.gen.Bytes()
+		f.prevdata = f.currdata
+		var scratchAddrs struct{ from, to uintptr }
+		if scratchData != nil {
+			scratchAddrs.from = uintptr(unsafe.Pointer(&scratchData[0]))
+			scratchAddrs.to = scratchAddrs.from + uintptr(len(scratchData))
+		}
+		f.currdata = scratchAddrs
+	}
 	if generationDebug {
 		// We might have already done this anyway, if we think we
 		// mapped stuff, but when debugging we want to do it
@@ -1934,6 +1947,19 @@ func (f *fragment) importPositions(set, clear []uint64, rowSet map[uint64]struct
 		}
 		return nil
 	})
+	if err != nil {
+		// we got an error. it's possible that the error indicates that something went wrong.
+		mappedIn, mappedOut, unmappedIn, errs, e2 := f.storage.SanityCheckMapping(f.currdata.from, f.currdata.to)
+		if errs != 0 {
+			f.Logger.Printf("transaction failed on %s. storage has %d mapped in range, %d mapped out of range, %d unmapped in range, %d errors total, last %v",
+				f.path, mappedIn, mappedOut, unmappedIn, errs, e2)
+			if f.prevdata.from != f.currdata.from {
+				mappedIn, mappedOut, unmappedIn, errs, e2 = f.storage.SanityCheckMapping(f.prevdata.from, f.prevdata.to)
+				f.Logger.Printf("with previous map, storage would have %d mapped in range, %d mapped out of range, %d unmapped in range, %d errors total, last %v",
+					mappedIn, mappedOut, unmappedIn, errs, e2)
+			}
+		}
+	}
 	return err
 }
 
@@ -2174,8 +2200,27 @@ func track(start time.Time, message string, stats stats.StatsClient, logger logg
 
 // snapshot does the actual snapshot operation. it does not check or care
 // about f.snapshotPending.
-func (f *fragment) snapshot() error {
-	_, err := unprotectedWriteToFragment(f, f.storage)
+func (f *fragment) snapshot() (err error) {
+	wouldPanic := debug.SetPanicOnFault(true)
+	defer func() {
+		debug.SetPanicOnFault(wouldPanic)
+		if r := recover(); r != nil {
+			fmt.Printf("snapshot panic!\n")
+			if e2, ok := r.(error); ok {
+				err = e2
+				// special case: if we caught a page fault, we diagnose that directly. sadly,
+				// we can't see the actual values that were used to generate this, probably.
+				if e2.Error() == "runtime error: invalid memory address or nil pointer dereference" {
+					mappedIn, mappedOut, unmappedIn, errs, _ := f.storage.SanityCheckMapping(f.currdata.from, f.currdata.to)
+					f.Logger.Printf("transaction failed on %s. storage has %d mapped in range, %d mapped out of range, %d unmapped in range, %d errors total",
+						f.path, mappedIn, mappedOut, unmappedIn, errs)
+				}
+			} else {
+				err = fmt.Errorf("non-error panic: %v", r)
+			}
+		}
+	}()
+	_, err = unprotectedWriteToFragment(f, f.storage)
 	if err == nil {
 		f.snapshotStamp = time.Now()
 	}
diff --git a/gendebug_test.go b/gendebug_test.go
new file mode 100644
index 000000000..3dd86fd8f
--- /dev/null
+++ b/gendebug_test.go
@@ -0,0 +1,39 @@
+// Copyright 2019 Pilosa Corp.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// +build generationdebug
+
+package pilosa
+
+import (
+	"fmt"
+	"os"
+	"testing"
+)
+
+func examineResults() {
+	results := reportGenerations()
+	if len(results) > 0 {
+		fmt.Printf("generations:\n")
+		for _, res := range results {
+			fmt.Printf("  %s\n", res)
+		}
+	}
+}
+
+func TestMain(m *testing.M) {
+	ret := m.Run()
+	examineResults()
+	os.Exit(ret)
+}
diff --git a/generation.go b/generation.go
index 34bf2b18b..7c1361b1d 100644
--- a/generation.go
+++ b/generation.go
@@ -20,6 +20,7 @@ import (
 	"io/ioutil"
 	"os"
 	"runtime"
+	"runtime/debug"
 	"sync"
 	"syscall"
 	"time"
@@ -82,7 +83,11 @@ type generation interface {
 	// ID indicates the source -- path and generation number -- that
 	// this generation represents.
 	ID() string
+	// Dead indicates whether this generation is Done.
 	Dead() bool
+	// Bytes reports the storage associated with this generation, if any.
+	// DO NOT USE THIS. Except if you're debugging mmap segfaults.
+	Bytes() []byte
 }
 
 type mmapGeneration struct {
@@ -164,9 +169,36 @@ func (m *mmapGeneration) Transaction(fileP *io.Writer, fn func() error) (transac
 	}
 	// We are done locking the generation itself for now.
 	m.mu.Unlock()
+	wouldPanic := debug.SetPanicOnFault(true)
+	defer func() {
+		debug.SetPanicOnFault(wouldPanic)
+		if r := recover(); r != nil {
+			if err, ok := r.(error); ok {
+				// special case: if we caught a page fault, we diagnose that directly. sadly,
+				// we can't see the actual values that were used to generate this, probably.
+				if err.Error() == "runtime error: invalid memory address or nil pointer dereference" {
+					if transactionErr == nil {
+						transactionErr = errors.New("invalid memory access during transaction")
+					} else {
+						transactionErr = fmt.Errorf("invalid memory access during transaction, previous error %v", transactionErr)
+					}
+					return
+				}
+			}
+			if transactionErr == nil {
+				transactionErr = fmt.Errorf("panic during transaction: %v", r)
+			} else {
+				transactionErr = fmt.Errorf("panic during erroring transaction: panic %v, previous error %v", r, transactionErr)
+			}
+		}
+	}()
 	return fn()
 }
 
+func (m *mmapGeneration) Bytes() []byte {
+	return m.data
+}
+
 // Done marks the generation done, and closes its file, but may not unmap it.
 // It's still conceptually possible to end up doing a Transaction against a
 // done generation, but it's a red flag.
diff --git a/generation_test.go b/generation_test.go
index 3dd86fd8f..75444fcf7 100644
--- a/generation_test.go
+++ b/generation_test.go
@@ -1,4 +1,4 @@
-// Copyright 2019 Pilosa Corp.
+// Copyright 2020 Pilosa Corp.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -12,28 +12,56 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 //
-// +build generationdebug
+// +build generationparanoia
 
 package pilosa
 
 import (
-	"fmt"
-	"os"
+	"runtime"
 	"testing"
+	"unsafe"
 )
 
-func examineResults() {
-	results := reportGenerations()
-	if len(results) > 0 {
-		fmt.Printf("generations:\n")
-		for _, res := range results {
-			fmt.Printf("  %s\n", res)
-		}
+func TestGenerationPanic(t *testing.T) {
+	f := mustOpenFragment("i", "f", viewStandard, 0, "none")
+	defer f.Clean(t)
+
+	for i := 0; i < f.MaxOpN; i++ {
+		_, _ = f.setBit(0, uint64(i*32))
+	}
+	// force snapshot so we get a mmapped row...
+	_ = f.Snapshot()
+	_ = f.row(0)
+	var prevData []byte
+
+	if f.gen.(*mmapGeneration).data == nil {
+		t.Fatalf("generation code didn't create a mapping, apparently?")
+	}
+	prevData = f.gen.(*mmapGeneration).data
+	f.mu.Lock()
+	_ = f.snapshotQueue.Immediate(f)
+	f.mu.Unlock()
+	runtime.GC()
+	for i := 0; i < (f.MaxOpN / 2); i++ {
+		_, _ = f.setBit(0, uint64(i*32)+23)
+	}
+	f.mu.Lock()
+	f.snapshotQueue.Await(f)
+	f.mu.Unlock()
+	runtime.GC()
+	newData := f.gen.(*mmapGeneration).data
+	if unsafe.Pointer(&prevData[0]) == unsafe.Pointer(&newData[0]) {
+		t.Fatalf("test can't run usefully, didn't get new data pointer")
+	}
+
+	err := f.gen.Transaction(&f.storage.OpWriter, func() error {
+		prevData[0] = 0x3c
+		return nil
+	})
+	if err == nil {
+		t.Fatalf("expected a panic to get caught, but nothing happened")
+	}
+	if err.Error() != "invalid memory access during transaction" {
+		t.Fatalf("expected \"invalid memory access during transaction\", got %q", err.Error())
 	}
 }
-
-func TestMain(m *testing.M) {
-	ret := m.Run()
-	examineResults()
-	os.Exit(ret)
-}
diff --git a/logger/logger.go b/logger/logger.go
index 9e895d482..3486e24a2 100644
--- a/logger/logger.go
+++ b/logger/logger.go
@@ -105,3 +105,27 @@ func (cl *CaptureLogger) Printf(format string, v ...interface{}) {
 func (cl *CaptureLogger) Debugf(format string, v ...interface{}) {
 	cl.Debugs = append(cl.Debugs, fmt.Sprintf(format, v...))
 }
+
+// Logfer is a thing that has only a Logf() method, like for instance,
+// testing.T or testing.B.
+type Logfer interface {
+	Logf(format string, v ...interface{})
+}
+
+// LogfLogger is a logger that wraps something that has a Logf interface
+// and makes it act like our logger.
+type LogfLogger struct {
+	wrapped Logfer
+}
+
+func (ll *LogfLogger) Printf(format string, v ...interface{}) {
+	ll.wrapped.Logf(format, v...)
+}
+
+func (ll *LogfLogger) Debugf(format string, v ...interface{}) {
+	ll.wrapped.Logf(format, v...)
+}
+
+func NewLogfLogger(l Logfer) *LogfLogger {
+	return &LogfLogger{wrapped: l}
+}
diff --git a/mmap_test.go b/mmap_test.go
index 542c35763..1d2cbcfee 100644
--- a/mmap_test.go
+++ b/mmap_test.go
@@ -16,8 +16,10 @@ package pilosa
 
 import (
 	"math/rand"
-	"sync/atomic"
+	"runtime"
 	"testing"
+
+	"github.com/pilosa/pilosa/v2/logger"
 )
 
 type cv struct {
@@ -31,37 +33,28 @@ type cv struct {
 // a failure mode we've been bitten by once...
 func TestMmapBehavior(t *testing.T) {
 	depth := uint(6)
-	var done int64
 	f := mustOpenBSIFragment("i", "f", viewStandard, 0)
+	f.Logger = logger.NewLogfLogger(t)
 	defer f.Clean(t)
 
-	ch := make(chan struct{})
-
 	for i := 0; i < f.MaxOpN; i++ {
 		_, _ = f.setBit(0, uint64(i*32))
 	}
 	// force snapshot so we get a mmapped row...
-	_ = f.Snapshot()
-	row := f.row(0)
-	segment := row.Segments()[0]
-	bitmap := segment.data
-
-	// request information from the frozen bitmap we got back
-	go func() {
-		for atomic.LoadInt64(&done) == 0 {
-			for i := 0; i < f.MaxOpN; i++ {
-				_ = bitmap.Contains(uint64(i * 32))
-			}
-		}
-		close(ch)
-	}()
+	err := f.Snapshot()
+	if err != nil {
+		t.Fatalf("initial snapshot error: %v", err)
+	}
 
 	values := make([]cv, 1024)
 	for i := range values {
-		cols := make([]uint64, 512)
-		vals := make([]int64, 512)
+		cols := make([]uint64, 128)
+		vals := make([]int64, 128)
 		for j := range cols {
-			cols[j] = uint64(rand.Int63n(ShardWidth))
+			// pick values in the first 16 cols of each of the 16
+			// shards in a default shardwidth, so each set will
+			// probably change some values from the previous one.
+			cols[j] = uint64(((rand.Int63n(16) & int64(i>>2)) << 16) + rand.Int63n(16))
 			vals[j] = int64(rand.Int63n(1 << depth))
 		}
 		values[i] = cv{cols, vals}
@@ -69,15 +62,16 @@ func TestMmapBehavior(t *testing.T) {
 
 	// modify the original bitmap, until it causes a snapshot, which
 	// then invalidates the other map...
-	for j := 0; j < 5; j++ {
-		for i := 0; i < f.MaxOpN/int(depth+1); i++ {
-			cv := values[i%len(values)]
-			err := f.importValue(cv.cols, cv.vals, depth, (i%3 == 1))
-			if err != nil {
-				t.Fatalf("importValue[%d][%d]: %v", j, i, err)
-			}
+	for i := 0; i < 32; i++ {
+		cv := values[i%len(values)]
+		runtime.GC()
+		err := f.importValue(cv.cols, cv.vals, depth, (i%3 == 1))
+		if err != nil {
+			t.Fatalf("importValue[%d]: %v", i, err)
+		}
+		err = f.Snapshot()
+		if err != nil {
+			t.Fatalf("snapshot[%d]: %v", i, err)
 		}
 	}
-	atomic.StoreInt64(&done, 1)
-	<-ch
 }
diff --git a/roaring/roaring.go b/roaring/roaring.go
index 1ddc5f798..5992942c1 100644
--- a/roaring/roaring.go
+++ b/roaring/roaring.go
@@ -1529,6 +1529,33 @@ func (r *baseRoaringIterator) Current() (key uint64, cType byte, n int, length i
 	return r.currentKey, r.currentType, r.currentN, r.currentLen, r.currentPointer, r.lastErr
 }
 
+// SanityCheckMapping is a debugging function which checks whether containers
+// are *correctly* recorded as mapped or unmapped.
+func (b *Bitmap) SanityCheckMapping(from, to uintptr) (mappedIn int64, mappedOut int64, unmappedIn int64, errs int, err error) {
+	b.Containers.UpdateEvery(func(key uint64, c *Container, existed bool) (*Container, bool) {
+		dptr := uintptr(unsafe.Pointer(c.pointer))
+		if dptr >= from && dptr < to {
+			if c.Mapped() {
+				mappedIn++
+			} else {
+				err = fmt.Errorf("container key %d, addr %x, inside %x+%d\n",
+					key, dptr, from, to-from)
+				errs++
+				unmappedIn++
+			}
+		} else {
+			if c.Mapped() {
+				err = fmt.Errorf("container key %d, addr %x, outside %x+%d, but mapped\n",
+					key, dptr, from, to-from)
+				errs++
+				mappedOut++
+			}
+		}
+		return c, false
+	})
+	return mappedIn, mappedOut, unmappedIn, errs, err
+}
+
 // RemapRoaringStorage tries to update all containers to refer to
 // the roaring bitmap in the provided []byte. If any containers are
 // marked as mapped, but do not match the provided storage, they will

From cb686dcad0757083388102d2f3f87d40c0c2235a Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Wed, 5 Feb 2020 15:02:11 -0600
Subject: [PATCH 4/9] make mmap test experiment with different amounts of
 mapping

This is sort of prototype-ish, but the idea is that we use SetMaxMapCount
from syswrap, which already exists, to let us test edge cases like
"what happens if you only sometimes have mapped data".
---
 mmap_test.go     | 37 +++++++++++++++++++++++++++++++------
 server/server.go |  4 ++--
 syswrap/mmap.go  |  5 ++++-
 syswrap/os.go    |  4 +++-
 4 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/mmap_test.go b/mmap_test.go
index 1d2cbcfee..5ce157bc4 100644
--- a/mmap_test.go
+++ b/mmap_test.go
@@ -15,11 +15,13 @@
 package pilosa
 
 import (
+	"fmt"
 	"math/rand"
 	"runtime"
 	"testing"
 
 	"github.com/pilosa/pilosa/v2/logger"
+	"github.com/pilosa/pilosa/v2/syswrap"
 )
 
 type cv struct {
@@ -27,11 +29,7 @@ type cv struct {
 	vals []int64
 }
 
-// This test should basically never fail, but it might if you were running
-// out of available mmaps. Which you can fake up by adding '&& false' to the test
-// in newGeneration in generation.go. So this is probably useless but it's
-// a failure mode we've been bitten by once...
-func TestMmapBehavior(t *testing.T) {
+func forceSnapshotsCheckMapping(t *testing.T) {
 	depth := uint(6)
 	f := mustOpenBSIFragment("i", "f", viewStandard, 0)
 	f.Logger = logger.NewLogfLogger(t)
@@ -64,7 +62,11 @@ func TestMmapBehavior(t *testing.T) {
 	// then invalidates the other map...
 	for i := 0; i < 32; i++ {
 		cv := values[i%len(values)]
-		runtime.GC()
+		// periodically force gc, so if we have a small pool of maps
+		// we'll go in and out of mapping mode
+		if i%5 == 0 {
+			runtime.GC()
+		}
 		err := f.importValue(cv.cols, cv.vals, depth, (i%3 == 1))
 		if err != nil {
 			t.Fatalf("importValue[%d]: %v", i, err)
@@ -75,3 +77,26 @@ func TestMmapBehavior(t *testing.T) {
 		}
 	}
 }
+
+// This test should basically never fail, but it might if you were running
+// out of available mmaps. Which you can fake up by adding '&& false' to the test
+// in newGeneration in generation.go. So this is probably useless but it's
+// a failure mode we've been bitten by once...
+func TestMmapBehavior(t *testing.T) {
+	var changed bool
+	var original uint64
+	defer func() {
+		syswrap.SetMaxMapCount(original)
+	}()
+
+	for _, mmapMaxVal := range []uint64{0, 3} {
+		prev := syswrap.SetMaxMapCount(mmapMaxVal)
+		if !changed {
+			original = prev
+			changed = true
+		}
+		t.Run(fmt.Sprintf("maps%d", mmapMaxVal), func(t *testing.T) {
+			forceSnapshotsCheckMapping(t)
+		})
+	}
+}
diff --git a/server/server.go b/server/server.go
index e9edeb293..a8bf9e095 100644
--- a/server/server.go
+++ b/server/server.go
@@ -229,8 +229,8 @@ func (m *Command) SetupServer() error {
 	runtime.SetBlockProfileRate(m.Config.Profile.BlockRate)
 	runtime.SetMutexProfileFraction(m.Config.Profile.MutexFraction)
 
-	syswrap.SetMaxMapCount(m.Config.MaxMapCount)
-	syswrap.SetMaxFileCount(m.Config.MaxFileCount)
+	_ = syswrap.SetMaxMapCount(m.Config.MaxMapCount)
+	_ = syswrap.SetMaxFileCount(m.Config.MaxFileCount)
 
 	err := m.setupLogger()
 	if err != nil {
diff --git a/syswrap/mmap.go b/syswrap/mmap.go
index 95e819999..4b680780a 100644
--- a/syswrap/mmap.go
+++ b/syswrap/mmap.go
@@ -34,10 +34,13 @@ var ErrMaxMapCountReached = errors.New("maximum map count reached")
 var maxMapCount uint64 = 60000
 var mu sync.RWMutex
 
-func SetMaxMapCount(max uint64) {
+// SetMaxMapCount sets the maximum map count, and returns the previous maximum.
+func SetMaxMapCount(max uint64) uint64 {
+	prev := maxMapCount
 	mu.Lock()
 	maxMapCount = max
 	mu.Unlock()
+	return prev
 }
 
 // Mmap increments the global map count, and then calls syscall.Mmap. It
diff --git a/syswrap/os.go b/syswrap/os.go
index 1704b0759..67f87fcd4 100644
--- a/syswrap/os.go
+++ b/syswrap/os.go
@@ -27,10 +27,12 @@ var fileCount uint64
 var maxFileCount uint64 = 500000
 var fileMu sync.RWMutex
 
-func SetMaxFileCount(max uint64) {
+func SetMaxFileCount(max uint64) uint64 {
+	prev := maxFileCount
 	fileMu.Lock()
 	maxFileCount = max
 	fileMu.Unlock()
+	return prev
 }
 
 // OpenFile passes the arguments along to os.OpenFile while incrementing a

From 337e451cc75031300eecb48df7c4d8ed5df9d7bc Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Fri, 7 Feb 2020 11:43:10 -0600
Subject: [PATCH 5/9] lint and review changes

Log an error in the probably-irrelevant case where we ended up with
a file, but Stat failed, which shouldn't ever happen we hope anyway.

Also explicitly discard the status from RemapRoaringStorage in a case
where we don't care.
---
 fragment.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fragment.go b/fragment.go
index dfc0e759c..13a4c8621 100644
--- a/fragment.go
+++ b/fragment.go
@@ -296,6 +296,9 @@ func (f *fragment) applyStorage(data []byte, file *os.File, newGen generation, m
 	if len(data) == 0 {
 		if file != nil {
 			fi, err := file.Stat()
+			if err != nil {
+				f.Logger.Printf("trying to apply new storage to existing bitmap, stat failed: %v", err)
+			}
 			if err == nil && fi != nil && fi.Size() == 0 {
 				return f.emptyStorage(file)
 			}
@@ -305,7 +308,7 @@ func (f *fragment) applyStorage(data []byte, file *os.File, newGen generation, m
 		// our containers to use that storage *to take advantage of
 		// mmap*, we'll just make sure our containers aren't pointing to
 		// old storage and say "nope".
-		f.storage.RemapRoaringStorage(nil)
+		_, _ = f.storage.RemapRoaringStorage(nil)
 		f.storage.SetSource(nil)
 		return false, nil
 	}

From 99d865c2eab56cd178b4742731f42626e75bee04 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Fri, 7 Feb 2020 12:29:31 -0600
Subject: [PATCH 6/9] ensure that we've unrequested mapping when applying empty
 storage

---
 fragment.go | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fragment.go b/fragment.go
index 13a4c8621..e0e4743ac 100644
--- a/fragment.go
+++ b/fragment.go
@@ -294,6 +294,9 @@ func (f *fragment) importStorage(data []byte, file *os.File, newGen generation,
 // to use a new storage as backing store.
 func (f *fragment) applyStorage(data []byte, file *os.File, newGen generation, mapped bool) (bool, error) {
 	if len(data) == 0 {
+		// This shouldn't be used anyway in this path, but just in
+		// case, we'll be explicit about it.
+		f.storage.PreferMapping(false)
 		if file != nil {
 			fi, err := file.Stat()
 			if err != nil {

From 7841a660a8775e45a7c6edfc5064db353c874405 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Fri, 7 Feb 2020 12:39:31 -0600
Subject: [PATCH 7/9] make sure setArray isn't copying mapped data addresses by
 accident in unionInPlace

---
 roaring/roaring.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/roaring/roaring.go b/roaring/roaring.go
index 5992942c1..99a87a0d9 100644
--- a/roaring/roaring.go
+++ b/roaring/roaring.go
@@ -3779,7 +3779,10 @@ func unionArrayArrayInPlace(a, b *Container) *Container {
 			// for InPlace, we actually want to ensure that
 			// we update a, as long as it's not frozen.
 			a = a.Thaw()
-			a.setArray(b.array())
+			// ... but we also want to be sure we don't end up
+			// copying in a mapped object into our not-mapped
+			// object.
+			a.setArrayMaybeCopy(b.array(), b.Mapped())
 			return a.optimize()
 		}
 		return a

From ba7db3028bc5173fec422fec91825c5a60b33ef5 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Fri, 7 Feb 2020 12:55:49 -0600
Subject: [PATCH 8/9] sanity-check: check whether containers are flagged as
 mapped before mapping

In the old unmarshal code, the decision to mark a thing as mapped (always
yes) happens separately from setting the mapping. What if this could ever
somehow possibly go wrong? Let's sanity-check that to be extra careful.
---
 roaring/unmarshal_binary.go | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/roaring/unmarshal_binary.go b/roaring/unmarshal_binary.go
index e80e834cf..208c15d53 100644
--- a/roaring/unmarshal_binary.go
+++ b/roaring/unmarshal_binary.go
@@ -88,7 +88,12 @@ func readOffsets(b *Bitmap, data []byte, pos int, keyN uint32) error {
 
 		// Map byte slice directly to the container data.
 		citer.Next()
-		_, c := citer.Value()
+		k, c := citer.Value()
+		if !c.Mapped() {
+			fmt.Printf("inexplicable: container %d (%d/%d) doesn't think it's mapped. fixing that.\n",
+				k, i, keyN)
+			c.setMapped(true)
+		}
 		switch c.typ() {
 		case containerArray:
 			c.setArray((*[0xFFFFFFF]uint16)(unsafe.Pointer(&data[offset]))[:c.N():c.N()])
@@ -108,7 +113,12 @@ func readWithRuns(b *Bitmap, data []byte, pos int, keyN uint32) error {
 	citer, _ := b.Containers.Iterator(0)
 	for i := 0; i < int(keyN); i++ {
 		citer.Next()
-		_, c := citer.Value()
+		k, c := citer.Value()
+		if !c.Mapped() {
+			fmt.Printf("inexplicable: container %d (%d/%d) doesn't think it's mapped. fixing that.\n",
+				k, i, keyN)
+			c.setMapped(true)
+		}
 		switch c.typ() {
 		case containerRun:
 			runCount := binary.LittleEndian.Uint16(data[pos : pos+runCountHeaderSize])
@@ -176,12 +186,17 @@ func (b *Bitmap) unmarshalPilosaRoaring(data []byte) error {
 
 		// Map byte slice directly to the container data.
 		citer.Next()
-		_, c := citer.Value()
+		k, c := citer.Value()
 
 		// this shouldn't happen, since we don't normally store nils.
 		if c == nil {
 			continue
 		}
+		if !c.Mapped() {
+			fmt.Printf("inexplicable: container %d (%d/%d) doesn't think it's mapped. fixing that.\n",
+				k, i, keyN)
+			c.setMapped(true)
+		}
 		switch c.typ() {
 		case containerRun:
 			runCount := binary.LittleEndian.Uint16(data[offset : offset+runCountHeaderSize])

From d742c67317b047d2afd11fde07c22b9426befe0d Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Fri, 7 Feb 2020 13:18:12 -0600
Subject: [PATCH 9/9] avoid race on max count reads and writes

---
 syswrap/mmap.go | 2 +-
 syswrap/os.go   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/syswrap/mmap.go b/syswrap/mmap.go
index 4b680780a..238f6dd6f 100644
--- a/syswrap/mmap.go
+++ b/syswrap/mmap.go
@@ -36,8 +36,8 @@ var mu sync.RWMutex
 
 // SetMaxMapCount sets the maximum map count, and returns the previous maximum.
 func SetMaxMapCount(max uint64) uint64 {
-	prev := maxMapCount
 	mu.Lock()
+	prev := maxMapCount
 	maxMapCount = max
 	mu.Unlock()
 	return prev
diff --git a/syswrap/os.go b/syswrap/os.go
index 67f87fcd4..61715f5bb 100644
--- a/syswrap/os.go
+++ b/syswrap/os.go
@@ -28,8 +28,8 @@ var maxFileCount uint64 = 500000
 var fileMu sync.RWMutex
 
 func SetMaxFileCount(max uint64) uint64 {
-	prev := maxFileCount
 	fileMu.Lock()
+	prev := maxFileCount
 	maxFileCount = max
 	fileMu.Unlock()
 	return prev