storage: SQLite write path robust under duress — write gate, atomic commit sections, torture-suite-proven - #45
Open
ConstanzeTU wants to merge 4 commits into
Open
ConstanzeTU wants to merge 4 commits into
ConstanzeTU wants to merge 4 commits into
Conversation
entlein
reviewed
Aug 28, 2026
|
|
||
| ra := &softwarecomposition.ContainerProfile{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "cp-retry-b", Namespace: "ns1", | ||
| Labels: map[string]string{"kubescape.io/rogue-state": "rogue"}}, |
There was a problem hiding this comment.
this test should not hinge on the new rogue label , this is a generic problem
entlein
reviewed
Aug 28, 2026
| // saveObjectBusyRetries bounds the in-place retries of the metadata+rename | ||
| // commit step on SQLite write-lock contention. Total added latency is at most | ||
| // 50+100+200ms — well inside any API request budget. | ||
| const saveObjectBusyRetries = 3 |
There was a problem hiding this comment.
magic numbers? what happens after 3 retries? Come on...
ConstanzeTU
changed the title
…ention SQLITE_BUSY / SQLITE_LOCKED from the metadata write surfaced to API clients as a 500 (issue kubescape#365). Since the commit step became atomic — metadata and payload rename under one savepoint, rolled back together — a contention failure leaves the staged payload untouched and the step is safe to retry in place. Retry up to three times (50/100/200ms backoff, bounded far inside any request budget); every other error class, including an interrupt from a canceled request context, still fails fast and cleans up the staged file. Unit: TestSaveObject_RetriesBusyThenSucceeds pins both directions — contention retried to success, non-contention surfaced on the first attempt.
…t sections atomic - Bind the connection interrupt to the request context instead of the pool acquisition context. The acquisition context carried a 5s timeout, so any operation still running 5s after taking its connection was killed by its own storage layer (the 'sqlite: step: interrupted' / 'clear bindings: interrupted' class). Acquisition deadline and execution lifetime are now separate. - Serialize all SQLite write sections on a per-pool in-process write gate. This process is the only writer of the database file, so write-lock contention between its own writers becomes structurally impossible instead of retried. The busy timeout drops from 60s to a 5s backstop (the 60s value parked writers on the SQLite lock while holding pool connections, starving unrelated reads on pool acquisition) and the bounded busy retry is retained as a backstop for extra-process writers only, reacquiring the gate per attempt and never sleeping while holding it. Lock ordering is fixed as per-key lock -> gate; gate holders acquire no per-key locks. - Mask the interrupt across the gated commit section: the savepoint spans an irreversible filesystem rename (or removal, for delete), and an interrupt landing on the savepoint RELEASE rolled the SQL back while the file operation persisted, tearing payload against metadata. A context already dead on entry fails fast before the savepoint opens; errors surfacing after the request context died map to the context error. - Commit a part profile's time-series row inside the same savepoint as its metadata insert and payload rename (PreCommitProcessor). Previously the row was written after the object's commit; on failure the part existed without its time-series row, invisible to consolidation and expiry forever, while client retries failed on KeyExists. - Make delete atomic: metadata delete, time-series delete and payload removal run in one gated savepoint. Metadata-delete errors were previously logged and ignored while the payload was removed regardless, leaving list results that disagreed with reads. - Consolidation holds the consolidated key's write lock and the gate around its per-key transaction; part payload reads inside the gated transaction are lock-free, which keeps the lock ordering acyclic. - Sweep orphan staged payload files at storage construction. - Fix a data race in TestFileSystemStorageWatchReturnsDistinctWatchers: compare watcher identity instead of deep-walking live watcher internals.
DURESS.md is the systematic failure-mode x operation truth table for the file/SQLite backend: every cell states its contract (impossible / fail-fast-clean / retry-in-place / converge / never-starve) and names the test or fix backing it, plus the architecture decision record for the in-process write gate. duress_test.go executes the table against a real file-backed database: interrupt lifetime across slow operations, part/time-series row atomicity, atomic delete, orphan staged-file sweep, staging fault injection, client-cancellation storms, same-key write hammering, a sustained mixed workload (concurrent creates, updates, deletes, hot-key contention, continuous readers, part ingestion and consolidation) and consolidation racing API writers on the same key. After every scenario the suite asserts the invariants: payload and metadata agree for every key, no key is wedged, only contracted error classes surface, reads never starve, and no staged files or orphaned rows remain.
ConstanzeTU
force-pushed
the
fix/sqlite-busy-retry
branch
from
August 28, 2026 07:30
4d0dca5 to
fd86188
Compare
…s ids ignored, goroutine-safe test helper - writeGate.acquire checks the context before re-entry and acquisition: with a free slot both select cases are ready and the runtime may pick acquisition for an already-canceled caller, letting it enter a savepoint it can no longer complete cleanly. - PreCommitSQL treats a BLANK report-series-id like an absent one — a blank seriesID row would make a non-part profile visible to consolidation. - duressParts no longer fails the test from worker goroutines ((*testing.T).FailNow must run on the test goroutine); it returns errors, and the storm loop records them through its error channel. - The 8s mixed-workload storm honors -short. - DURESS.md: the Create sequence names the time-series row inside the single commit savepoint, matching row 8.
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.
Fixes kubescape#365 (the surfaced symptom; the lasting damage was already removed by kubescape#366).
Problem
SQLITE_BUSY/SQLITE_LOCKEDfrom the metadata write surfaces to API clients as a 500 when a ContainerProfile create/update races another writer past the busy timeout. Observed repeatedly in component-test CI (see the analysis on kubescape#365, including a negative result on raising the busy timeout: parked writer connections drained the pool and starved reads cluster-wide, so waiting longer is the wrong direction).Fix
Since kubescape#366 the commit step is atomic — metadata insert and payload rename run under one savepoint and roll back together, leaving the staged payload untouched on failure. That makes the step safe to retry in place: this PR retries it up to three times on BUSY/LOCKED only (50/100/200ms backoff, bounded far inside any request budget). Every other error class — including an interrupt from a canceled request context — still fails fast and removes the staged file.
Tests
TestSaveObject_RetriesBusyThenSucceedspins both directions: two contention failures retried in place with the third attempt committing and fully visible via GET, and a non-contention failure surfacing on the first attempt with no retry. Fails on the pre-fix code (no retry loop), passes post-fix.TestSaveObject_MetadataFailureLeavesPayloadUntouched,TestSaveObject_RenameFailureRollsBackMetadata) and the fullpkg/registry/filepackage pass unchanged.Note: this branch is based on the current upstream main (04cd064), so it can be re-targeted there unchanged.