Fix the C1File close-vs-write data race - #1086
Conversation
Closing a C1File while a write is in flight raced on the database handle: closeRawDB nil'd c.db while validateDb read it on another goroutine, so a write could slip past the open-check onto a handle being torn down. TestC1ZConcurrentClose reproduces this on main today — it fails there and passes here. Closed-ness is now an atomic flag rather than a nil'd pointer: closeRawDB CAS-flips dbClosed before closing rawDb, validateDb loads it, and the pointers stop being written outside construction. The rawDBOpen predicate replaces the bare rawDb != nil checks in Close, finalize, closeWithoutSave, and copyIsolateSync, which had quietly changed meaning once a closed store could still hold a non-nil handle — finalize consulting the stale check could conclude the handle was open and delete the database file out from under a save. Extracted from the store-teardown branch so the live bug fix is not queued behind that review; the four files here are byte-identical to their versions there, and that branch rebases over this cleanly. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (c *C1File) rawDBOpen() bool { | ||
| return c.rawDb != nil && !c.dbClosed.Load() |
There was a problem hiding this comment.
🟡 Suggestion: one rawDb != nil open-check was not converted — cloneCopy at pkg/dotc1z/clone_sync.go:241 still does if c.rawDb == nil { return ErrDbNotOpen }, and its doc comment on line 236 explicitly calls it "defensive against a future caller" that does not reject a closed handle. Since a closed C1File now keeps a non-nil rawDb, that guard no longer detects closed-ness — the exact stale-meaning problem this PR fixes in Close, finalize, closeWithoutSave, and CopyIsolateSync. It's masked today (SnapshotTo calls validateDb, CloneSync reaches getSync/LatestSyncID which do), so this is consistency rather than a live bug: switch it to c.rawDBOpen() (or validateDb) and drop the now-false claim in the comment.
Separately, this doc says "Two idioms release the handle and both must read as closed here", but the package's most common idiom — f.rawDb.Close() with no nil-out (snapshot_test.go:182,210,249,264, copy_isolate_sync_test.go:63,99,151,172, clone_sync_test.go:177, bulkload_test.go:189) — still reads as open. The matching new comment in finalize ("recognize a released handle however it was released", line 739) overclaims for the same reason: a caller using that idiom before Close still checkpoints a closed handle and takes the cleanupDbDir branch that deletes the working database. Worth narrowing both comments to the two idioms actually covered (flag-flip, and close-plus-nil).
| // validateDb ensures that the database has been opened and not yet closed. | ||
| func (c *C1File) validateDb(ctx context.Context) error { | ||
| if c.db == nil { | ||
| if c.db == nil || c.dbClosed.Load() { |
There was a problem hiding this comment.
🟡 Suggestion: this removes the data race but not the check-then-use window. A writer can load dbClosed == false here, then closeRawDB CAS-flips the flag and calls rawDb.Close() before the writer acquires the pooled connection — database/sql then returns sql: database is closed (not ErrDbNotOpen) for that one iteration. TestC1ZConcurrentClose asserts require.ErrorIs(t, err, ErrDbNotOpen) on the terminating write (c1file_concurrent_test.go:140) and on the follow-up EndSync (line 147), so landing in that window fails the test rather than degrading gracefully. The window is narrow and pre-existed in the same shape (c.db was nil'd only after rawDb.Close() returned), and publishing the flag before the close narrows it further — but since this test is the PR's oracle, consider mapping the driver's closed error onto ErrDbNotOpen in the write path so the contract holds for any interleaving.
General PR Review: Fix the C1File close-vs-write data raceBlocking Issues: 0 | Suggestions: 4 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness; the new commit was additionally reviewed at suggestion level. All three code findings from the previous review are addressed: the guard in Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Review findings on the close-vs-write fix, verified and addressed: - validateDb passes, then closeRawDB flips dbClosed and closes the handle before the query acquires a connection: database/sql returns its unexported "sql: database is closed" sentinel instead of the ErrDbNotOpen the concurrent-close test pins. Add dbNotOpenOnClosed, which translates the driver's closed-handle errors to ErrDbNotOpen only when dbClosed is set, and apply it at the chunked-insert funnel (all record Puts) and the sync-run stamp (EndSync). - cloneCopy still guarded with a bare nil check, which cannot see closeRawDB's release now that the pointer stays set; switch it to rawDBOpen and fix the comment that called the old check defensive. - cloneCopy's initFile teardown wrote rawDb/db post-construction, contradicting the never-reassigned invariant; route it through closeRawDB instead. - Narrow the rawDBOpen and finalize comments: a direct rawDb.Close() that leaves the field set (a common test idiom) still reads as open, so those docs no longer claim every release is recognized. Declined: adding -race to PR CI. The nightly workflow already runs the dotc1z shard under the race detector, which covers the concurrent-close test; PR CI deliberately stays race-free for wall-clock (docs/TESTING.md). Co-authored-by: Cursor <cursoragent@cursor.com>
| func (c *C1File) closeRawDB(ctx context.Context) error { | ||
| _, span := tracer.Start(ctx, "C1File.closeRawDB") | ||
| var err error | ||
| defer func() { uotel.EndSpanWithError(span, err) }() | ||
| if c.rawDb == nil { | ||
| if c.rawDb == nil || !c.dbClosed.CompareAndSwap(false, true) { | ||
| return nil | ||
| } | ||
| // Copy the rawDb to a local variable to avoid race conditions. | ||
| rawDb := c.rawDb | ||
| c.rawDb = nil | ||
| err = rawDb.Close() | ||
| c.db = nil | ||
| err = c.rawDb.Close() | ||
| return err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (high confidence in the mechanism, residual/pre-existing): the doc says a racing caller "fails closed rather than entering a database/sql call that is about to be torn down", but the flag only fences callers that have not yet passed validateDb. sql.DB.Close() does not wait for the in-use connection, so a writer that already holds the single pooled conn keeps committing. In finalize, truncateWAL releases that conn and a hot-looping writer parked in db.conn() is handed it immediately, so the write commits WAL frames after the checkpoint while closeRawDB returns — then the os.Stat(walPath) check at line 785 sees a still-truncated WAL and saveC1z reads only the main db file, silently dropping rows the writer got nil for. This PR fixes the error contract, not this durability window. Consider fencing writes (RWMutex read-held across the tx / in-flight counter drained by closeRawDB) or, at minimum, correcting the claim here so the residual window is documented.
| if err == nil || !c.dbClosed.Load() { | ||
| return err | ||
| } | ||
| if errors.Is(err, sql.ErrConnDone) || strings.Contains(err.Error(), "sql: database is closed") { |
There was a problem hiding this comment.
🟡 Suggestion: this mapping hinges on database/sql's unexported errDBClosed text ("sql: database is closed"). If a Go release reworks that string, the branch stops matching and nothing fails loudly — the only coverage is TestC1ZConcurrentClose, which would just get rarer, harder-to-diagnose CI flakes on the require.ErrorIs(err, ErrDbNotOpen) assertions. A deterministic unit test that opens a *sql.DB, closes it, does a BeginTx, and asserts dbNotOpenOnClosed maps the result would pin the string at every Go bump.
| _, err = c.db.ExecContext(ctx, query, args...) | ||
| if err != nil { | ||
| return err | ||
| return c.dbNotOpenOnClosed(err) |
There was a problem hiding this comment.
🟡 Suggestion: endSyncRun is wrapped but its sibling durable write CheckpointSync (sync_runs.go:622, the sync-token stamp) is not, so a Close racing a checkpoint still surfaces the raw driver sentinel. The doc at c1file.go:1521 acknowledges the partial application, but sync-token persistence is the one write whose failure classification callers are most likely to grow a dependency on. Consider wrapping CheckpointSync's ExecContext too, or scoping the documented contract to "record Puts and EndSync" so nobody assumes it holds fleet-wide.
| return err | ||
| } | ||
| if err = initFile.rawDb.Close(); err != nil { | ||
| if err = initFile.closeRawDB(ctx); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion (unchanged code, adjacent): the defer at line 259 says it will "return an error if that fails", but cloneCopy has an unnamed error return, so err = errors.Join(err, ...) inside the closure writes to a dead local and the temp-dir cleanup error is silently dropped — including the Windows/ATTACH case the comment at line 292 is worried about. CopyIsolateSync gets this right with (err error) at copy_isolate_sync.go:96; the same named return here would make the defer effective. Same applies to the early return err at line 286, which currently bypasses nothing but would compose correctly once named.
1 participant
Closing a C1File while a write is in flight raced on the database handle: closeRawDB nil'd c.db while validateDb read it on another goroutine, so a write could slip past the open-check onto a handle being torn down. TestC1ZConcurrentClose reproduces this on main today — it fails there and passes here.
Closed-ness is now an atomic flag rather than a nil'd pointer: closeRawDB CAS-flips dbClosed before closing rawDb, validateDb loads it, and the pointers stop being written outside construction. The rawDBOpen predicate replaces the bare rawDb != nil checks in Close, finalize, closeWithoutSave, and copyIsolateSync, which had quietly changed meaning once a closed store could still hold a non-nil handle — finalize consulting the stale check could conclude the handle was open and delete the database file out from under a save.
Extracted from the store-teardown branch so the live bug fix is not queued behind that review; the four files here are byte-identical to their versions there, and that branch rebases over this cleanly.