fix(leveldb): flush by rotating the memtable; open the db atomically - #29661
Open
chrisnojima wants to merge 3 commits into
Open
chrisnojima wants to merge 3 commits into
chrisnojima wants to merge 3 commits into
Conversation
Flush used to write a sentinel key and then CompactRange the whole key space just to force the memtable out. That rewrites tables that had nothing to do with the flush. Open a transaction and immediately discard it instead: goleveldb rotates a non-empty memtable and waits for it to land in a table, without compacting anything. Concurrent calls serialize on goleveldb's own write lock, so a call that arrives late still covers every write made before it. The lazily-opened db field was assigned under the *read* lock, racing every other reader. Make it an atomic.Pointer, and have OpenTransaction return LevelDBOpenClosedError instead of dereferencing a nil db after Close. The cleaner no longer runs a separate monitorAppState goroutine with its own cancel channel; clean() samples State()/NextUpdate() inside the same critical section that takes the running flag, so a transition can't slip through between the sample and the batch loop.
zoom-ua
reviewed
Sep 22, 2026
| l.G().Log.Debug("| Opening LevelDB options: %+v", l.Opts()) | ||
| l.db, err = leveldb.OpenFile(fn, l.Opts()) | ||
| db, openErr := leveldb.OpenFile(fn, l.Opts()) | ||
| err = openErr |
| // openedDb returns the DB without triggering a lazy open, or nil if it isn't | ||
| // open. Callers must hold the read lock. | ||
| func (l *LevelDb) openedDb() *leveldb.DB { | ||
| return l.db.Load() |
Collaborator
There was a problem hiding this comment.
no need for a 1 line wraper. we call db.Load() directly extensively throughout this diff already..
|
|
||
| // A lazy open racing a Nuke reopens rather than reporting closed. | ||
| for i := 0; i < 8; i++ { | ||
| wg.Add(2) |
Collaborator
There was a problem hiding this comment.
post go 1.25 use wg.Go. agents should run go fix on diffs to fix these kinds of issues they're not trained on.
…own db goleveldb's OpenTransaction keeps its write lock when rotating the memtable or waiting on compaction fails, e.g. a journal create hitting ENOSPC. Flushing through it could leave every later write, and Close, blocked for good. It also held the lock for the whole table write. Flush now calls FlushMemdb from the keybase/goleveldb fork, which releases the lock before waiting and on every error; the fork also fixes OpenTransaction itself. OpenTransaction goes through the lazy open like every other operation instead of reporting a never-opened db as closed. The open db is handed to each action rather than reloaded. A clean keeps the db and stop channel it started with, so a Nuke and reopen mid-clean can't redirect it to the new db. start() resets lastKey and always installs a fresh LRU, which retires isShutdown. Status and clearCache read the cache pointer under its lock.
…lock Close closed goleveldb before stopping the cleaner, so a running clean could hold an iterator across Close, which goleveldb says is unsafe. Stop now runs first, detaches the cleaner from its db, and waits for a running clean to exit; the clean's between-batch sleep wakes on stop. OpenTransaction waited on goleveldb's write lock while holding our read lock. A Close or Nuke queued behind it blocked every new reader, including a Get from the goroutine holding the current transaction, so nobody could proceed. It now opens the db lazily, then waits outside the lock. Flush goes through the same error handling as other operations, so a full disk starts a forced clean and a corrupt db is nuked. Only Nuke resets the cleaner's position; Close and reopen keep the same data, so cleaning resumes where it stopped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #29637. Based on
master, independent of the app-state stack.Why
Flush()exists to get pending writes onto disk before the app is suspended or killed. The way it did that had real costs, and the db handling around it had several latent correctness bugs. These changes can be reviewed separately from the app-state work.Master bugs this fixes
Flush()puts a sentinel key (pm:ff:flush-sentinel), callsCompactRangeover the whole range, then deletes the key. Every flush writes a bogus key into the "pm" table and pays for a full-keyspace compaction, when all that's needed is getting the memtable onto disk.dbfield under the read lock, which is shared, so the assignment races every reader that doesn't go throughdbOpenerOnce.OpenTransaction()dereferences an unopened or closed db instead of opening it lazily or returning an error.closeLockedcloses goleveldb before stopping the cleaner, so a clean can hold an iterator acrossClose, which goleveldb documents as unsafe.monitorAppStategoroutine with its own cancel channel, so it misses a transition that lands between the state sample and the batch loop.setDbonly set the field, so the cleaner stayed shut down.Decision for review: this depends on a patch to our goleveldb fork
Please evaluate this choice explicitly. go.mod moves the
github.com/syndtr/goleveldb => github.com/keybase/goleveldbreplace from9881c0c(our fork'smaster) toe81a996. That commit sits on the fork branchnojima/flush-memdband is not merged into the fork'smaster. If this approach is accepted, that branch should get its own PR against the fork and be merged, and go.mod repinned to the merge commit, before this PR lands.What the fork commit changes (about 40 lines plus tests, in
leveldb/db_write.goandleveldb/db_transaction.go):OpenTransactionreleases the write lock when it fails. It takeswriteLockCand then returns without releasing it if rotating the memdb or waiting on compaction fails. For example, creating the new journal file fails with ENOSPC on a full phone. After that, everyPut/Write, andDB.Closeitself, blocks forever. OurClose/Nukethen deadlock too, because they wait on callers stuck holding our read lock. Upstreamsyndtr/goleveldbmasterhas the same bug.CompactRangereleases the lock on the matching error path, so this is an oversight, not a design choice.DB.FlushMemdb(). It swaps a non-empty memdb out under the write lock, releases the lock, waits for the memdb compaction to write the table, and returns once every earlier write is in a table. It runs no table compaction. This is the memdb half ofCompactRange, in the same order.Why a fork patch instead of working around it in this repo:
CompactRange(what master does): after the memdb flush it always runs a table compaction over the range. Any range that makes it flush the memtable also overlaps the new table, so every flush compacts tables. That cost is what this PR set out to remove.OpenTransaction+Discard(the previous revision of this PR): it does flush the memtable only, but it hits the lock leak above. It also holds the write lock for the whole table write, so every concurrent chat write stalls for that time. The write buffer is 12 MB.writeLockCis unexported, andDB.Closeneeds the same lock, so there's no way to recover from outside.What it costs: we carry about 40 more lines of divergence from upstream in the fork. The rejected alternative is to keep master's sentinel + full-range
CompactRangeflush and accept its compaction cost. That needs no fork change.Fork test coverage (
leveldb/db_flush_test.go, each mutation-checked: reverting the fix makes it fail):FlushMemdbadds exactly one table, compacts none, and writes no table when the memdb is empty.FlushMemdbandOpenTransactionboth leave the write lock free.What this changes here
Flush()callsdb.FlushMemdb()under our read lock. It's still a no-op when the db isn't open, and it never triggers a lazy open. Its errors go through the same handling as every other operation: a full disk starts a forced clean, and a corrupt db is nuked.dbfield becomes anatomic.Pointer.doWhileOpenAndNukeIfCorruptedpasses the opened db to its action, so callers don't reload it.OpenTransaction()goes through that same lazy-open path. It then waits on goleveldb's write lock outside our read lock. Otherwise a Close or Nuke queued behind it would block every new reader, including the goroutine holding the current transaction, and nothing could proceed.clean()samplesState()/NextUpdate()in the same critical section that takes the running flag.Stop()detaches the cleaner from its db and waits for a running clean to exit before goleveldb is closed. The clean's between-batch sleep wakes on stop.start(db)replacessetDb(db)and installs a fresh LRU, soisShutdowngoes away.Nukeresets the cleaner's position. Close followed by a reopen keeps the same data, so cleaning resumes where it stopped.Status()andclearCache()now read the cache pointer under its lock.One behaviour change to note in review
The old cancel goroutine was started only on mobile (
isMobile). The new in-loop sampling runs everywhere. On desktopMobileAppStatestays FOREGROUND and never fires, so a clean runs to completion exactly as before. The difference is that the code path is now shared rather than mobile-only, and the cleaner no longer storesisMobile.Not changed
A
pm:ff:flush-sentinelkey can already be sitting in a db, left there if master'sFlushwas killed between itsPutandDelete. This PR leaves it alone: it's a few bytes in the permanent table and nothing reads it. Deleting it would mean a lookup on every open, kept around forever.Ordering
No file overlap with the stack, so it can merge in any position. Like the other two standalone PRs, it's best landed before or with #29651, since
leveldb_cleaner.gohas one of the three loops that hardcode a FOREGROUND seed. It also needs the fork branch merged first (see above).Verification
go build ./...,go vet, andgofmtare clean, andgolangci-lint --new-from-rev masterreports 0 issues. All leveldb and cleaner tests pass under-race.OpenTransaction;OpenTransactionwaiting while a Nuke is queued; a clean restarting from the first key after Nuke but resuming after Close and reopen; Close waiting for a running clean; a clean after Close doing nothing;Statusracing a reopen. Each failed before its fix and was mutation-checked against it.Flush()errors through the shared error handler.OpenFileoffers no way to inject ENOSPC or corruption into a flush.