Conversation
…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.
…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.
Internal + v1beta1 RogueArtifact/RogueArtifactList registered in both schemes. Generated deepcopy/conversion/openapi/protobuf follow from update-codegen.
…f/client for RogueArtifact Ran hack/update-codegen.sh + go-to-protobuf on a linux rig per the storage README (deepcopy interface markers + ListMeta on the List type). Full go build ./... green on the rig.
REST store + strategy + kubectl-get table converter (Name/State/Learning/ Workload/Kind/Phase/Since), registered as rogueartifacts in the endpoint map. go build ./... + vet green on the rig.
…ion test kubectl get rogueartifacts reads State/Learning/Workload from labels (List is metadata-only). TestRogueArtifact_RoundTrip pins Create + label-survives-List.
…art data The consolidated profile was saved only when a maintenance pass merged new part data. A lifecycle transition can arrive in a pass of its own: the completed/full finalization computes after the final part was already merged in an earlier pass, and the expired finalization by definition runs with nothing left to merge. In both shapes the pass mutated the in-memory annotations, deleted or cleared the time-series rows, and then skipped the save — leaving the served profile stuck in 'ready' forever with nothing left to consolidate (observed in CI: a profile whose processor logged 'completed/full, skipping further processing' was still served as not-completed 15 minutes later). Save now happens when the pass produced new data OR changed the status/completion annotations.
node-agent stamps kubescape.io/status: failed on containers that exit non-zero (helpers.Failed in k8s-interface), but ValidateStatusAnnotation never whitelisted it: every save of such a profile was rejected Invalid and the client retried the identical object forever, degrading the agent under crash-looping workloads. A failed container's profile is legitimate forensics and now persists.
path.Clean("") returns ".": an empty open path in a chunk (a failed
openat("") recorded upstream) became a literal '.' entry in the consolidated
profile. Empty input now stays empty.
Observed: a user-authored NetworkNeighbor with an explicit 'port: 0' literal
returns 'port: null' after create+get (live repro on a fresh cluster; standalone
gob probe: encode/decode of NetworkPort{Port:*0} yields Port=nil). encoding/gob
flattens the pointer and omits zero values, so the port-0 literal — defined as
'restrict to port 0 only' — silently becomes an any-port stanza for consumers
that treat an absent ports entry as a wildcard (node-agent R0011, Test_28
port_zero_is_literal_not_wildcard).
Fix: internal-only NetworkPort.PortZero marker, stamped at the single payload
encode site and consumed at the payload decode sites. Legacy payloads carry no
marker: nonzero ports round-trip unchanged and absent ports are never invented.
Unit gob_portzero_test.go pins survival (spec-level + section-level) and legacy
compatibility; pkg/registry + pkg/apis suites pass unchanged.
…ield The PortZero marker broke layout parity between internal and v1beta1 NetworkNeighbor, but the generated conversions still unsafe-cast the slices — consolidation's checksum marshal then read corrupted memory (SIGSEGV in json.Marshal under ./pkg/registry/file load tests). conversion-gen now emits field-wise conversion; the internal->v1beta1 direction is manual so the gob-only marker never leaks to API clients. json:"-" keeps it out of JSON views (golden fixtures, metadata rows); gob ignores json tags and still carries it.
Goroutine-dump-proven livelock: the consolidation transaction holds the write gate; its PreSave refreshes the collapse-settings cache (TTL 10s < 30s maintenance interval, so expired EVERY cycle); with no CollapseConfiguration CR applied the refresh Get hits a missing payload and the unconditional gated self-heal parks the full lockTimeout on the very gate its caller holds (fresh pool conn, so re-entry cannot match) — cpCtx dies, the whole pass rolls back, and the same key fails every ~35s cycle indefinitely (observed live: 385 failures at exactly 5.00-5.02s; four component tests starved serially; the episode stops within one cycle of applying the CR). Self-heal now (1) runs only when a metadata row actually exists for the key and (2) try-acquires the gate — best-effort, never waiting. Collapse TTL raised above the maintenance interval. Regression test pins a Get of an absent key under a foreign-held gate: red pre-fix at exactly lockTimeout, green post-fix; heal-when-needed still covered.
…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 (cherry picked from commit 957af4c)
…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 (cherry picked from commit fd5522b)
…tedData Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> (cherry picked from commit 3d546e0)
loadOrInitializeProfile's new-profile branch bypasses k8s.io/apiserver's generic Create path (it writes directly through storage.Interface), so nothing ever assigned a UID. An empty UID makes the generic PATCH handler's hasUID check treat the object as nonexistent, so any standard Kubernetes PATCH (kubectl annotate/label/edit, merge-patch clients) against an existing ContainerProfile returned 404 NotFound. Confirmed live against armo-dev-stage via delve, filed as kubescape#385, and now fixed by generating a UID the same way rest.FillObjectMetaSystemFields does for a standard REST Create. Audited every other in-repo fresh-ObjectMeta construction and confirmed none has the same gap. Deliberately out of scope: backfilling a UID onto ContainerProfile objects already persisted before this fix -- they remain unpatchable until naturally recreated by consolidation. Fixes kubescape#385 Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> (cherry picked from commit f56d126)
…path The four upstream fixes taken here are production-code fixes that apply cleanly. Two of the test files they carry do not, because they exercise upstream's single-writer/shard redesign — a design this fork deliberately has not adopted, and which upstream is still stabilising (a deadlock fix, a same-hour revert, a self-stall fix and two panic-containment layers landed in the nine days before the commit taken here). collapse_settings_self_stall_test.go called NewPool with a busy-timeout argument upstream added alongside that redesign. This fork's NewPool takes (path, size). The argument is dropped; the test's timing assertions are unchanged and it passes, so the self-stall guard the fix exists for is still covered. containerprofile_lane0_test.go is removed rather than adapted. It calls saveObject with upstream's signature — context, an old-object parameter and a commit callback — which belongs to the write path we did not take. Adapting it would mean porting the semantics we declined, not fixing a call site. That is a real cost and it is stated rather than hidden: the consolidation transaction's defer-and-rollback fix is taken WITHOUT its regression test. The fix itself is 26 lines and reviewed; the test that would pin it is coupled to a different write path and has to wait for the rebase that decides between them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G1r7S1rr7wMrTXhnEvjzMg
Reverts 828e305, 420eed5, 7fc16b5, e800f98 and 277af94, returning this branch to f2a1f02 — the tree is now byte-identical to that commit. Three component-test subtests began failing on the build that carried these, all in the authored and signed profile path: an authored profile binding to a workload, a signed suggestion healing in place, and a signed overlay being enforced rather than merely accepted. Each failed at a poll cap rather than on a wrong answer, which is the shape of waiting for something that never arrives. The same tests passed on the same node-agent image with the previous storage build, and this bump was the only deliberate change between the two runs. The run that would have isolated it was killed by an unrelated control-plane starvation on the test rig; a two-arm rerun on a clean cluster is in progress. So the picks are reverted rather than fixed, because the deployed chart is being standardised across the fleet and a suspected regression in the profile binding path is not something to ship while the question is open. The suspicion is not proven and may not survive the rerun. If it does not, these commits can return unchanged — they are individually sound and were verified before release. Of the four, the metadata.uid generation for new ContainerProfiles is the one worth examining first if the rerun implicates this bump, since it changes the object at create time and the failing tests turn on identity.
/proc/<pid> and /proc/<pid>/task/<tid> carry kernel-assigned numbers with no meaning past the lifetime of the task that holds them, and the collapse threshold cannot remove them. Collapse fires on a node's child COUNT, so it only reaches identifiers a workload produces in bulk: a container with three threads yields three tids, far below any sane threshold, and those three literals freeze into the profile. The next run of the same workload draws different numbers, so every open under /proc/<pid>/task/ is then unprofiled -- a permanent false-positive source that grows with each restart rather than converging. Both positions are now collapsed structurally, on first sight, independent of cardinality. Only all-digit segments qualify (plus an already-collapsed identifier, since the node agent rewrites /proc/<pid> at report time and the tid therefore arrives under a dynamic parent). Names stay names: /proc/self, /proc/thread-self, /proc/net and /proc/sys are unchanged, and in /proc/self/task/<tid> only the tid moves. The routing detail matters. These segments do NOT go through the level's DynamicIdentifier child: that child sets IsNextDynamic, which routes every sibling through it, so a single numeric pid would drag /proc/self, /proc/cpuinfo and /proc/sys into one pattern and cost the profile the distinction between reading a named procfs file and walking a task directory. They share a reserved trie key instead, unreachable as a real segment because segments cannot contain '/', and the node it reaches still emits the dynamic identifier, so the key never appears in a profile. A prefix an operator has configured as noise (Threshold 1) still wins and yields the wildcard. Scope is deliberately those two positions. File descriptors under /proc/<pid>/fd/ are left to the threshold, which does reach them. Consolidation folds profiles written before this: AnalyzeOpens re-analyses every stored path on save, so recorded literals converge to the pattern and their flags merge rather than being dropped. 28 new subtests fail on the pre-fix analyzer and pass after, covering first- sight collapse, the names that must survive, prefix anchoring (/procfs, /var/proc, tasks-not-task), the few-threads case the threshold cannot reach, profile-scope folding with flag merge, idempotence across repeated saves, non-widening to the zero-or-more wildcard, that the emitted pattern still matches live events and still rejects the neighbours it should, the threshold-1 override, and that cardinality collapse still applies below the tid. The rest of the storage suite passes unchanged.
…eal input The previous commit described the pid and the tid as one problem. They are not, and the difference matters for what the fix is actually worth. The pid ALWAYS collapsed. The node agent rewrites /proc/<pid> to the dynamic identifier at report time with an anchored regex, unconditionally and with no threshold involved. Only the tid was ever left literal: the regex is anchored at the start of the path and replaces one match, so /proc/<pid>/task/<tid> arrives at storage with the pid already dynamic and the tid still a number. That input is what made the level-wide dynamic child expensive. A dynamic child sets IsNextDynamic, which routes every sibling at that level through it, so the agent's own pid rewrite was by itself enough to swallow the named procfs reads. On the exact six-path shape the agent emits, the pre-fix analyzer produced six entries in which /proc/cpuinfo had become /proc/<dyn>, /proc/self/status had become /proc/<dyn>/status, and /proc/sys/kernel/randomize_va_space had become /proc/<dyn>/kernel/randomize_va_space -- a per-task path that cannot exist -- while all three tids stayed literal. It now produces four: the three named reads intact, and one folded task pattern. So the routing choice is not defensive. It repairs a loss of profile fidelity that was already happening in production on every container that reads its own procfs, which is most of them. This adds only a test. The analyzer is unchanged and the image already built from the previous commit is unaffected.
⋯ matches exactly one segment and * matches zero-or-more, so * is the broader of the two. processSegment routed an explicitly supplied * through an existing ⋯ child, rewriting it and NARROWING the stored profile below what the author wrote. It happened in both insertion orders, and to a * in a middle segment too. Applying a ContainerProfile that declares /bitnami/postgresql/data/base/⋯ /bitnami/postgresql/data/base/* stored only the ⋯ form. Every relation file two levels down — /bitnami/postgresql/data/base/16384/2650 — then fell outside the profile and raised R0002: 25 alerts on a ZITADEL postgres whose SBoB had declared exactly that subtree. An explicit * now promotes the node via createWildcardNode, which already absorbs the accumulated children. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfoMnkruL8WV7Njfk5Mmkj
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
3 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.
The bug
⋯matches exactly one segment and*matches zero-or-more, so*is the broader of the two.processSegmentrouted an explicitly supplied*through an existing⋯child, rewriting it and narrowing the stored profile below what the author wrote.Minimal reproduction against a live cluster — apply a ContainerProfile declaring four paths, read it back:
Order-independent, and it reaches a
*in a middle segment too (/a/b/*/c→/a/b/⋯/c).What it cost
A ZITADEL postgres SBoB declared
/bitnami/postgresql/data/base/*. Storage kept only the⋯form, so every relation file two levels down —/bitnami/postgresql/data/base/16384/2650— fell outside the profile. 25 R0002 alerts on a profile that had declared exactly that subtree, with nothing in the applied object to explain why: the file and the stored object differ by one line out of 358.The fix
An explicit
*now promotes the node throughcreateWildcardNode, which already absorbs the accumulated children.consolidateOpenswas not involved — its "patterns always survive" invariant holds; the loss happened earlier, during trie insertion.Tests
Both insertion orders, the middle-segment case, and the single-form cases. The middle-segment assertion is on coverage rather than on the literal string — a trailing
*already spans the segments below it, so a broader stored form is fine and a narrower one is not.TestStoredPatternStillMatchesWhatTheAuthorDeclaredpins the invariant that actually matters: whatever is stored must still match everything the authored pattern did.go test ./pkg/registry/...passes.🤖 Generated with Claude Code
https://claude.ai/code/session_01MfoMnkruL8WV7Njfk5Mmkj