fix(storage): consolidation self-stall on the CollapseConfiguration settings cache - #401
Merged
Merged
Conversation
…s goroutine A consolidation save calls PreSave from inside processTimeSeriesInTransaction, on a connection that already holds SQLite's write lock (the pass has run ReplaceTimeSeriesContainerEntries). PreSave calls the CollapseSettings provider; when its 10s cache had expired the provider did a storage Get on a SECOND pool connection, and with no CollapseConfiguration CR present get()'s missing-payload DeleteMetadata on that connection needed the write lock the same goroutine holds. It waited in SQLite's busy handler for the full busy timeout (DefaultBusyTimeout, 60s), holding the WAL writer lock the whole time: every shard commit and every other consolidation worker queued behind it. Nothing else refreshes this cache in production (TS profile Creates return from PreSave before the provider call) and the consolidation interval (30s) exceeds the TTL (10s), so this fired on every tick that had data to consolidate, whenever no CR was applied. Live since 2ea734d (v0.0.291); 6abe45d's cache reduced it from per-save to per-tick. Make the provider stale-while-revalidate: prime the cache synchronously at wiring time (no transaction is open there), serve every later call from one atomic load, and run the refresh in a CAS-guarded background goroutine. The caller's goroutine never takes a connection, a lock or a statement. Staleness becomes TTL plus one call; the two tests that asserted "the very next call reflects the edit" now use Eventually. The TTL is captured once at construction so the background refresh never reads the package var. Acceptance (fail before / pass after, real pool, 2s busy timeout): - TestCRDCollapseSettingsProvider_NoStorageIOUnderHeldWriteLock: 2.004s -> <500ms - TestCRDCollapseSettingsProvider_NoLockWaitOnCallerGoroutine: 1.000s -> <500ms - TestConsolidateTimeSeries_DoesNotStallOnCollapseRefresh: 2.005s -> <500ms Design: .omc/plans/collapse-settings-self-stall.md Docs: docs/features/collapse-settings-self-stall.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
get() pruned the metadata row whenever the payload file was missing, without checking that a row exists. A DELETE that matches no row still opens a write transaction, so every read of an absent key acquired SQLite's write lock and waited behind any in-flight writer for up to the busy timeout (60s), which the C busy handler does not interrupt. This is the statement that turned the CollapseConfiguration refresh into a self-wait (previous commit) and it makes REST GETs of missing keys queue behind long writers. Check ReadMetadata first and delete only an existing orphaned row. A read of an absent key is now a SELECT, which never waits on a writer in WAL mode. Orphan pruning (a51d55f) is unchanged; the corrupted-payload branches, where a row is expected, are untouched. Acceptance (fail before / pass after, real pool, 2s busy timeout): - TestGet_AbsentKeyDoesNotWaitOnWriter: 2.004s -> <500ms - TestGet_PrunesOrphanedMetadataRow: guard, passes before and after Docs: docs/features/collapse-settings-self-stall.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
…ockTimeout Review follow-up on the self-stall tests. Two latent -race shapes: - NoLockWaitOnCallerGoroutine shrank lockTimeout and restored it in t.Cleanup while the background refresh reads it in acquireLockedConn. pool.Close (registered earlier, so it runs later in LIFO order) joins only a goroutine that already holds a connection, and the read happens before pool.Take, so a late-scheduled refresh could race the restore -- the same shape as the collapseSettingsTTL race fixed in the provider. Leave lockTimeout at its default: it only bounded the pre-fix failure (now 5.004s instead of 1.000s), nothing post-fix needs it. - All three tests relied on pool.Close to join the refresh. Wrap the real storage in a Get-counting shim and require.Eventually the prime plus one refresh have completed before the test returns, after the timing assertions so the pre-fix failure is still reported as the stall. Re-verified: fixed tree 20x under -race clean; on unfixed origin/main the three tests fail on the timing assertion (2.003s / 5.004s / 1.980s). Docs: docs/features/collapse-settings-self-stall.md 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 |
Contributor
Author
|
cc @entlein |
|
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
Fixes a consolidation goroutine self-stalling on its own held SQLite write lock for up to the full busy-timeout (60s in production) via an unrelated settings-cache refresh — confirmed as the dominant driver of kubescape/node-agent's component-tests CI flakiness (real-CI validation dropped failures from a 15-19/31 baseline to 0-1/31 with a workaround alone).
Overview
A consolidation goroutine holding SQLite's write lock (inside its own deferred transaction) could self-stall for up to the full busy-timeout (
DefaultBusyTimeout, 60s in production) via an unrelated settings-cache refresh.Root cause:
NewCRDCollapseSettingsProvider's cache (10s TTL) is refreshed from insideContainerProfileProcessor.PreSave, which itself runs inside consolidation's open transaction on connection A. The refresh callsGeton a second pool connection B. When noCollapseConfigurationcustom resource exists, that read's underlyingget()finds no payload file and issues aDeleteMetadataon connection B — which needs the same SQLite write lock connection A already holds, on the same goroutine. SQLite's busy handler retries for the full timeout before giving up.This fires on essentially every consolidation tick with data whenever no
CollapseConfigurationCR exists (node-agent's RESTCreatereturns fromPreSavebefore this point, so only consolidation saves trigger a refresh; the 30s consolidation interval exceeds the 10s TTL, so nearly every tick hits an expired cache). Live since v0.0.291 (2026-06-26) — this predates and is independent of the write-shard/single-writer work in #397/#399.Real-world confirmation: adding a
CollapseConfigurationresource to kubescape/node-agent's component-tests chart (zero storage code changes) dropped CI failures from a 15-19/31 baseline to 0-1/31 across three independent runs — this is very likely the dominant driver of the flakiness that motivated the broader write-path investigation.Fix
Two independent commits, each with its own fail-before/pass-after tests:
957af4cd—NewCRDCollapseSettingsProviderbecomes stale-while-revalidate: primed synchronously at construction, hot path is a single atomic load, refresh runs in a CAS-guarded background goroutine. No pool/lock/statement work ever happens on the caller's goroutine again.e3429c2f—get()checksReadMetadatabefore issuingDeleteMetadataon a missing payload, so a read of an absent key is a SELECT, never a write-lock-requiring DELETE (defense in depth; orphan-pruning behavior for a key whose row does exist is unchanged).bf690e36— test hygiene follow-up from independent review: removes a latent-raceflake (a shrunklockTimeoutpackage var the background refresh could read unsynchronized) by joining the refresh explicitly instead.How to Test
All new tests fail deterministically on unfixed code at ~the busy-timeout duration and pass after each corresponding fix;
AC-B1still fails with only Fix A applied, confirming both fixes are independently necessary.Additional Information
Went through an independent adversarial code review (verdict: SHIP, no blocking findings) that re-derived every claim from source rather than trusting the design doc, including tracing the CAS-guarded refresh for genuine race-freedom and checking all six callers of
get()individually. Full root-cause chain, design rationale, and review are recorded indocs/features/collapse-settings-self-stall.md.git merge-treeagainst currentmain(which now includes #399) reports zero conflicts.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd
AI Review
Independent code-review subagent (Claude, Fable model) · verdict: SHIP, no blocking findings. Re-derived every claim from source rather than trusting the design doc: reproduced the fail-before timings independently in scratch worktrees, traced the CAS-guarded background refresh for genuine race-freedom, and checked all six callers of
get()individually to confirm none depend on the removed DELETE side effect. Two minor test-hygiene items found (both latent-raceflakes in test code, not production code) — addressed inbf690e36.Ticket
None