fix(drivers): correct data-loss, memory-safety and lifecycle bugs - #3126
fix(drivers): correct data-loss, memory-safety and lifecycle bugs#3126gaby wants to merge 70 commits into
Conversation
leveldb: - add the missing DeleteWithContext method, without which *Storage did not satisfy the storage.Storage interface it documents - stop reporting every read failure as a cache miss in Get - always write the expiration envelope and only treat a decoded envelope as one when it carries a value, so values that are themselves JSON objects are no longer read back as nil - delete keys through a batch in Reset instead of mutating during iteration - make Close idempotent pebble: - Reset only flushed the memtable, so it never removed any key - Get surfaced pebble.ErrNotFound instead of the documented nil, nil - release the value closer on every path and stop swallowing decode errors - round sub-second expirations up so they no longer expire immediately - panic instead of log.Fatal, a library must not exit the process bbolt: - copy values out of Get, the returned slice pointed into the mmap and was only valid for the life of the transaction - recreate the bucket in Reset instead of deleting while iterating over it - return an error instead of dereferencing a missing bucket badger, memory: - make Close idempotent, a second call blocked forever on the done channel - compare badger.ErrKeyNotFound with errors.Is and stop dropping read errors ristretto: - wait for the asynchronous write buffer to drain in Set, so a Get that follows Set observes the value - copy values in and out of the cache All drivers: the *WithContext methods now honour an already cancelled context, as the storage interface and the TCK conformance suite require.
Adds regression tests for the data-loss, memory-safety and lifecycle fixes, including a bbolt test that forces the database to remap so an aliased Get result would be observable, and a redis test asserting that New no longer panics when the connection check is skipped. The pebble Reset and Delete tests asserted the buggy behaviour, they now assert the documented contract instead. READMEs no longer describe the context methods as dummies, and bbolt and pebble document their expiration semantics.
pebble:
- derive the expiration deadline from the real time rather than the
truncated Created stamp and round it up, entries expired up to a second
early otherwise
- configDefault recursed into itself when called with no argument, so
pebble.New() overflowed the stack
leveldb:
- mark envelopes with an explicit version field, the previous shape
heuristic still misread a legacy raw payload such as {"value":"aGk="}
- flush Reset and the collector in bounded batches instead of buffering the
whole keyspace in memory
- document that entries are stored as an envelope, which is visible through
Conn()
badger, leveldb, memory:
- closing the done channel dropped the handshake that guaranteed the
collector was idle, so Close could race an in-flight value log GC. Wait
for the collector to return, and record the close result so repeated
Close calls report it instead of a spurious 'already closed' error.
ristretto:
- correct the Set comment, waiting makes an admitted write visible, it does
not make admission certain
Test databases are no longer generated inside the package directories, and
the ones previously committed are removed and ignored.
…eset leveldb: requiring the version marker rejected the unmarked envelopes that earlier versions wrote for keys with an expiration, so Get returned the raw envelope JSON and gc never expired those entries. An unmarked document is now read as an envelope when it has exactly the two fields such an envelope had, which still leaves a bare JSON payload untouched. pebble: Reset buffered a delete for every key in a single batch, so peak memory scaled with the key count. It now commits in bounded chunks, the same way leveldb.Reset does.
The same truncation the previous commits fixed in pebble applies to several other drivers, where a duration below a second collapses to zero and zero does not mean 'expire now': - memory: uint32(exp.Seconds()) made a sub-second expiration immediate, so the very next Get was a miss. The deadline is now derived from the current time and rounded up, as in pebble. - memcache: an expiration of zero means 'never expires' in memcached, so a sub-second expiration turned into a permanent entry. Expirations above the 30 day limit are also converted to the absolute Unix timestamp memcached expects, they used to be sent as a relative value and read as a timestamp in the past. - cassandra, scylladb: a TTL of zero means 'no TTL', with the same effect. - etcd: an expiration of zero attached the key to a lease with a TTL of zero, which etcd raises to its own minimum, so a key that was meant to live forever was dropped seconds later. Keys with no expiration are now written without a lease, and sub-second expirations round up. Also stops discarding the error from the pebble Reset iterator's Close, which reports child iterator teardown failures that Error never sees, and removes a require call from inside a require.Eventually condition, which testify runs on its own goroutine.
leveldb: an unversioned envelope is indistinguishable from a payload that happens to be the same JSON object, so an expired one is now reported as a miss without being deleted, by Get and by the collector alike. An envelope carrying a version this driver does not understand is reported as an error rather than handed back as if the envelope were the value. ristretto: Wait, Del and Clear all block on buffers that Close closes, so a Set racing a Close could panic or hang forever. Operations now hold a read lock and Close holds the write lock, which also makes Close idempotent and turns use-after-close into an error instead of a crash. memcache: an expiration far enough in the future overflowed the 32 bit expiration field and wrapped into a timestamp in the past, expiring the entry immediately. It is clamped to the largest value the field can hold. pebble: ConfigDefault set WriteOptions to a zero value, meaning asynchronous writes, while a caller-supplied config left it nil, which Pebble reads as synchronous. Now that New() works, the two paths would have differed in durability, so the default is nil as well.
…lack leveldb: refusing to delete an expired unversioned envelope left every such entry on disk forever after an upgrade, and it bought nothing: an entry that was misdetected already hands back the wrong value from Get, so declining to reclaim it protects nothing. Those entries expire and are reclaimed again, exactly as earlier versions handled them, and the README records that the unmarked shape is ambiguous for databases written before this version. memory: clamp the computed deadline so a very distant expiration cannot wrap the 32 bit field, and document the accuracy the second-granularity cached clock gives, an entry may outlive its expiration by up to two seconds and is never dropped early. Reading the real clock per Get would about halve this storage's throughput, which is why the cached one exists.
…ive TTLs leveldb: decode read every entry twice, once as a map to test for the version field and once as the struct, which about halved Get throughput. The version field is now a pointer, so its absence is visible from the single struct pass and only an unversioned document pays the second one. ristretto: Reset held the lock for reading, but Ristretto documents Clear as non-atomic and assumes nothing else is in flight, so a concurrent Set could report success for a write Clear had already discarded. It takes the lock exclusively now. memory: a negative expiration is treated as no expiration, matching the other drivers; it used to wrap into a far-future deadline. memcache: the absolute timestamp used past the 30 day limit is derived from the rounded seconds rather than the raw duration, which was undoing the round-up and expiring entries up to a second early.
Every driver read a negative expiration as a deadline in the past, so Set(key, value, -time.Hour) silently stored an entry that was already gone. Set now treats anything at or below zero as no expiration, in leveldb, badger, ristretto, memory, arangodb, mongodb, nats, neo4j, aerospike, sqlite3, mysql, postgres and mssql alike. memory: expirations are stored as an exact nanosecond deadline instead of a whole second compared against a clock refreshed once a second, which had compounded into a two second floor: a 100ms entry lived for 2s. It now expires at 100.1ms. Get only reads the clock for entries that have an expiration, so throughput is unchanged (33ns/op, same as before). ristretto: a write Ristretto drops rather than buffering is reported as an error instead of being silently lost. pebble: document what the nil WriteOptions default means, a disk flush per write, and how to trade it for throughput.
badger: Entry.WithTTL truncates the deadline to a whole second, so a sub-second expiration landed on the current second and the entry was gone almost immediately. The rounded up deadline is set directly instead. pebble: Close is now idempotent, a second call used to panic with 'pebble: closed', unlike every other driver hardened here. memory: a deadline past the year 2262 saturates instead of wrapping to a negative one, which made the entry disappear on the next Get. aerospike: the seconds conversion truncated, so a 1500ms expiration became a one second TTL. redis: go-redis reads a negative expiration as its KeepTTL sentinel, which carried the previous expiration over instead of clearing it. clickhouse: the last driver still reading a negative expiration as a deadline in the past. ristretto: a dropped write is back-pressure from a cache rather than a failure, so Set no longer turns it into an error. The sqlite3 test database is no longer generated in the package directory, and the one that had been committed is removed and ignored.
…TL math memory: the internal timestamp updater existed only to avoid reading the clock on the hot path, which the exact nanosecond deadlines replaced. It has no callers left, so it goes, along with the goroutine it started. Sweeps over the whole map read the clock once rather than once per entry. aerospike: compute the TTL in an explicit int64 and clamp it to the uint32 Aerospike carries it in, instead of relying on Duration arithmetic that read as a comparison against one nanosecond. redis: document that an expiration at or below zero clears any expiration the key had, rather than reaching go-redis as its KeepTTL sentinel.
…fault TTL configDefault compared int(cfg.GCInterval.Seconds()) against zero, so any interval below a second read as zero and was silently replaced by the ten second default. A store configured to collect every 50ms collected every ten seconds instead, in arangodb, badger, memory, mssql, mysql, neo4j, postgres, sqlite3 and surrealdb. leveldb already compared the duration itself, which is what the others do now. Found by probing the memory collector: an entry with a 50ms expiration was still in the map 400ms after a 50ms collection interval was configured. cassandra: the same truncation applied to the configured default expiration, where a sub-second value became a TTL of 0, which Cassandra reads as no TTL at all rather than as an immediate one.
… drivers Three of the six findings from the review are addressed; the other three are noted below. Close double-call: mysql, postgres, mssql, sqlite3, neo4j and arangodb still blocked forever on a second call, because the first consumed the only receive on the unbuffered done channel, and surrealdb panicked closing an already closed one. They now use the same sync.Once plus stopped channel as badger, leveldb, memory and pebble, so Close is idempotent and waits for the collector to stop touching the database. Sub-second expirations: mysql, postgres, mssql, sqlite3, neo4j, arangodb, nats, surrealdb and clickhouse stored the deadline as time.Now().Add(exp) truncated to a whole second, so an entry expired up to a second early and a sub-second expiration could be written already past. The deadline is rounded up, as in the drivers fixed earlier. Tests that asserted the truncating behaviour are updated: the ones that slept just past a whole-second expiration now poll, and the GC tests collect as of a moment past the rounded deadline rather than as of now. redis: SkipConnectionCheck now also covers the Reset flush. Combining it with Reset against an unreachable server still panicked, on exactly the error the option exists to opt out of. Not changed, with reasons: the TTL round-up and the GCInterval clamp are duplicated across drivers because each driver is its own Go module and they share no code by design; unifying them would mean making every driver depend on a common module. The leveldb decode double-parse only affects documents written before the version marker existed, entries this version writes are parsed once.
… tests redis: SkipConnectionCheck swallowed every Reset failure, not only the unreachable-server one it documents. A flush that fails because the server rejected the command, a permission denial for instance, panics again. Close could hang on a stalled collector sweep in mysql, postgres, mssql, sqlite3, neo4j and arangodb, because the sweep query ran on a background context that nothing cancelled. The sweep now runs on a context cancelled by Close, so it is abandoned rather than holding the caller open. This was true before this branch too: the old unbuffered send blocked on the same query. Tests: the expiration tests in postgres, cockroachdb and nats slept just past a whole-second expiration, which the deadline round-up made too tight, so they now poll. The arangodb pair asserted against a second, freshly created store rather than the one holding the key, which made it pass no matter what; each test now writes and reads through the same store. The two remaining review findings are the TTL round-up and Close/GC shutdown blocks being repeated per driver. They stay repeated: every driver is its own Go module with no shared dependency, so factoring them out would mean making all of them depend on a common module.
mysql, postgres: a caller-supplied *sql.DB or pgxpool.Pool was closed when initialization failed, taking down a connection the caller owns and may still be using. Only a connection this driver opened is closed now. arangodb: Close cleared the driver, collection, connection and binding params with no lock, so a Get or Set still in flight dereferenced nil. It stops the collector and leaves the fields alone, which turns a late call into a driver error rather than a crash. bbolt: Close is idempotent, it was the last driver where a second call went straight through to the database, and its own suite double-closes it. Reset deletes through a cursor instead of dropping and recreating the bucket, which also reset the bucket sequence that Conn callers can read. mongodb: an entry was dropped up to a second before it expired, because Get truncated both the stored deadline and the current time to whole seconds. It compares the deadline itself. redis: SkipConnectionCheck now skips the Reset flush outright rather than attempting it and classifying the failure. The option exists so New makes no network call, and flushing is one.
…d handles arangodb: bindingParams was never initialized and the collector wrote to it, so the first sweep panicked with an assignment to a nil map and took the process down, one GCInterval after New. The same field was shared between the collector and callers with no lock. Bind variables are passed per query now, which removes both, and the query cursor is closed rather than leaked. mysql, postgres, neo4j, redis: Close closed the connection even when the caller had supplied it through Config.Db, Config.DB or NewFromConnection, pulling a handle out from under the rest of their application. Each storage tracks whether it opened the connection and only closes what it owns. This is the same ownership bug already fixed on the initialization path. aerospike: a negative expiration fell back to the configured default TTL instead of meaning no expiration, which is what the sentinel TTLDontExpire is for. The clamp also stops short of the two reserved TTL values rather than landing on them.
… support neo4j: the ownsDB guard covered Close but not the two initialization error paths, which still closed a caller-supplied driver on a reset or index failure. scylladb: Close closed a session supplied through Config.Session, which stays the caller's to close. surrealdb: the collector swept on an uncancellable background context, so a stalled query could hold Close open. Its context methods also discarded the context outright even though the SurrealDB client takes one, so they are now the real implementations and the context-free variants delegate to them; the collector's sweep runs on the context Close cancels. aerospike: a negative Config.Expiration fell through to the one-second minimum instead of meaning no expiration, the same way a negative argument to Set now does. bbolt: the bucket-missing error is exported, callers had no way to tell it apart with errors.Is. Tests: a Close_Twice case for mysql, postgres, mssql, neo4j, arangodb and surrealdb, which got the idempotent Close without one; and the redis NewFromConnection test closes the client it opened, now that Close on a borrowed one is correctly a no-op.
mysql, postgres, neo4j: every other initialization failure released a connection this driver had opened, but the ping and connectivity check panicked while still holding it, leaking the handle. scylladb: New closed a caller-supplied session on a keyspace or drop failure, ignoring the ownership flag added for exactly that; and Close had no idempotency guard, unlike the sibling drivers. Both fixed. redis: Close is idempotent too, the last driver without a guard. cassandra: the TTL round-up existed twice in one file and had already drifted between the two copies; they share one helper now. The same logic stays repeated across drivers, which are separate Go modules with nothing to share through.
scylladb: one init failure path, the table creation, still closed a caller-supplied session unconditionally. redis: an owned client leaked when the ping or the reset flush failed during New. Close is idempotent, and a Close on a borrowed client is covered by a test that checks the owner's client still works afterwards. mongodb: Close is idempotent, it was the last driver where a second call went through to the client. leveldb: an unknown envelope version is only reported as one when the document also carries a value, so a pre-existing payload that merely happens to have a field of that name is still returned as the payload it is. postgres: dropped a redundant ownership check, closeOwned already makes it. ristretto: SkipWaitForWrite lets throughput-sensitive callers keep Ristretto's buffered write behaviour. It stays off by default, so a Get after a Set sees the value. Adds Close_Twice coverage for redis and scylladb, and a memory benchmark for the path that reads the clock: Get costs 34ns with no expiration and 83ns with one, which is what exact expirations cost now that the second-granularity cached clock is gone.
…our ctx in memcache badger, mssql, sqlite3, surrealdb: an initialization failure after the connection was opened left it, and in badger's case the directory lock, behind. They release it now, like the drivers fixed earlier. memcache: the context methods discarded the context, the last driver still doing so. They reject a context that is already cancelled or past its deadline before touching the storage, and the README no longer calls them dummies. pebble: documents that a Reset spanning more than one chunk is not atomic, a concurrent reader can see the database part way through. Pebble offers no multi-batch transaction, and the alternative is a memory cost that scales with the key count.
…TL granularity mongodb: NewWithContext was missed by the connection-release work entirely. Its ping, drop and two index-creation failures panicked while still holding the client. mysql, postgres, mssql, scylladb: every constructor released an owned connection on each error path except the last one, checkSchema, which panics from inside itself. It runs behind a recover that releases the connection and re-panics, so the schema error still reaches the caller unchanged. bbolt: New leaked the file handle, and the OS lock with it, when the bucket reset or creation failed. Expirations stored as a whole-second deadline are rounded up, so an entry outlives its expiration by up to a second rather than being dropped early. That was only documented on some drivers; mysql, postgres, mssql, sqlite3, neo4j, arangodb, nats, surrealdb and clickhouse now say so too.
nats: a value the driver could not gob-decode was reported as expired, which deleted it and hid the decode error behind a cache miss. It is returned as the error it is. neo4j, surrealdb: the cleanup that releases a connection on an initialization failure ran on the caller's context, which may be exactly what failed; a context already done would have skipped the close it exists to perform. Closing runs on its own context. surrealdb: Get deleted an expired entry on a background context rather than the caller's, unlike the collector in the same file. memory: Conn handed out the live map, so reading it raced the garbage collector writing to it. It returns a snapshot instead. mongodb: Get took an item from the pool and never returned it, so the pool never served a second read. The item goes back on every path and the value is copied out first, since the caller keeps it.
…nup closes mongodb: returning Get's item to the pool, which the previous commit introduced, exposed that releaseItem never cleared ObjectID. Get decodes into the pooled item, so the next Set reusing it would have carried another document's _id and been rejected. It is cleared with the rest of the fields. neo4j, surrealdb: the cleanup close that runs when initialization fails now uses a bounded context rather than an unbounded background one, so a stuck connection cannot hang the constructor in the very path that exists to release it.
The cleanup that releases the client when initialization fails still ran on an unbounded context, unlike the equivalent paths in neo4j and surrealdb, so a stuck connection could hang the constructor in the path meant to release it.
…rd arangodb after Close clickhouse, aerospike, cassandra: each leaked its connection when a step after opening it failed. Cassandra's was the worst of the three, since New returns nil to the caller, leaving nothing that could close the session afterwards. clickhouse, etcd: Close is idempotent, the last two drivers without a guard. arangodb: ArangoDB has no connection to tear down, so after Close the storage silently kept talking to the server. Operations take a read lock and report errClosed once Close has taken it for writing, which also waits for calls already in flight rather than clearing fields underneath them. mongodb: Close is bounded like the constructor cleanup, since the interface gives it no context and a stuck connection must not hang the caller. Adds a test that releaseItem clears every field, the regression the previous commit introduced. surrealdb README no longer describes the context methods as no-ops, they pass the context to the client. Hand-rolled byte copies are bytes.Clone.
…ed closes leveldb, pebble: Reset threw away the deletes it had already queued when the iterator failed part way through. Earlier chunks are committed by then, so the pending one is flushed before the iteration error is reported. etcd: a Set whose Put failed left the lease it had just been granted behind, occupying the server until it expired on its own. It is revoked. mongodb, neo4j: Close latched whatever the underlying close returned, so a timeout, which is exactly what the bounded context produces, left the storage permanently reporting an error it could never clear. Success is latched, failure is reported and a later Close tries again. neo4j's Close is bounded too, it could hang forever while the cleanup path beside it could not. aerospike, cassandra: Close is idempotent, the last two drivers without a guard. gocql panics on a double session close.
…o benchmarks cassandra: Close returned nothing, so *Storage did not satisfy the storage.Storage interface its own README documents, and could not be used generically or run against the TCK. It returns an error, which existing callers keep discarding unchanged. ristretto: the Close test closed the shared store that the benchmarks below it still use, so go test -bench=. failed. It closes a store of its own, and covers the second Close while it is there. surrealdb: Close is bounded by the same closeTimeout the file already uses for its constructor cleanup, rather than running unbounded. pebble: when the decode and the closer both fail, both errors are reported. The close error used to mask the one that says the entry is corrupt. leveldb: an envelope carrying this driver's version but no value did not come from Set, which never stores an empty one, so it is reported as corrupt rather than read back as an ordinary miss. arangodb: the closed flag is atomic instead of mutex-guarded, so a call in flight, which is a full network round trip, no longer holds a lock that Close and later calls queue behind. badger: documents that Close waits for an in-flight value log sweep, which Badger gives no way to interrupt.
… unify the Close contract valkey, rueidis were skipped by this series entirely. Their Set built the expiration with Ex, which truncates to whole seconds, so a sub-second expiration became EX 0 and the server rejected the command; it uses PX, which carries the expiration exactly. Their constructors leaked the client when the ping or flush failed, and Close was not idempotent. The Close contract was inconsistent across the drivers: most latched whatever the first call returned, mongodb and neo4j reported failures and allowed a retry. Generic cleanup code could not rely on either. Every driver now has one contract: once the close has succeeded further calls do nothing, and a close that fails is reported so the caller can try again. Stopping the collector still happens exactly once, so a retried Close cannot close the done channel twice. cassandra: removes a stale comment claiming Close is not safe to call concurrently, directly above the new one saying it is safe to call twice.
…ndling valkey, rueidis: CacheTTL lived in a package-level variable that every New overwrote, so two storages in one process fought over it and the write raced every concurrent Get reading it. It is a field on the storage. aerospike: the context methods discarded the context, the last driver still doing so. They reject a context that is already cancelled or past its deadline before touching the storage, and the README no longer calls them dummies.
…racy deletes nats: Set reported the ErrKeyNotFound that sent it down the Create path even when the Create succeeded, because the inner err shadowed the outer one. The initialization error is also read under the same lock as the handle it describes, rather than racing the connection handler that writes it. bbolt: New always opened a write transaction to create the bucket, so Config.ReadOnly panicked at construction every time. A read-only database checks that the bucket is there instead. leveldb: Get deleted an entry it found expired, which LevelDB gives no way to do conditionally, so a value a concurrent Set had just written could be dropped. Get reports the miss and leaves reclamation to the collector.
|
Updated numbers, superseding the ~3.57x I quoted before:
The harness re-ran both and reproduced 2/2, so this is signal, not noise. I am not "fixing" this, because the fix would be to reintroduce the bug. Ristretto's There is no cheaper way to get read-after-write out of an asynchronous cache short of maintaining a shadow map of pending writes in the driver, which trades a measured cost for real complexity, extra locking, and unbounded memory growth. That seems clearly worse. So this is a semantics-vs-throughput call, and it belongs to the maintainers rather than to me:
Everything else on the branch is green: all 28 lint jobs, the test matrix, and the other benchmark modules — including valkey, memcache, postgres and arangodb, which were red last run on Docker registry rate limiting and recovered on their own. Generated by Claude Code |
This comment has been minimized.
This comment has been minimized.
…e failures nats: every operation attempted before the bucket exists wrapped s.err with %w, which renders as "kv not initialized: %!w(<nil>)" whenever nothing has failed yet — the normal state under RetryOnFailedConnect with the server still down, or after a short WaitForConnection. The message is now a sentinel that wraps the initialization failure only when there is one, and the dial error New tolerates under RetryOnFailedConnect is recorded rather than dropped, so the message has something to say. leveldb: errUnknownEnvelope and errCorruptEnvelope surface from the exported Get but were unexported, so a caller meeting an entry from a newer driver could not match them and fall back. Exported alongside ErrReadOnly and documented. Also re-syncs the pebble, redis and rueidis README Config listings with their config.go, which the comment trim in 3c08cda left behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
This comment has been minimized.
This comment has been minimized.
…kipped Reset arangodb: Delete returned ArangoDB's 1202 for a key that was not there, so it was the one method that failed on a miss — Get in this same PR already treats 1202 as a miss. It is now a no-op, as the interface implies and as every other driver behaves. redis: Reset combined with SkipConnectionCheck is still skipped, since flushing is the network call that option exists to avoid, but it is logged instead of dropped in silence, so a storage starting on top of keys the caller asked to have cleared says so. Also brings the README Config listings back in line with config.go for the eight drivers whose fields or docs this PR changed: aerospike and cassandra still advertised Config.Expiration as applied, ristretto was missing SkipWaitForWrite, nats was missing Reset, aerospike was missing InitialConnectionTimeout. valkey's config.go keeps the README's valkey wording rather than the redis terms it had inherited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
Two more review rounds, three fixes — and the four red benchmark checks explainedPushed in Fixed
Assessed, deliberately not changed
I also re-synced the README The four red benchmark checks on
|
| Check | Log |
|---|---|
compare (nats) |
create container: toomanyrequests: Rate exceeded |
compare (redis) |
No such image: public.ecr.aws/docker/library/redis:7 — the pull that lost the race |
In both, the other two benchmarks in the same job produced numbers seconds later. Nothing to fix in this PR; the sustained answer is a registry mirror or authenticated pulls at the org level.
compare (ristretto) is the synchronous-Set decision, still yours and still unanswered.
compare (neo4j) reported 8.90x on Benchmark_Neo4jStore_Set (452µs → 4.02ms), and it is noise. Worth stating plainly because 8.9x with a reproduced retest looks alarming. git diff 8a52cc8..2790c3f -- neo4j/ is empty — that commit touched nats, leveldb and READMEs only — and compare (neo4j) passed on 8a52cc8. Identical code, opposite verdict, on a runner that was simultaneously losing container pulls to rate limiting. The harness's retest re-measures on the same machine minutes later, so sustained host contention reproduces rather than washing out. Neo4j Set still issues the one MERGE it always did; the only change to that path in the whole PR is rounding the expiry up to whole seconds, which is arithmetic.
Verification
gofmt clean; go vet and golangci-lint run --tests=false report 0 issues in every changed module; go test -race -count=1 passes in the container-free modules.
Generated by Claude Code
This comment has been minimized.
This comment has been minimized.
|
Following up on It recurred on
The neo4j source is byte-identical across both of those heads and the one before them, which is checkable without reading the diff —
For completeness, the only change this PR makes anywhere near I'm not treating it as actionable and I'm not going to keep posting each time it fires. If it keeps flapping it's the neo4j benchmark job that wants attention, not this diff — most plausibly the container-backed drivers being measured on shared runners, where a single slow container skews a millisecond-scale query. Happy to look at stabilising it in a separate PR if that's useful. Generated by Claude Code |
Each explanation this PR introduced now says its point in one line. 338 blocks of two to five lines become 338 single lines, across 47 files. Left as they are: the Config struct listings and the READMEs, where the godoc convention of a blank line and a trailing "Optional. Default is ..." is what the surrounding fields already do, and the handful of comments this PR only moved rather than wrote. No code changes: the diff is comment lines only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
|
@codex review |
This comment has been minimized.
This comment has been minimized.
|
One follow-up on On
The dependency bumps the So the same code has now been reported as pass, 8.90x, 4.10x and 9.52x, and this round the flag landed on a function the diff never touches. The measurement is what is varying — most plausibly container-backed drivers being timed on shared runners against a baseline published from
Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40a5d8f3be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The bookkeeping set is named SetName + "_fiber_schema". Aerospike caps a set name at 63 bytes, so a SetName over 50 bytes -- valid on its own, and working before the bookkeeping moved to its own set -- produced a derived name the server rejects, and New panicked. The derived name is now truncated to fit, carrying a digest of the full SetName so two long names cannot converge on one bookkeeping set. It still ends in the reserved suffix, so it cannot collide with a caller's set either. Reported by Codex review on #3126. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46f4a747b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
goleveldb's DB.Close tears the database down before it returns: the teardown runs unconditionally once its closed flag is set, and the error it returns comes from a prior compaction failure or the underlying storage closer. Leaving s.closed false meant the retry the doc comment invited could only ever return leveldb: closed. The state is now latched either way, so the failure is reported once and a later Close is a no-op. The doc comment says so rather than promising a retry that cannot succeed. Reported by Codex review on #3126. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
|
✅ No significant benchmark change. e1354ff vs main@be73c3e · 9/9 results compared · retest: 2 reported re-checked · noise-aware thresholds · full results |
Keeps the ones giving a reason a reader cannot infer -- why the digest is carried, why the closed state is recorded on failure -- and removes those that just narrate the next line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
❗ 1 benchmark slower (3.32x)
e1354ff vs main@be73c3e · 9/12 results compared · 3 new, 0 gone · retest: 1/1 regressions reproduced · noise-aware thresholds · full results · |
Removes 78 comments across the branch that narrated the line beneath them: closeOwned release notes repeated in five drivers, the "stopping the collector happens once" pair that sits over a stopOnce already saying so, assertion narration in tests, and doc comments on unexported identifiers whose names already read as the sentence. Kept every comment carrying a reason a reader cannot infer -- engine behaviour, contract references, and the bugs a line exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
redis: FLUSHDB is a keyless command, and go-redis sends those to a single shard, so Reset cleared one master and reported success -- the same defect Keys was fixed for. Both, and New's reset, now go through one forEachNode helper. arangodb: IsNotFoundGeneral matches any 404, so a dropped collection read as a cache miss forever. Narrowed to 1202, which is what the comment already claimed. pebble, leveldb: the collector decoded each entry's whole payload to read one timestamp, base64 and all, once per scanned key and again per candidate. Both now decode the expiry alone; leveldb shares its envelope classification between the two decoders rather than duplicating it. The resume path also no longer seeks to the first key before seeking past the cursor. memory: one expiry predicate instead of two, maps.Clone for the snapshot, and Delete's pre-lock closed check dropped -- it guarded nothing. Tests: the expiry polling loops asserted the deadline after breaking out, so a key that vanished late still passed; twelve sites now assert before the break, matching the two that already did. Also: dead AlwaysPipelining block beside the flag that supersedes it, an unreachable clamp and a vestigial alias in aerospike, and four manual clamps replaced with min. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
|
✅ No significant benchmark change. e1354ff vs main@be73c3e · 9/12 results compared · 3 new, 6 gone · retest: 1 reported re-checked · full results |
Collapsing expired() into expiredAt(time.Now().UnixNano()) in the previous commit made the clock read an argument, so it evaluated on every Get rather than only for entries carrying an expiry. Benchmark_Memory_Get went from 16 to 52 ns/op in CI; locally the tell was Get and Get_WithExpiration costing the same. expired() is back, guarding on the expiry before it reads the clock and delegating the comparison to expiredAt, so there is still one definition of what expired means. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1354ff742
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := s.db.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| s.closed = true |
There was a problem hiding this comment.
When Badger reports a value-log, manifest, or directory-sync error from DB.Close, it has still completed teardown and its internal closeOnce prevents that work from running again. Leaving s.closed false therefore makes the next Storage.Close call invoke a no-op DB.Close, receive nil, and falsely report that the failed cleanup was successfully retried; record the storage as closed regardless of the first result while still returning that result.
Useful? React with 👍 / 👎.
| err := testStore.Set(key, val, 900*time.Millisecond) | ||
| require.NoError(t, err) | ||
|
|
||
| deadline := time.Now().Add(900 * time.Millisecond) |
There was a problem hiding this comment.
When the synchronous Set takes nonzero time and the 900 ms TTL has little rounding slack before the next whole-second boundary, the stored expiry is based on the timestamp taken inside Set, but this test starts its deadline only after Set returns. The loop can consequently continue past the legitimate stored expiry and report an early expiration; this occurred while running the Pebble suite. Capture the deadline before calling Set so the assertion covers only the requested TTL interval.
Useful? React with 👍 / 👎.
|
|
||
| // Schema info is stored with a special key | ||
| schemaKey, err := aerospike.NewKey(s.namespace, s.setName, "_schema_info") | ||
| schemaKey, err := aerospike.NewKey(s.namespace, s.schemaSetName, schemaInfoKey) |
There was a problem hiding this comment.
When opening a database created by an earlier driver version without Reset, its schema record still lives at _schema_info in the configured user set, but this lookup now checks only the derived metadata set. It therefore treats every upgrade as a brand-new schema and writes the configured version and fresh timestamps, bypassing the existing-version comparison—for example, a persisted version newer than the configured one is silently replaced in GetSchemaInfo by the lower configured version. Probe and migrate the legacy record before creating new metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one — the probe it needs would undo a change this PR makes deliberately.
The legacy record lives at _schema_info in the caller's set. Moving the bookkeeping out of that set is the point of the change: _schema_info is now an ordinary key that callers may read, write and have cleared by Reset, which Test_AeroSpikeDB_SchemaInfoKey_Is_Not_Reserved pins by storing []byte("user value") there. Probing that location on every fresh open and treating what it finds as driver metadata reintroduces exactly the coupling the change removes — and the bins are only usually distinguishable, since nothing stops a caller writing a version bin.
The consequence you describe is real and already stated in the PR body, under the behaviour-change table:
The schema record is recreated once on upgrade. In exchange no key name is reserved and
Resetclears everything the caller stored.
On the version comparison specifically: this bookkeeping is informational. Nothing in the driver reads SchemaVersion to gate a data migration — createOrVerifySchema writes it, GetSchemaInfo reports it. So a persisted 5 being replaced by a configured 1 after upgrade changes what GetSchemaInfo returns until the next configured bump, and nothing else. That is a one-time cost on a record the driver created for itself, weighed against permanently reserving a key in the caller's keyspace.
Happy to revisit if someone is using GetSchemaInfo to drive their own migration logic across this upgrade — that would change the calculus, and it isn't something I can see from here.
Generated by Claude Code
Badger's DB.Close wraps its teardown in a sync.Once and returns the error that run produced. A second call skips the Once and returns nil, so leaving s.closed false meant a caller retrying after a value-log or manifest failure got nil and believed the cleanup had succeeded. The state is latched either way now, matching leveldb. Also fixes a flaky pebble test: it started its "must not expire early" window after Set returned, but the entry's deadline is measured inside the call, so the window outlasted the entry by however long Set took. Reported by Codex review on #3126. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyjTyY3uQKAcGom9SQM1tZ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8dcf6fe75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := s.conn.Close(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When bbolt reports a munmap, file-unlock, or file-close error, its close routine has already set the database's opened flag to false and torn down its resources. Returning here without setting s.closed makes the next Storage.Close call invoke an already-closed database, receive nil, and falsely report that the failed cleanup was successfully retried; preserve the first error but record the storage as closed regardless.
Useful? React with 👍 / 👎.
| if err := s.db.Close(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When an owned go-redis client returns an error while closing a connection or pool, it has already marked those pools closed and completed the rest of teardown. Leaving s.closed false causes later operations to bypass ErrClosed and reach the closed client, while another Close cannot perform a real retry and commonly returns the pool's already-closed error; latch the closed state while still returning the first teardown error.
Useful? React with 👍 / 👎.
bbolt, clickhouse, etcd, mssql, mysql, sqlite3 and redis returned early on a Close error and left their closed flag false, so a second Close retried a teardown that had already run and reported success. Each of these engines completes teardown regardless of the error it returns: bbolt clears db.opened first, database/sql sets db.closed first, etcd cancels the client context before closing the connection, and go-redis CASes its pool state. Record the flag and return the error instead. For redis the stale flag also let later operations reach a closed pool rather than reporting ErrClosed. mongodb, neo4j and surrealdb keep the retry behaviour: they close under a bounded context where a timeout is genuinely transient.
3 participants
A correctness sweep across the driver modules: 56 commits, 91 files, 25 modules, +5,174 / −835.
It started as a fix for a handful of leveldb/pebble/bbolt bugs and grew as the same classes of defect turned up in driver after driver. Nothing here is a refactor for its own sake — every change fixes a behaviour that is wrong against the
storage.Storagecontract or against the engine it wraps.Each module is its own Go module, so there is no shared helper package to fix any of this in one place; the repeated patterns below are genuinely repeated per driver.
How to review
Reviewing commit by commit is not worth it — the history is 56 incremental passes over the same tree. Review by theme instead:
Every fix has a regression test where the driver can be tested without a container.
Cross-cutting fixes
Interface conformance
*Storagein several drivers did not actually satisfystorage.Storage(leveldb was missingDeleteWithContextentirely). The*WithContextmethods now honour an already-cancelled context and returncontext.Canceled, as the interface documents and the TCK conformance suite asserts. Drivers that faked context support by ignoring the argument (surrealdb, etcd, aerospike, cassandra, pebble) now thread it through to the engine.Miss vs. failure in
GetGetis documented to returnnil, nilfor a missing key and an error for everything else. Several drivers collapsed the two, reporting connection failures, decode failures and permission errors as cache misses — which reads as silent data loss to the caller. Sentinel comparisons were also switched toerrors.Is.Sub-second expirations
Roughly fifteen drivers computed the deadline with second granularity by truncation, so any
expunder a second — and the sub-second remainder of any other — expired immediately or was dropped early. Deadlines are now rounded up, and drivers that can express milliseconds natively (valkey, rueidis viaPX) use them.GCIntervaland Cassandra's default TTL were truncated the same way in config handling.A negative
expnow uniformly means "no expiration" rather than "already expired" — and in redis specifically, a negative duration was being read as go-redis'sKeepTTLsentinel, which silently carried the previous expiration over instead of clearing it.Connection ownership
Two symmetrical leaks:
NewFromConnection-style constructors took a caller-supplied client and closed it inClose, tearing down a client the application was still using elsewhere. Ownership is now tracked;Closeonly closes what the driver opened.ClosecontractCloseis now uniformly idempotent (a second call returns the first call's result rather than panicking on a re-closed channel or blocking forever on a done channel), non-hanging (GC goroutines are stopped and waited for before the handle is closed, so the collector cannot write to a database that is closing), and retryable where the underlying close can fail. Drivers with a background collector expose a stopped-channel handshake rather than a bareclose(done).Cleanup paths that could block indefinitely (mongodb
Disconnect, neo4j, surrealdb) are now bounded by a timeout.Background collectors
The expiry collectors were rewritten in leveldb, pebble, nats, arangodb and mongodb to be race-free and bounded: they scan a capped number of keys per tick, buffer deletions in bounded batches, resume from a persisted cursor on the next tick (so a database larger than one scan window is not starved at the head), and re-check the entry under the lock before deleting so a concurrent writer's fresh value is not destroyed. Where the engine supports it, deletion is conditional on the revision that was read (nats
LastRevision).Per-driver highlights
arangodb — the collector wrote to a nil map, crashing the process. Its filter was
doc.exp <= @exp, which matched every key stored without an expiration (exp == 0) and deleted the entire collection on the first tick. Check-then-act writes replaced with a single AQLUPSERT.pebble —
Resetonly calledFlush(), so it never removed a key.configDefaultrecursed into itself infinitely when called with no arguments.log.Fatalon an internal error exited the host process. AddedErrClosedguards, an expiry collector, and bounded batching inReset.leveldb — the whole
Configwas constructed and then ignored, soReadOnly, the leveldb tuning options andGCIntervaldid nothing. Values are now written in a versioned envelope (_fiber_storage_v) with a fallback that still reads entries written by earlier versions, so a value that is itself a JSON object is no longer read back as nil. MissingDeleteWithContextadded.bbolt —
Getreturned a slice pointing into the memory-mapped file, valid only for the life of the transaction; the caller was reading freed memory.Resetmutated the bucket while iterating it. A missing bucket was dereferenced instead of reported (ErrBucketNotFound).redis —
Keys()usedSCANon aUniversalClient, which in cluster mode returns the keys of one node only; it now fans out withForEachMaster. PlusSkipConnectionCheck(see below).ristretto —
Setreturned before ristretto's asynchronous write buffer had drained, so aGetimmediately after aSetcould miss. Values are copied in and out of the cache.Resettook the wrong lock.memory —
Setchecked the closed flag and then took the lock, so aCloselanding in between let it store an expiring entry after the collector had stopped, with nothing left to reclaim it. The flag is now re-checked under the lock.aerospike —
Resetdeleted the driver's own schema record, losing bookkeeping. That bookkeeping now lives in its own Aerospike set, so the scanResetruns never sees it (see From review below).mongodb — pooled items returned to the
sync.Poolkept theirObjectID, so a stale_idleaked into the nextSet. AddedErrClosed, a default operation timeout, and closed-state guards.nats —
Keys()fanned out one request per key; it now uses a singleWatch. Delete-on-read is conditional on the revision that was read, so it cannot discard a value written between the read and the delete.surrealdb — the expiry query deleted on
exp <= nowincludingexp == 0, silently wiping non-expiring keys; real context support added.cassandra / scylladb / mysql / mssql / postgres / sqlite3 / clickhouse — delete-on-read races replaced with atomic statements or server-side TTL, TTL clamping, and the shared
Get/Closefixes above.etcd — a failed
Putleft its lease attached to nothing, occupying the server until it expired; the lease is now revoked.From review
pebble —
Closeonly recorded the close whendb.Close()succeeded. Pebble marks the database closed either way and panics if it is closed again, so the retry the doc comment invited would panic. pebble, leveldb — a GC sweep running across aResetwrote its pre-Resetcursor back over thenilResethad stored, sending the next sweep past everything written since; sweeps now carry an epoch.arangodb — the collection was interpolated into the AQL text rather than bound. A name the server accepts is not necessarily a bare AQL identifier, so a collection called
fiber-storageparsed as a subtraction and made everySetfail. Now bound as@@collection.aerospike — the schema record shared the caller's set, so storing under
_schema_infowrote onto the bookkeeping record, whichResetthen exempted; the user's value survived a reset. Bookkeeping moved to its own set, so_schema_infois an ordinary key again, and the_fiber_schemasuffix is reserved so two storages cannot collide.scylladb —
KeyspaceandTablewere interpolated into every CQL statement with no validation beyond a non-empty keyspace. CQL cannot bind identifiers, so an application deriving either name from untrusted input had an injection point. Now validated as bare CQL identifiers, matching the cassandra driver.Behaviour changes worth an explicit decision
These are intentional and visible to users. Flagging them rather than burying them in the diff:
aerospike.Config.Expirationandcassandra.Config.Expirationare deprecated and no longer appliedexp == 0as "no expiration". A config-level default silently overrode that, so callers could not store a non-expiring key.cassandra.ErrNotFoundandErrKeyExpireddeprecatedGetnow reports both asnil, nilper the interface contract, so neither is returned any more.pebble.ConfigDefault.WriteOptionsis nownil(synchronous) instead of&pebble.WriteOptions{}configDefaultmadeConfigDefaultreachable for the first time. AnyConfigthat set onlyPathalready gotnil; leaving the default async would have made the no-arg constructor less durable than every partial config in the wild. Set&pebble.WriteOptions{}explicitly to opt back into buffered writes.ristretto.Setis now synchronous by defaultGet-after-Setto work.Config.SkipWaitForWriterestores the old fire-and-forget behaviour for throughput-sensitive callers. This costs ~3x onSet(1757 vs 581 ns/op) — deliberate, and the one call that needs a maintainer decision.NewFromConnection(redis and peers) no longer closes the caller's client inCloseReadOnlyin bbolt and leveldb is now enforcedErrReadOnlyinstead of appearing to succeed.Closenow closes the storage, not just the connection (redis, rueidis, valkey, memory)ErrClosed. Closing a storage built from a borrowed redis client previously left it fully usable, and amemorySetafterClosestored an entry the stopped collector could never reclaim.Getare now client-side cached by defaultConfigDefault.CacheTTListime.Minute, butconfigDefaultguarded on!= time.Second, so anyone who leftCacheTTLunset got0— andDoCache(..., 0)disables caching. The documented default was unreachable. It now applies, and the TTL is per-storage rather than a package-level variable every instance overwrote. This is whyBenchmark_Rueidis_Getreports 23675 → 87 ns/op.SetName + "_fiber_schema"), and that suffix is now reservedResetclears everything the caller stored. ASetNameending in the suffix is refused, since it would name another storage's bookkeeping set as its own data set.KeyspaceorTablethat is not a bare CQL identifierNew exported errors:
bbolt.ErrBucketNotFound,bbolt.ErrReadOnly,leveldb.ErrReadOnly,pebble.ErrClosed,mongodb.ErrClosed,redis.ErrClosed,rueidis.ErrClosed,valkey.ErrClosed,memory.ErrClosed.New configuration
SkipConnectionCheckNewdo no network call at all, so an unreachable Redis reports errors on first use instead of panicking at construction. This is the config-option approach agreed in the issue thread, rather than changingNew's signature, which was ruled out there as a breaking change.SkipWaitForWriteSet.DisableAlwaysPipeliningGCIntervalCheck status
compare (ristretto)— needs a maintainer decision, and its green tick is not trustworthy. It is the only check red because of a choice made in this PR (the synchronousSetrow above). On some runs it reports success anyway: when the job finds no stored baseline it logsbaseline=noneand skips the comparison and the "Fail On Regression" step entirely. The measured 3.57x regression exceeds the job'sALERT_THRESHOLD: 300%, so it fails whenever a baseline is present. Please treat this check as unresolved regardless of its current colour.Other red benchmark jobs are Docker registry rate limiting, not this branch. The benchmark jobs pull their images in parallel and hit
toomanyrequests; depending on where the pull fails that surfaces either astoomanyrequests: Rate exceededor asNo such image: <image>when the container is then created. It has hitvalkey,rueidis,postgres,memcacheandcassandra, and it reproduces onmain. These are transient — in the memcache and cassandra runs, the other two benchmarks in the same job produced numbers seconds after the first failed to get a container.This PR removed one contributor to that pressure:
benchmark-setup.shwas starting aredis:7container forrueidis | valkeythat nothing connected to (both build their config from testcontainers; the only fixed-address tests aret.Skipped cluster ones).rueidisandvalkeyboth pass since. A durable fix — a registry mirror or authenticated pulls — is infrastructure work outside this PR.Verification
gofmt -l .clean.go vet ./...andgolangci-lint run --tests=false --timeout 5m ./...clean (0 issues) in every module.go test -race -count=1 ./...passing in the modules that need no container: memory, badger, bbolt, leveldb, pebble, ristretto, sqlite3, mockstorage.Closes #1924.
Summary by CodeRabbit
New Features
Bug Fixes
Closecalls are now safe and reliable.Documentation