Skip to content

fix(store): preserve final sync during shutdown - #1397

Draft
corylanou wants to merge 2 commits into
mainfrom
fix/store-close-monitor-race
Draft

fix(store): preserve final sync during shutdown#1397
corylanou wants to merge 2 commits into
mainfrom
fix/store-close-monitor-race

Conversation

@corylanou

@corylanou corylanou commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Description

Preserves the final database flush when Store-owned monitors are slow or ignore cancellation, while eliminating the snapshot encoder race with database handle cleanup.

  • cancels Store monitors immediately, then lets each database complete its final WAL-to-LTX sync, replica sync, and handle cleanup before waiting for those monitors
  • makes the post-flush monitor wait interruptible by Close(ctx) or the CLI second-signal channel
  • registers snapshot readers while holding the DB execution semaphore
  • closes and joins every active snapshot encoder before final sync and before DB.Close() clears db.f
  • closes and joins the encoder when a snapshot backend returns early, whether it returns success or failure

Motivation and Context

A full race sweep while validating PR #1396 caught DB.Close() clearing database fields while monitorCompactionLevel() was still taking a snapshot. The initial fix moved s.wg.Wait() before DB.Close(), but cold review found that ordering introduced a durability regression: file fsync and SFTP operations can ignore cancellation, so shutdown could block before the final WAL-to-LTX and replica sync. A second signal could not interrupt that wait.

The review also found that waiting for the Store monitor did not completely eliminate the original race. snapshotReader launches an encoder goroutine that was not joined when a backend returned early; the monitor could finish while that encoder still used db.f.

Both major findings and the test-coverage finding were confirmed. The revised design does not make final-flush correctness depend on a backend monitor stopping:

  1. Store cancellation prevents normal new work.
  2. DB.Close() acquires the DB execution semaphore, closes all registered snapshot pipes, and joins their encoders.
  3. The database performs its final local and replica sync and clears its handles.
  4. Store waits for any remaining backend monitor work only after the durability work is complete; caller cancellation or a second signal can interrupt that wait.

This deliberately avoids placing an unbounded backend wait in front of the final flush. Discovered while validating #1396.

How Has This Been Tested?

The regressions fail on the previous PR head 4364eab:

  • an early-success snapshot backend lets Snapshot return while its encoder still holds the checkpoint lock
  • a non-cancelable snapshot upload prevents Store.Close() from reaching the final sync

The replacement Store regression runs through the real snapshot monitor and snapshotReader, leaves the backend blocked after cancellation, writes pending WAL data, and verifies that shutdown still advances both the local and replica TXIDs before a second signal interrupts the remaining monitor wait.

go test -race -count=10 -run '^(TestDB_SnapshotJoinsEncoderOnEarlyReplicaReturn|TestStore_CloseFlushesBeforeInterruptibleMonitorWait|TestStore_WaitForMonitorsHonorsContext)$' .
go test -race -count=10 -timeout 45m ./...
pre-commit run --all-files
go build -o bin/litestream ./cmd/litestream
go build -o bin/litestream-test ./cmd/litestream-test
go test -tags 'integration,soak' -run '^TestLTXBehavior$' -short -v -timeout 10m ./tests/integration/

The low-, high-, and burst-volume behavioral profiles all passed shutdown, restoration, and integrity validation.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (would cause existing functionality to not work as expected)

Checklist

  • My code follows the code style of this project (go fmt, go vet)
  • I have tested my changes (go test ./...)
  • I have updated the documentation accordingly (not needed; shutdown contract is unchanged)

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

PR Build Metrics

All clear — no issues detected

Check Status Summary
Binary size 37.09 MB (+8.0 KB / +0.02%)
Dependencies No changes
Vulnerabilities None detected
Go toolchain 1.25.12 (latest)
Module graph 1240 edges (0)

Binary Size

Size Change
Base (ad0c7b7) 37.09 MB
PR (3ead5ba) 37.09 MB +8.0 KB (+0.02%)

Dependency Changes

No dependency changes.

govulncheck Output

=== Symbol Results ===

No vulnerabilities found.

Your code is affected by 0 vulnerabilities.
This scan also found 1 vulnerability in packages you import and 3
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.

Build Info

Metric Value
Build time 42s
Go version go1.25.12
Commit 3ead5ba

History (4 previous)

Commit Updated Status Summary
6aceff1 2026-07-27 18:43 UTC 37.00 MB (+8.0 KB / +0.02%)
798f4c6 2026-07-27 17:43 UTC 37.00 MB (+8.0 KB / +0.02%)
3b2b3d1 2026-07-27 13:13 UTC 37.00 MB (+12.0 KB / +0.03%)
67f2318 2026-07-25 09:03 UTC 36.98 MB (0.0 KB / 0.00%)

🤖 Updated on each push.

@github-actions github-actions Bot added the metrics: vulns-found govulncheck found vulnerabilities label Jul 25, 2026
@corylanou
corylanou marked this pull request as draft July 25, 2026 08:26
@corylanou corylanou changed the title fix(store): stop monitors before closing databases fix(store): preserve final sync during shutdown Jul 25, 2026
@corylanou
corylanou force-pushed the fix/store-close-monitor-race branch 2 times, most recently from 687e053 to 81870c5 Compare July 27, 2026 17:41
Cancel and join Store background monitors before closing managed databases. This prevents compaction and snapshot work from reading DB fields while DB.Close clears them.
Cancel Store monitors before database closure, but move their potentially unbounded wait after each database has completed its final WAL and replica sync. Let the caller context or second signal interrupt that post-flush wait.

Track active snapshot readers under the DB executor semaphore and close and join their encoder goroutines before clearing database handles. This removes the remaining snapshotReader file-handle race even when a backend returns early or ignores cancellation.
@corylanou
corylanou force-pushed the fix/store-close-monitor-race branch from 81870c5 to c4a2240 Compare July 27, 2026 18:41
@github-actions github-actions Bot removed the metrics: vulns-found govulncheck found vulnerabilities label Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant