fix: Lane 0 — close the write-path shard bypasses and immutability gaps - #399
Merged
Conversation
Traced the residual component-tests flakiness (kubescape/node-agent, 15-17/30 jobs failing even after #397's sharded single-writer fix) to deleteProcessedTimeSeries's DeleteContainerProfile call: it takes a raw pool connection outside the singleWriter shard system entirely, unlike SaveContainerProfile (already routed through guaranteedUpdateSingleWriter, priorityLow) and Create/GuaranteedUpdate's own commits (routed through the 8-shard system). A raw connection has no way to yield to -- or be yielded by -- a live shard commit, so a genuine collision for SQLite's single writer lock blocks the loser for up to the full busy-timeout (DefaultBusyTimeout, 60s in production) instead of the microsecond in-process channel wait every shard-routed write already enjoys. Confirmed locally (containerprofile_load_test.go, LOAD_CONSOLIDATORS=1): a pure 20-way concurrent write burst alone was flawless (9543/9543 ops, p99=115ms), but adding one concurrent ConsolidateTimeSeries pass collapsed throughput by >250x (36 ops/8s, 14 failed, multi-second "database is locked" stalls) -- the same fast, heterogeneous failure shape seen in CI, not the original catastrophic hang. Adds singleWriter.runOnShard(ctx, key, priority, fn): fn runs inside the key's own shard goroutine, holding the same pool connection and per-key lock a commit would, so it can't race a live commit for SQLite's lock. Routes deleteContainerProfileArbitrated (the actual delete call site) through it. Intentionally does NOT wrap the whole consolidateKeyTimeSeries unit this way -- an earlier attempt at that self-deadlocked, since consolidation's own updateProfile already calls SaveContainerProfile for the SAME key/shard, and that shard's one goroutine would then be waiting on itself. storageImpl.delete is a safe leaf operation for this: no lock of its own, no calls back into anything shard- or lock-routed. Result after the fix, same repro scenario: ~5000+ ops/8s, near-zero failures, p50 in microseconds, p99 ~5ms (down from 9.9s) -- confirmed reproducible across independent runs. Full pkg/registry/file test suite passes, including -race (only the pre-existing, confirmed-unrelated watch.go race remains, verified against unmodified main). Updates docs/features/single-writer-priority-queue.md's "Known gaps" section with this finding and its fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
matthyx
added
the
ai-assisted
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Summary:
|
… system Follow-up to the previous commit (routing ConsolidateTimeSeries's Delete through runOnShard). Re-tracing real node-agent CI logs after that fix landed found the SAME raw-connection bypass in AfterCreate's WriteTimeSeriesEntry call -- fired on essentially every TS ContainerProfile create, i.e. most of node-agent's write traffic. A synthetic local repro limited to ContainerProfile writes alone hadn't reproduced this, but real CI's aggregate write volume across every resource kind sharing this write path was enough to surface it as "sqlite: step: interrupted" (the caller's own request context expiring mid-statement on the raw connection) -- initially misread as an unrelated node-agent-side flakiness source, since the failing tests' own assertions don't mention storage at all. Two mistakes made while landing this, both caught before committing: 1. First attempt keyed runOnShard on the consolidated (non-TS-suffixed) base key. Many distinct TS-suffixed profiles for one container all share one base key, so that collapsed traffic that should spread across all 8 shards onto whichever few shards those few base keys hash to -- p50 went from microseconds to 15s in the load test. Fixed by keying on the TS-suffixed profile name instead (the same key its own metadata commit already uses), matching the commits' own distribution. 2. createSingleWriter (singlewriter.go) still pre-took a pool connection to hand AfterCreate via ctx -- dead weight once AfterCreate stopped reading it (no Processor.AfterCreate implementation needs a ctx-embedded connection anymore), held uselessly by every concurrent caller for the whole call while runOnShard also took its own connection, exhausting the pool under load and reproducing the same 15s-p50 regression. Removed. Result in containerprofile_load_test.go's LOAD_CONSOLIDATORS=1 scenario, with both this fix and the previous commit applied: p50=55us, p99~4ms, only 1/8s of ops over 5s -- an improvement on the previous commit's already-good numbers (p50 microseconds, p99~5ms, but occasional >5s stalls remained). Full pkg/registry/file test suite passes, including -race (only the pre-existing, confirmed-unrelated watch.go race remains). Updates docs/features/single-writer-priority-queue.md with this finding, its fix, and the two mistakes made getting there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
1 task
|
Summary:
|
…te-shard system" This reverts commit f3bf94e.
Contributor
Author
|
Update: reverted the second commit (WriteTimeSeriesEntry routing) from this PR. A deeper follow-up audit found real problems with both fixes on this branch that I want to be transparent about before anyone reviews further:
Recommend holding off on reviewing until that follow-up lands. Sorry for the churn — better to catch this now than after merge. |
|
Summary:
|
…t the shard deleteContainerProfileArbitrated routed StorageImpl.delete through runOnShard, so the delete's synchronous watchDispatcher.Deleted ran inside the shard goroutine while it held its pool connection and Lock(key). A "/" watcher that stops reading blocks send until its own ctx ends, freezing that shard -- and every job hashed to it, REST included -- for as long as the remote client chooses. Split delete into deleteLocked (SQLite + filesystem, no dispatch: a true leaf) and delete (deleteLocked + dispatch, byte-identical behaviour for Delete / DeleteWithConn / DeleteContainerProfile). The arbitrated path now routes deleteLocked and dispatches Deleted on the caller after runOnShard returns, the createSingleWriter / guaranteedUpdateSingleWriter convention. runOnShard's contract is stated precisely in its doc comment and in docs/features/single-writer-priority-queue.md: fn must be a leaf AND non-blocking, no watchDispatcher.* inside fn. TestDeleteArbitrated_StalledWatcherDoesNotFreezeShard fails at 0f75129 (the same-shard probe hits its 2s deadline) and passes after this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…ck (PRE-3) processTimeSeriesInTransaction called endFn(&err) inline after updateProfile. A panic raised inside updateProfile -- after the first series' Replace DELETE had taken SQLite's write lock inside the pass's deferred transaction -- escaped with the transaction open, and the connection went back to the pool with nobody left to end it. Every later writer then waited on that lock for the full busy timeout (SQLITE_BUSY). endFn is now deferred directly: sqlitex's endFn recovers the panic, rolls the transaction back and re-panics, so the panic still propagates (PRE-3 fixes the lock, not the crash) but the write lock is released. A second deferred closure preserves the error wrapping for both a failed updateProfile and a failed COMMIT. TestProcessTimeSeriesInTransaction_PanicLeavesNoOpenTransaction injects the panic from the second series' TS read (after the first series' DELETE) and asserts a write on another connection succeeds immediately. It fails at 9cb7ce6 with "database is locked" and passes after this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
… (finding T) updateProfile's gate 1 (no InstanceID annotation on the merged profile, so the observed save has no target) returned the merged TS keys as processed while persisting nothing. consolidateKeyTimeSeries then deleted those TS objects, losing the data they carried. INV-PROCESSED: a tsKey is returned only when the profile it was merged into was persisted by this pass (or, after finding X, reclaimed unmerged by the frozen gate). Gate 1 now returns nil, nil. TestUpdateProfile_MissingInstanceID_ProcessedIsNil builds its TS profile without an InstanceID annotation (with one, mergeContainerProfileTS would put the key on the profile and gate 1 would be unreachable). It fails at 9cb7ce6 (processed names the merged key) and passes after this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…inding U) mergeTimeSeriesData appended every row's suffix to the delete list before reading its object, so a row whose read failed with a transient (non-NotFound) error was still collapsed into its neighbour by consolidateContinuousTimeSeries and then deleted by Replace with its object never merged: data loss. When that row was the chain head, the terminal branch fired on the unmerged row and the profile was stamped Completed/Full without its data (U-b). The transient arm now neither appends the suffix nor hands the row to the consolidation: mergeTimeSeriesData returns kept (the input minus the transient rows, carrying the loop's HasData=false mutations) and processTimeSeries consolidates kept. The chain is consolidated around the gap, the row stays in the table with HasData=true and is retried next tick, when the chain collapses normally. The other three arms are unchanged. A series whose rows all took the transient arm returns before updateProfileStatus, which indexes newTimeSeries[0] unconditionally. The retry is logged at Warning (was Debug). Not appending alone is not enough: the chain would still be collapsed across the row, the two sides would fork on every later tick and the profile could only finish Completed/Partial through the expired path. Tests (fail at 9cb7ce6, pass after): U-1 non-head transient row survives and heals without a fork; U-2 head transient row does not complete unmerged; U-4 all-transient series runs no statement and does not panic. U-1 and U-2 also fail on the not-appending-only variant. Pins: U-3 (a collapsed HasData=false row is deleted -- fails if the !HasData arm drops its append), U-5 (a permanently failing read keeps its row and the key enumerated). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…finding X, E3)
"Once a profile is completed/full nothing should update it" was enforced at
the REST boundary (PrepareForUpdate resets the update) and at TS admission
(PreSave refuses the Create), but consolidation's own save was outside the
contract: SaveContainerProfile's tryUpdate ignored the persisted object it
was handed under Lock(key) and wrote whatever the pass had merged. A row from
another replica's series, a row that landed after the completing pass, or a
row admitted by PreSave's unlocked read was merged on a later tick into a
Completed/Full profile and dispatched as Modified.
Two layers, both on the persisted state:
X-A, the frozen gate at the top of updateProfile: when the payload the pass
read is Completed/Full, every listed row is reclaimed with the existing
(seriesID, tsSuffix IN listed)-scoped Replace delete and every HasData object
is scheduled for deletion; no TS object is read, nothing is merged, nothing
is saved. Counted (storage_consolidation_frozen_reclaimed_total{what}), one
Warning per key per tick. The gate reads the persisted state, never the copy
the loop is about to stamp, so the completing tick always passes.
X-B, the refusal inside the write: tryUpdate returns ProfileFrozenError when
the object GuaranteedUpdateWithConn read under Lock(key) is Completed/Full.
The pass's transaction rolls back and the next tick takes X-A. Counted
(storage_consolidation_frozen_refusals_total, expected zero).
A frozen tick sends no consolidated slug. The predicate is captured BEFORE
the pass: profile is passed by value, but its Annotations map is shared by
every copy, so evaluating it after the pass -- as the design text assumed --
would have made the completing tick look frozen and the guard "never send".
X-2 catches that.
E3, the divergence heal. saveObject renames the payload into place and the
row commits afterwards, so a process crash or a failed COMMIT between the two
leaves the payload Completed/Full at n+1 while the row (what LIST, WATCH and
PreSave read) still says Learning at n. Today that heals itself by re-merge
on the next tick (duplicating Spec); under X-A alone it would never heal --
the gate would reclaim every row while the row stayed Learning for the
container's life and PreSave kept admitting reports. consolidateKeyTimeSeries
now reads the metadata row in autocommit before the pass's transaction; on
payload-ahead it calls HealDivergence, which takes Lock(key), opens a
BEGIN IMMEDIATE (so an in-flight completer's COMMIT has landed before the
re-read), re-reads the row, and re-persists the payload as-is through
saveObject -- RV+1, no merge, no new data -- then dispatches the lost
Modified after COMMIT. Counted (storage_consolidation_divergence_total
{payload_ahead}); a failing heal fails the tick before X-A and is counted
by step (storage_consolidation_heal_failed_total{reason}). The inverse
shape, metadata-ahead (a lost rename after a power loss, B16), is counted
and warned only; today's merge-and-re-save proceeds and PreSave's revert
restores Completed from the row, RV colliding with the row's own n+1.
softwarecomposition.IsCompletedFull is the one predicate, shared by the
status setters and every gate above.
Tests. Fail on today's behaviour, pass after: X-1 (frozen base reclaims rows
and objects, nothing read or written, zero events, no slug; active, expired
and single-writer-off), X-3 (a base completed on another connection between
the pass's read and its write is refused, the transaction rolls back, the
next tick reclaims), E3-1 (payload-ahead healed without merge: row and
payload at n+2, Spec byte-equal, one Modified received after COMMIT, then
X-A reclaims in the same tick; also fails on X-A without E3), E3-2
(metadata-ahead counted, then today's re-save pinned: Learning handed to the
save, Completed restored by PreSave, Completion from the row, RV n+1
colliding). Pins: X-2 (the completing tick is not refused and sends one
slug), X-4 (Completed/Partial is not frozen), X-5 (an unlisted row survives
a frozen tick and is reclaimed next), E3-3 (a concurrent completer wins:
the heal waits at BEGIN IMMEDIATE with Lock(key) held, writes nothing),
E3-4 (a heal that fails after its own rename converges on the next heal),
and the predicate table test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…PC-TS-4) updateProfileStatus's two terminal branches (expired and active Completed/ Full) ran DeleteTimeSeriesContainerEntries -- DELETE FROM time_series WHERE kind/namespace/name, no seriesID, no tsSuffix -- from inside the consolidation transaction. That statement was safe only if every row under the key had already been merged, which is false twice over: updateProfile merges one series per iteration and the terminal branch breaks the loop, so the series later in map order were deleted unmerged; and the pass's list runs before its transaction, so a row committed in between was deleted too. Every row it removed had its TS object orphaned on disk (the objects were not in processed), and the rows it removed belong to a profile the same pass stamps Completed/Full, which under finding X must never be merged anyway. Delete both calls and add nothing: processTimeSeries already calls ReplaceTimeSeriesContainerEntries unconditionally afterwards, and on a terminal branch updateProfileStatus returns an empty newTimeSeries, so Replace's (seriesID, tsSuffix IN listed)-scoped DELETE removes exactly the rows the pass read for that one series and inserts nothing. Rows of an unreached series, and rows that landed after the list, survive to the next tick, where the frozen gate reclaims them WITH their objects, counted. updateProfileStatus loses its ctx, its storage receiver and its error return: a pure function that cannot execute SQL. The interface method that only it called is removed; the package-level function keeps its two REST callers. The FIXME at ReplaceTimeSeriesContainerEntries now states that its DELETE must stay list-scoped and is the only time_series delete on this path. seriesOrder is a test seam beside consolidateKey: nil keeps map order in production; TS4-B forces A before B so the terminal branch's unreached series is chosen, not inherited from map iteration. TS4-B (TestConsolidate_TerminalBranch_LeavesUnreachedSeriesIntact) fails with the whole-key delete present (B's row is gone after tick 1) and passes after: B's row and object survive tick 1, and tick 2 reclaims B unmerged with RV, Spec and annotations unchanged and zero events. TS4-C is a pin of the non-terminal branch against a future whole-key statement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…connection counters storage_single_writer_commit_total gains outcome="panic" for a commit the shard goroutine's recover converted to an error, and two counters back the connection-clean check on the shard's pool release: storage_single_writer_dirty_connection_total (rolled back before reuse) and storage_single_writer_dropped_connection_total (could not be rolled back; the pool shrinks by one). All three must stay zero. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…r (L0-B layer 1) runOnShard's closure ran bare on the shard goroutine, so a panic in it unwound the shard and, with no recover() anywhere in the package, exited the process. callGuarded recovers the closure's panic and returns it as an error (with the stack in the message), so the job fails to its own caller, the per-key lock and pool connection are released by commit()'s defers as on any error, and the shard takes the next job. TestSingleWriter_CustomPanic_CallerGetsErrorShardSurvives crashes the test binary on the previous commit (panic: injected custom panic) and passes here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…cs (L0-B layers 2+3) Two shipped hazards in the shard commit loop, one mechanism split across two frames because Go unwinds commit()'s defers before process() sees the panic: Layer 2 -- putChecked replaces commit()'s `defer s.pool.Put(conn)`. commit() calls release(&err) normally, not deferred (so a failed rollback's error is not lost), so a panic in writeMeta/renamePayload skipped it and returned a connection with an open SAVEPOINT to the pool; Pool.Put only checks stepped statements. putChecked runs on every exit, removes the temp payload on any non-committed exit, tests AutocommitEnabled and CheckReset, rolls back / resets a dirty connection before reuse (dirty_connection_total) and drops one it cannot clean instead of letting Put panic during the unwind (dropped_connection_total). It calls no recover() of its own; the defer is a closure so committed is read at execution time. Layer 3 -- process() recovers a panic that escapes commit() (sqlitex.Save's release panics on a failed ROLLBACK TO, outside callGuarded's reach), delivers errCommitPanic to the caller, counts outcome=panic, logs the stack, and takes the next job. Before this an unrecovered panic on the shard goroutine exited the process, and a caller waiting on that shard would never have received a result. Tests (each crashes the binary on the previous commit; run one per process for the fail-on-today evidence): - CustomPanicWithSteppedStatement_ConnectionIsReset: layer 2's CheckReset branch (Put would have panicked "active statement" during the unwind) - WriteMetadataPanic_ShardSurvivesConnectionRolledBack: the dangling savepoint is detected and rolled back, temp payload gone, key reusable - ReleasePathPanic_ShardSurvives: release's own panic, from commit()'s frame - SuccessfulCommit_DoesNotRemoveRenamedPayload: discriminates the argument-at-defer-time bug (verified to fail against `defer w.putChecked(...)`) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…ounters Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…nt row alone (E3-5, E3-6) HealDivergence deferred endFn(&err) on the closure's named error, so a COMMIT that failed after the payload rename surfaced as the raw SQLite error and healFailureReason fell through to "unknown" -- a label outside the documented set, for exactly the I/O-class failure the heal exists to repair. The deferred end now wraps any error that is not already a healFailure as reason "commit" (a ROLLBACK failure panics in endFn, so that is the only shape left), and the metrics doc names it. The same function treated a NotFound row as healable and would have re-persisted the payload -- resurrecting a key a crash between the row's delete and the payload's remove had already deleted. The caller's pre-check never asks for a heal on that shape; HealDivergence now returns nil on it too, mirroring the caller, instead of relying on it. E3-5 pins the failed-COMMIT label (an authorizer denies the COMMIT at prepare time on the pass's own connection; the row rolls back, the payload lands one version further ahead, the next heal converges) and E3-6 pins the absent-row no-op (row not resurrected, payload untouched, nothing dispatched, nothing counted). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…t-watchers test TestFileSystemStorageWatchReturnsDistinctWatchers asserted assert.NotEqual on two live *watcher values, which reflect.DeepEquals their fields while each watcher's shipIt goroutine is running -- a data race the detector reported in most runs (16 reports in 20 on the base commit). The test's intent is that Watch returns a new object each call; assert.NotSame checks exactly that, by pointer, and reads nothing the goroutine writes. Pre-existing; not introduced by Lane 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…ter heal test (E3-3) TestHealDivergence_ConcurrentCompleterWins slept a fixed 300ms and then probed Lock(key) with a 50ms timeout, assuming the heal goroutine had reached Lock(key) by then -- a starved runner could fail the probe spuriously. The probe is now a require.Eventually that polls until the lock is held (releasing immediately whenever the probe wins), and the elapsed-time assertion that only restated the sleep is dropped; the "heal must not return while the completer's transaction is open" check stays, on a bounded wait. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
matthyx
changed the title
|
Summary:
|
Contributor
Author
Real-CI validation: no measurable improvement over same-day baseline (n=3 each)Validated this branch (Lane 0, commit Failure counts (out of 31 component-tests jobs):
The two distributions overlap almost completely and are not distinguishable at this sample size — Lane 0 is not clearly better or worse than baseline on job-level pass/fail counts. Per-test breakdown: 8 of 31 tests fail in every single one of the 6 runs regardless of which storage image was used (Test_02_AllAlertsFromMaliciousApp, Test_06_KillProcessInTheMiddle, Test_11_EndpointTest, Test_14_RulePoliciesTest, Test_27_ApplicationProfileOpens, Test_28_UserDefinedNetworkNeighborhood, Test_32_UnexpectedProcessArguments, Test_36_MultiContainerPerContainerBinding), plus 3 more that fail in 5 of 6 runs (Test_17_ApCompletedToPartialUpdateTest, Test_30_IgnoreExcludeAndLearningDuration, Test_49_EphemeralContainerFullTreatment). This ~11-test core is the dominant failure source in both baseline and Lane 0 and looks environmental/harness-related (CI runner resource pressure, timing assumptions in the kind cluster), not specific to either storage write path. (Initially flagged Test_17 as possibly Lane-0-specific after baseline's first run missed it — resolved once baseline reached n=3: it shows up in 2 of 3 baseline runs too.) Storage-side signal: pulled the storage pod's own logs (not just node-agent's) for a 6-job sample from one Lane 0 run and one baseline run. Lane 0 shows a real but modest reduction in apiserver Net assessment: no clear regression, no clear improvement in CI outcomes as currently measured. The component-tests suite's aggregate pass/fail signal is dominated by failures unrelated to the storage write path, which limits how visible any storage-side fix can be at this level — a more targeted signal (e.g. direct measurement of Handler-timeout / lock-error volume, as sampled above, or the local Throwaway validation PRs (closed after data collection, branches kept): kubescape/node-agent#956, #957, #958, #959, #960, #961. |
This was referenced Sep 8, 2026
Closed
Closed
Contributor
Author
Step 1-L measurement: Tier B paired A/B of this branch vs its merge-baseMeasured with the new two-tier storage measurement harness ( Verdict line:
Reading. REST operations improve dramatically and significantly under contention: Get p99 −28%, Create p99 −57%, GuaranteedUpdate p95 −22%, pool-wait timeouts −77%, throughput +15%, all p < 0.001, no lock timeouts, no conflicts, no errors. Consolidation ticks get measurably slower: each processed TS row now costs a synchronous shard round-trip (pool take + This is a real, quantified instance of exactly the row-vs-payload-split overhead that the separate full-ACID redesign effort (in progress) is expected to eliminate. Recorded here so the cost is on the record alongside the correctness fixes; the Tier A work-budget diff for this branch (per-scenario statement/lock/take deltas) will be carried in the harness PR's description. |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
|
Summary:
|
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
1 participant
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.
Summary
Follow-up to #397 (sharded single-writer commits), which fixed a catastrophic write-concurrency regression in kubescape/node-agent's component-tests but left a residual, smaller flakiness (15-17/30 jobs failing, vs. a 0-1/30 historical baseline) — documented as an open gap in
docs/features/single-writer-priority-queue.md's "Known gaps" section.This PR grew from a single fix into "Lane 0" — a standalone-shippable bundle of every write-path bypass, invariant gap, and panic-containment issue found during a deep, multi-round audit of the shard/single-writer system (
.omc/plans/raw-write-bypass-elimination.md), independent of the larger Step 3 phase-split redesign that will follow in a separate PR. Every fix below has an acceptance test that fails against pre-fix code and passes after.What's in Lane 0
L0-C — the original fix.
ConsolidateTimeSeries's delete step took a raw pool connection outside the write-shard system entirely, so a genuine SQLite write-lock collision blocked the loser for up to the full busy-timeout (60s in production) instead of the microsecond in-process wait every shard-routed write gets. Fixed by routing it throughsingleWriter.runOnShard. A later audit found the routed callback still synchronously dispatched its watch event while holding the shard's lock/connection — a slow watch consumer could freeze the whole shard. Fixed by splittingdeleteinto a shard-safe leaf (deleteLocked) and dispatching the watch event from the caller afterward, matching the convention Create/Update already use.L0-A — five correctness fixes on the consolidation write path:
defer endFn(&err)fix so a panic mid-transaction rolls back cleanly instead of leaking a held write lock.kept-based filtering.HealDivergence) for the one regression that guard would otherwise introduce: a crash between payload-rename and metadata-commit leaving a profile's on-disk state ahead of its row. The heal takes the same per-key lock every other writer does, so it cannot race a live commit; verified against the vendored SQLite driver's exact commit/rollback semantics.L0-B — panic containment in the shard commit loop (
singlewriter.go), since every write in the system now routes through it: a caller-supplied job panicking used to either crash the process or wedge its shard forever. Three layers —callGuarded(recovers a panicking job into an error for its caller),putChecked(ensures the shard's pool connection is never leaked or double-returned even mid-panic), and arecover()around the commit loop itself so one bad job can't take its shard down. New metrics: panic outcome counter, dirty/dropped-connection counters.Process
Every fix in Lane 0 went through a full Planner→Architect→Critic design review cycle before implementation (Fable-driven, documented in
.omc/plans/raw-write-bypass-elimination.mdPart C, Revisions 13-20), was implemented by a separate execution pass with fail-on-today verification for every acceptance criterion, and then went through an independent, adversarial final code review that itself re-verified the trickiest claims against the vendored SQLite driver's actual source rather than trusting the design docs. That review's five non-blocking findings (a mislabeled heal-failure metric, a theoretical row-resurrection edge case in the heal, a pre-existing flaky race in an unrelated watch test, one documentation residual, one test-hardening nit) are folded in.Testing
pkg/registry/file,pkg/metrics,pkg/apis/softwarecompositionsuites pass.-raceclean across the whole package and 20x-repeated runs of every new/touched test, includingGOMAXPROCS=1and16variants — with one pre-existing, confirmed-unrelated flaky race inTestFileSystemStorageWatchReturnsDistinctWatchers(reproduced on unmodifiedmain) now also fixed as part of this PR (identity comparison instead of a racy deep-equal on live watcher state).Updates
docs/features/single-writer-priority-queue.mdanddocs/features/storage-lock-pool-metrics.mdwith these findings and fixes.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
AI-skills: none