feat(storage): full-ACID ContainerProfile backend with write-gate-sharing by matthyx · Pull Request #402 · kubescape/storage

matthyx · 2026-09-11T15:33:40Z

Summary

Adds a full-ACID ObjectStore backend for ContainerProfile storage — one atomic BEGIN IMMEDIATE SQLite transaction per write, payload stored as a payloads table BLOB, replacing the legacy split row(SQLite)+payload(file) design. Gated behind ContainerProfileSqliteBackend (default false); the legacy path is untouched and remains the default until this is rolled out.

Bundled into this branch because they were found and fixed along the way:

Write-gate-sharing: closes an entire bug class (an ungated writer busy-waiting against gated traffic for the full SQLite busy-timeout) across all 13 resource kinds, found independently twice before being fixed systematically (W1-W13 enumeration, one process-level gate).
Consolidation-starvation fix: a per-series reservation (sqliteobject_keyreserve.go) bounding same-series writer/consolidation contention, with two follow-up tuning passes and a schema fix (see below) once Tier B measurement caught real regressions the initial fix introduced.
Startup migration (sqliteobject_migration.go) from the legacy on-disk format into the new schema, plus cpexport (cmd/cpexport), a reverse-export tool that reconstructs legacy files so a downgrade to an older image doesn't corrupt the store (full-ACID payloads can't be read by an old binary without it).
postgres-connector compatibility: verified the new backend doesn't break armosec/postgres-connector's parallel implementation — the coupling is purely at the ContainerProfileStorage Go interface, unaffected as long as that interface itself doesn't change.

Validation (Tier B A/B measurement, `hack/perf-ab.sh`)

Final confirmation run: PAIRS=40 on a dedicated (non-shared-CPU, 0% steal confirmed) DigitalOcean droplet, comparing legacy vs. objectstore backends on identical code otherwise.

metric	effect	verdict
`update-p95-ms`	-12.0%	PASS
`list-p95-ms`	-70.5%	PASS
`tick-p99-ms`	-87.2%	PASS
`get-p99-ms`	-41.0%	PASS
`create-p99-ms`	-53.5%	PASS
`ops-per-s`	+80.8%	PASS
hard safety gates (panics, ungated writes, `over-five-sec`, `err-other`)	zero on both arms	PASS

Two real regressions were found and fixed during this validation, not just tuned away:

update-p95-ms regression (writers occasionally blocking up to 1s on an active consolidation reservation) — fixed by shrinking the writer-side wait bound to 50ms, then splitting it from an unrelated retry-vs-retry queueing wait (keyReserveWaitMax vs. new keyReserveQueueMax) once a naive shared-knob tune traded the cost onto a second metric.
list-p95-ms regression, caused by (1)'s fix: shorter writer waits meant more un-consolidated TS rows sitting in the table, and List had no way to exclude them from its scan — fixed with an indexed is_time_series column (additive migration, backward-compatible) filtering them out of the three List SQL paths, verified locally via go bench (row-count sensitivity 16-18x → ~1.4x) before spending another droplet cycle.

Two metrics remain informational/explained, not blocking:

cleanup-tick-ms (+2047%): expected queueing behind the same shared write-gate under +80.8% more ops/s — not a bug, same design tradeoff write-gate-sharing was built around.
The harness's own load-marked contamination gate fires on ~80% of pairs; root-caused via a controlled swap experiment to be a real, stable property of the legacy backend generating more system load (not measurement noise or a harness bug), so it's expected and doesn't invalidate the per-metric statistics above (computed independently of that flag).

write-bytes overhead (+10%) was also checked: fixed per-object cost, shrinks toward 0% as payload size grows (1MB: -0.2%, 5MB: +0.006%), not a scaling risk.

How to test

go test ./pkg/registry/file/... -count=1 — full suite, including new concurrency/starvation/migration-scale/List-exclusion regression tests.
hack/perf-ab.sh (see above) for the A/B performance comparison; PERF_AB_BACKEND=objectstore to exercise the new path locally.
cmd/cpexport has its own test building and running the binary against a populated store.

Rollout

Feature-flagged off by default (ContainerProfileSqliteBackend=false). Opening as a draft given the scale (schema/migration changes to core storage) — not requesting merge yet, this is for review of the design and the validation evidence.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BkhY5wB7cKArz1dMxAoP3c

AI Review

Agent-based design/code review (Claude Code subagents) across multiple passes during development: write-gate-sharing design (Architect/Critic revisions), migration sweep (caught a silent-data-loss predicate-ordering bug before it shipped, fixed in 020027376), the is_time_series List-exclusion schema fix (independently re-verified: diff review, local rebuild, full test suite, before pushing to measurement).
Verdict: no unresolved findings; all issues found during review were fixed in the commits listed above, not deferred.

Ticket

None

…d-in golden Count, per hot-path scenario, the SQL statements by Go call site, the per-key lock acquisitions by mode, the pool takes, the payload-file operations and the watch events, and compare them exactly against pkg/registry/file/testdata/workbudget.golden.json. Eight single-goroutine scenarios: S1 learning tick, S2 empty tick, S3 frozen tick, S4 divergent tick, S5 REST Get, S6 REST Create of a TS profile, S7 no-op GuaranteedUpdate, S8 delete of a processed TS profile. Runs inside `go test ./...`, ~1 s, no env gate; `-update` rewrites the golden, and a golden change is a review item stated per row in the commit. Instrumentation, nil in production (one atomic load and a nil check per call, no allocation): - observeStmt at the 13 sqlitex.Execute sites in sqlite.go and at the three transaction openers (Transaction, Save:saveObject, Save:commit), behind an atomic.Pointer so the harness can install and clear it while a leaked goroutine from another test is still executing statements. - utils.SetLockObserver in pkg/utils/mutex.go: Lock/RLock report their mode and outcome, the read/write axis the lock-wait histogram lacks. One production change: ObservePoolWait at the two connection-take sites that never reported to storage_pool_wait_duration_seconds -- ContainerProfileStorageImpl.WithConnection (the listing and every consolidation worker) and createSingleWriter's AfterCreate connection. The histogram's Help text already claimed to cover them. Every scenario pins singleWriterEnabled, runs under a package mutex, and ends with a settle check: observers stay installed for 50 ms after the scenario returns, are cleared, and are read once more under their mutex; a late statement, lock, file op or pool take fails the test as `contaminated: <site>`, never as a flaky golden mismatch. Hook closures never touch testing.T. Four fixed invariants hold regardless of the golden: a no-op writes nothing (S2, S7); a frozen tick writes nothing to the base (S3; gated on frozenTickInvariant until the frozen gate lands, because on this tree the tick still rewrites a Completed/Full base); one REST read is one read lock and one connection (S5); ROLLBACK is zero. Golden recorded at this commit (main, 4535872). Rows worth a reader's attention: every consolidated-profile PreSave issues a DeleteMetadata for the missing SBOM payload (2 per save in S1/S3/S4, 2 in S7); the consolidation pass deletes processed TS profiles on the worker's own connection with no lock (S1: 3 deletes, 0 takes, 0 locks). Design: .omc/plans/raw-write-bypass-elimination.md A.13.3 (Revision 25, with the Critic's R-1..R-3 corrections applied). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…nd a relative verdict Refactor the load test into runLoadScenario(cfg) and add TestPerfABRound: one round of FIXED WORK with CLOSED-LOOP clients on the production shape (pool 10, 8 shards, Workers 2, GOMAXPROCS=8; 6 writers x 2400 Creates, 25 readers x 12000 Gets, 3 updaters x 1200 GuaranteedUpdates, 12 base keys, consolidation ticking every 250 ms plus three ticks after the clients finish -- about 30 s of work per round), reported as JSON -- per-class percentiles, wall time and ops/s, the six process-registry series as per-round deltas, and an `effective` block read back from the constructed objects (probed pool size, len(shards), Workers, GOMAXPROCS, singleWriterEnabled, op counts, the pinned collapse-settings TTL) -- and as BenchmarkPerfAB/* lines for benchstat. A request-context deadline is classified as a timeout, not an "other" error. Every profile is pinned Learning/Partial so no base freezes and every Create reaches the commit path. hack/perf-ab.sh (make perf-ab) builds BASE (default the merge-base with origin/main) in a worktree with HEAD's harness overlaid, refuses to start when the 1-minute load exceeds nproc/2, pins both binaries to nproc/2 distinct cores, runs an A A A early-abort probe, interleaves PAIRS=10 base/head rounds on fresh databases while sampling the load average per round into schedule.txt, and hands the rounds to hack/perfab for the verdict: a paired t-test on per-round log-ratios as the primary statistic, Mann-Whitney U as the second opinion, REGRESSION on threshold breach and either test significant (SPLIT flagged), the paired CV and the MDE computed post hoc with t_{N-1} quantiles, N' iterated once, hard rows for errOther / >5 s / commit panics, and CONFIG MISMATCH when any round's effective block differs. Exit codes: 0 PASS, 1 REGRESSION, 2 CONFIG MISMATCH, 3 INCONCLUSIVE (probe CV, paired CV, or more than ceil(PAIRS/3) load-marked pairs), 4 UNDERPOWERED. Thresholds are pre-registered in testdata/perfab.thresholds.json; no absolute latency is checked in. Two harness isolation choices, both echoed in `effective`: the SQLite busy timeout is 5 s, and collapseSettingsTTL is pinned to an hour. With the 10 s default, a consolidation save that has already written refreshes the CollapseConfiguration cache on a second connection; with no CR present, get()'s DeleteMetadata on that connection waits on the write lock the same goroutine holds until the busy timeout, and every shard commit waits with it. Whether a round crosses a TTL boundary is wall-clock phase, not the change under test (it made half the rounds 6 s longer); the stall is a bug in its own right, visible in the over-one-sec row of an unpinned run. GuaranteedUpdate is gated on p95, not p99: under the pinned shape about 1% of updates hit acquireLockedConn's 250 ms connection-attempt cliff, so its p99 straddles the cliff and is bimodal round to round (~40 ms vs ~280 ms) while p50/p95 are stable; p99 is still reported (info) and the cliff itself is the pool-wait-timeouts row. .github/workflows/perf-ab.yaml runs the same driver on workflow_dispatch, nightly, and on PRs labelled `perf`, reporting the verdict (including INCONCLUSIVE) in the check summary and failing only on REGRESSION. CONTRIBUTING.md and docs/features/storage-measurement-harness.md state the rule: hot-path PRs carry the golden diff and the verdict line. Design: .omc/plans/raw-write-bypass-elimination.md A.13.4 (Revision 25, with the Critic's R-4 effective-config echo). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

… and PoolOptions with wal_autocheckpoint=0 Additive, nullable columns and a separate payloads table so the 13 legacy kinds are untouched; SchemaMigrations() is exported so harnesses build the same pool. NewPoolWithOptions sets PRAGMA wal_autocheckpoint=0 in PrepareConn for EVERY connection when asked (K-3: autocheckpoint is a per-connection sqlite3_wal_hook), and lets tests install a statement authorizer. Adds docs/features/containerprofile-sqlite-backend.md describing the whole prototype (design: .omc/plans/full-acid-storage-architecture.md §3.2, §6.2). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

storage_sqlite_write_hold_seconds{path}, storage_write_gate_wait_seconds {priority}, storage_sqlite_busy_wait_seconds, storage_cp_cas_conflict_total {op}, storage_cp_ownership_refusal_total{op}, storage_sqlite_wal_pages, storage_sqlite_freelist_count, storage_sqlite_checkpoint_total{outcome} (design §9 observability; PM-3's detector lands with the store). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

… ObjectStore The gate is a caller-side two-lane FIFO ticket semaphore owning one dedicated write connection taken from the pool once (design §6.4, R6): acquisition order prepare -> ticket -> BEGIN IMMEDIATE -> COMMIT -> release -> dispatch; the releaser hands the ticket straight to the next waiter (highBurstLimit fairness re-hosted from singleWriter.run); a waiter whose ctx fires at the instant of grant hands the ticket back (INV-5); panics in the body are contained and rolled back (L0-B); a connection that cannot be cleaned is replaced, never dropped; Close returns the connection so Pool.Close can complete (K-5). The checkpointer runs PRAGMA wal_checkpoint(PASSIVE) on its own pooled connection, kicked by a -wal size check after each gated commit (zombiezen v1.4.0 has no sqlite3_wal_hook) plus a timer for ungated writers, supervised with recover/restart and counted (K-3). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…file backend storage.Interface over metadata (rv/uid columns) + payloads (versioned JSON body) + time_series, written in ONE BEGIN IMMEDIATE...COMMIT on the gate's connection (design §3.4): Create = in-transaction TS admission via json_extract + INSERT ON CONFLICT DO NOTHING + payload + time_series row; GuaranteedUpdate = UPDATE ... WHERE rv=:rv AND uid=:uid (changes()==0 -> conflict, re-read, retry) with changes()==1 asserted on the payloads UPDATE (K-6); Delete = metadata RETURNING + payloads + time_series. Nothing but SQL on prepared bytes runs under the gate (INV-1). GET/LIST read the join; the metadata LIST statement is the legacy one, so continue tokens stay rowids. ContainerProfileStorage over the store (§3.6/§3.7): WithConnection hands the pass a read handle, BeginTransaction opens a staged write set, Save/Replace/ Delete stage prepared statements with CAS predicates captured from the same reads (R4: per-TS rv/uid on processed deletes), and the end function commits the set under the gate, reporting ErrWriteConflict on any failed CAS. GetSbom delegates to the legacy StorageImpl (R7). Processor adaptations (mechanical, legacy path unchanged): TimeSeriesRowFor lets a backend fold AfterCreate's row into Create's transaction; AfterCreate goes through the TimeSeriesEntryWriter interface instead of a type assertion (R8); a ProcessedDeleteStager backend receives the processed-TS deletes before the end function (so they join the tick); one retry on ErrWriteConflict (§3.7 Phase 3, N=2); ConsolidationHooks is a test seam. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

SetForeignKinds installs a predicate; get (full branch only), fetchListPage (fullSpec), getListWithSpec, appendGobObjectFromFile, delete, CreateWithConn, GuaranteedUpdateWithConn, createSingleWriter and guaranteedUpdateSingleWriter REFUSE a foreign kind's key with an InternalError (counted, logged) before touching a row, a payload file or the self-repair deletes. Metadata-only reads are not refused (design §5.6). Refuse, not route: a mis-wiring must fail loudly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

Off by default. When on: the pool disables autocheckpoint on every connection, the default StorageImpl carries the ownership guard, containerprofiles are served by the ObjectStore (NewREST and NewCustomREST receive it unchanged) and its Close runs as a pre-shutdown hook before the pool is closed (K-5). Only what the prototype needs; no migration, cleanup or GNP re-pointing (§7.6). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…NV-5 tests CAS matrix, create states, in-transaction TS admission (gap 1), K-6, K-3, K-5, codec fidelity, pagination; gate FIFO/burst policy, INV-5 hand-off under cancellation, Close, panic containment; checkpointer trigger, timer fallback, supervised restart; INV-1 (no callback, dispatch or pool take under the gate; only metadata/payloads/time_series on the gate connection); INV-4 (every guarded legacy path refuses a CP key with zero statements, zero writes by PRAGMA data_version, zero file ops, rows untouched; metadata reads served). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

Storage-level: identical sequences (errors, RVs, LIST pages, watch events, canonical objects) plus the intended divergences asserted as WHAT differs: AfterCreate crash atomicity, TS admission after base completion, rowid-stable pagination (and its continue-token side effect), creationTimestamp truncation, the R4 per-TS CAS conflict-and-retry - and a sixth the design did not list: v1beta1 spec collections without omitempty decode as [] from JSON and nil from gob. Consolidation windows compared; INV-3 asserted as parity because the X-A frozen gate is Lane 0 code absent from origin/main. REST-level: the same CustomREST over both backends (Phase 4 pattern). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…y boundary Generated Create/Update/Delete/TS-create/tick sequences against a model, with the gated transaction interrupted at every statement boundary by an error, a panic, or a rollback-and-abort (a process crash as SQLite sees it); INV-2 (metadata row <=> payloads row, rv == json rv, uid == json uid) asserted from a fresh connection after every step, plus a deterministic sweep of every (kind x boundary) proving pre-tick-or-post-tick, nothing between. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md (schema commit of this series) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…cid-containerprofile Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…ent observer The Step 1-L harness selects the backend with PERF_AB_BACKEND=legacy|objectstore (provenance in the round JSON, not in the effective config, so the two arms of an A/B are not a CONFIG MISMATCH; the gate's owned connection is added back to the probed pool size). hack/perf-ab.sh gains PERF_AB_BASE_ENV / PERF_AB_HEAD_ENV so BASE=HEAD compares two configurations of the same commit. Per the design's PM-1 and C.14: a LIST client class (list-p99-ms, headline) and write-bytes from /proc/self/io (info). The harness pool now uses the production schema and K-3's autocheckpoint setting for the objectstore arm. INV-4 additionally asserts zero executed statements through Step 1-L's observeStmt (the design's instrument). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…read connection Tier B on the ObjectStore showed 5-10 s silent stalls on ticks and updates. The store's own histograms cleared the gate (queue wait p99 29 ms, BEGIN IMMEDIATE busy wait p99 0.5 ms, hold p99 187 ms) and pointed at the checkpointer: with 25 readers pinning WAL read marks the -wal file never drops below the kick threshold, every commit re-kicked it and it ran 7,104 PASSIVE checkpoints in one 26 s round, rewriting the wal-index header continuously; readers that see the header change retry with SQLite's quadratic backoff. A minimum spacing between runs (250 ms, kicks coalesced) brings that to ~50 per round and removes most of the tail. GetSbom (R7) delegated to the legacy StorageImpl.Get, which took a second pool connection while the caller already held one; reuse the caller's read handle through GetWithConn (still the default instance's lock map). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…yloads (PM-1) A plain metadata JOIN payloads with the page predicate on metadata let SQLite drive the join from payloads and read every body: Tier B measured LIST p95 6.8 ms (legacy) vs 80 ms. The page is now selected in a subquery on metadata by rowid (the legacy listMetadataKeys statement) and joined to payloads by primary key - the design's PM-1 mitigation, applied. Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…ot the legacy self-repair path Root cause of Tier B's 5-10 s stalls on updates and ticks (never on GETs or TS creates): PreSave on a base profile calls GetSbom; when no SBOM exists the legacy get() opens the payload file, misses, and runs its self-repair DELETE FROM metadata unconditionally - a write-lock acquisition on an ungated pool connection that busy-waits behind the gate's continuous commits for the whole busy timeout (5 s in the harness, 60 s in production), twice per update (the 10 s maxima). Verified by elimination: gate/busy/hold histograms clean, stalls independent of reader count (1 vs 25), present only on the two paths that read the SBOM. Three rounds after the fix: 0 ops over 1 s (was 10-39 over 5 s per round), update max 6-23 ms (was 10 s), tick p99 0.3-0.45 s (was 4.3 s). This is the design's ungated-writer-vs-gate class (§5.3, §6.2) surfacing on a legacy-kind READ; the general fix is gate sharing, out of the prototype's scope. Docs-exempt: covered by docs/features/containerprofile-sqlite-backend.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…not instant The two backends stamp metav1.Now() in their own ticks; runs straddling a second boundary differed by one truncated second under -race. Docs-exempt: test-only change Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

Reconciles the full-ACID ObjectStore prototype with Lane 0's merged X-A frozen-profile guard, E3 divergence heal, PC-TS-4 processed-delete staging, PRE-3, and L0-B panic containment. Conflict resolutions: - pkg/metrics/metrics.go: pure union of both sides' new metrics. - pkg/registry/file/storage.go: kept Lane 0's deleteLocked/delete split (L0-B: endFn must be deferred for panic safety), re-added the prototype's refuseForeign kind-ownership guard to the delete() wrapper (the same choke point both Delete and DeleteWithConn go through). - pkg/registry/file/containerprofile_processor.go: merged Lane 0's E3 divergence-check block and panic-safe deferred endFn into the prototype's ProcessedDeleteStager-aware processTimeSeriesInTransaction (three-value return, ErrWriteConflict passthrough for the retry-once wrapper), and the ConsolidationHooks/seriesOrder struct fields. Implements objectStoreCPStorage.HealDivergence (sqliteobject_cpstorage.go), the interface method Lane 0 added to ContainerProfileStorage. The divergence shape legacy's HealDivergence repairs -- payload rename landed, metadata row's COMMIT did not -- requires two independently timed write steps; ObjectStore writes both in one BEGIN IMMEDIATE ... COMMIT (stampAndEncode's metadataJSON and body come from the same in-memory object), which SQLite's WAL makes atomic across a crash (recovery discards any transaction without a valid final commit frame, regardless of synchronous=NORMAL vs FULL, which affects durability, not atomicity). The shape is therefore unreachable by construction, so this is a fast defensive check, not a repair: re-reads metadata and payload from ONE statement (readRow's join, not the caller's two racy separate reads) and pages (metric + Error log) if they ever disagree, rather than guessing at an undefined-here repair. Fixed a test call site (containerprofile_lane0_test.go) for the merged three-value processTimeSeriesInTransaction signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…inerProfile Post-merge verification found X-A (the frozen-profile guard) was NOT actually uniform across backends: legacy's SaveContainerProfile tryUpdate closure rechecks IsCompletedFull(input) at write time and returns ErrProfileFrozen, but objectStoreCPStorage's closure ignored `input` entirely and always returned the merged profile. The shared frozen gate in ContainerProfileProcessor.updateProfile only catches a base that was ALREADY Completed/Full when the tick started; it cannot see a completer that lands after that read but before this pass's own save. On the ObjectStore backend that race would silently overwrite a just-completed base with a stale merge (and, in the staged/write-set path, actually succeed the CAS, since the Phase 1 read that supplies the CAS expectation happens after the completer's commit). Fixed by mirroring legacy's tryUpdate check in objectStoreCPStorage. SaveContainerProfile, using the fresh `input` GuaranteedUpdate/the staged path's own Phase 1 read supplies -- not the pass's stale read -- so the whole tick's write-set transaction correctly refuses and rolls back, exactly as the legacy comment describes. Added TestX_A_SaveRefusesConcurrentlyFrozenBase (the race window: completer lands between a pass's frozen-gate read and its own save) and updated TestINV3_FrozenBaseParity to assert Lane 0's real X-A guarantee (RV unchanged, no merge, no Modified event, series reclaimed) now that Lane 0 is merged, instead of the old pre-Lane-0 parity placeholder. Both pass identically on both backends. Regenerated testdata/workbudget.golden.json (Step 1-L Tier A) for the legacy path's real, now-merged costs: - S3/S4 (frozen/divergent tick): X-A's frozen gate short-circuits to reclaim-unmerged instead of the old full merge+save, eliminating saveObject/WriteJSON/WriteTimeSeriesEntry/fs.rename/events.Modified and most of the old path's reads and locks. - S1 (learning tick), S3, S4, S7 (no-op update): DeleteMetadata drops because df50b1e (already on origin/main) removed GetSbom's legacy self-repair DELETE FROM metadata on an absent SBOM -- PreSave calls GetSbom on every container-profile save these scenarios exercise. - S1: ReadMetadata rises from E3's new autocommit divergence-check (GetContainerProfileMetadataNoLock) at the top of every tick. - S8 (delete_ts): lock.lock and pool_take each +1 because deleteProcessedTimeSeries now routes through deleteContainerProfileArbitrated -> singleWriter.runOnShard (PC-TS-4), taking the shard's connection and per-key lock instead of a raw, ungated connection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…pology (R0a) The write-gate-sharing harness, part one: the half that stands on plain main (design: .omc/plans/write-gate-sharing.md §7 AC-G2, §4 R0). - migrationBinaryPath now reaches all three gob-migration exec sites (migrateObject / W6 and appendGobObjectFromFile / W8 hard-coded /usr/bin/migration; only execMigrationTool / W7 had the seam). Behaviour is byte-identical; the seam lets the matrix's wrong-type cells split by tool outcome on every path (CR-4). - openFixtureConn: a non-pool sqlite.OpenConn on the same file, the one handle tests seed state through (CR-2b), opened after the pool's migration with a busy timeout (R-9). - TestACG2_FlagOff: 13 read entry points × 6 key states (+3 cleanup-tick states), each cell in a fresh env with a pool connection holding BEGIN IMMEDIATE. Non-repair cells < 500 ms; the repair cells (get()-readers × orphan/corrupt/wrong-type, list readers × wrong-type/tool-succeeds, cleanup × unreferenced/file-without-row) are pinned to the busy-timeout stall — the legacy topology's residual as a golden, so a regression and a fix both show as a red cell. Cells run in parallel (one migration script decides by path), ~1.2 s wall. Two matrix precision notes against the design text: GetList(fullSpec) is a get()-reader (fetchListPage → get(noLock)), so its orphan/corrupt cells repair like Get's; and the corrupt fixture is an empty payload (io.EOF), the only truncation shape get() classifies as corrupt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…expected red (R0b) Part two of the write-gate-sharing harness (design §7 AC-G1, §3.6, T-G1, T-G2). This commit is RED by design: on the prototype's flag-on topology (ObjectStore gated, legacy kinds ungated) it records W1–W9b as ungated writes and fails at exactly those sites. That failing run is the evidence the bug class exists there (R0's gate: "red at exactly W1–W9 and nowhere else"); R1 turns it green. Production side: - sqlite.go: every pool connection carries a write authorizer that reports each INSERT/UPDATE/DELETE prepared on it (prepare-time: a cached statement was first prepared, and recorded, on that same connection). Count-only in production: storage_sqlite_ungated_write_total{op,table}, the R2 canary. PoolOptions.Authorizer chains a second authorizer (tests' recorders). - writegate_registry.go: one live gate per pool, enforced at construction (errGateExists; PM-G8), package-level map keyed by pool pointer (R-7). - sqliteobject_gate.go: `owned` = every connection the gate has ever held (initial + each cleanOrReplace replacement, never removed) and owns(conn, seq) with the Close sequence bound (CR-1, R-8). Harness: - main_test.go: TestMain installs the AC-G1 ledger (record + stack, judged per pool at each gated env's cleanup and swept once more at exit; pools that never had a gate are vacuous). - tableRecorder is always-on from PrepareConn (start/stop deleted, mark/ since instead — CR-2c); every ObjectStore-suite fixture write moves to the non-pool fixture handle (CR-2b); flag-off suites are unarmed and unchanged. - TestTG1_EveryLegacyWriteSiteIsGated: W1–W9b, one env each, asserting no write on a non-owned connection and the site's statement on the gate's. - TestACG2_FlagOn: the matrix under flag-on with the gate as the holder (CR-3), plus PreSave(base CP); pool size 4 (R-9). - TestWriteGate_SecondGateOnPoolRefused; TestWriteGate_SwapKeepsPreSwap- ConnectionOwned (T-G2 swap variant). The swap is forced by a panic inside fn after installing a closed interrupt: sqlitex's end function clears the interrupt before its own ROLLBACK, so CR-5's "return an error" would not reach cleanOrReplace's replacement branch; the gate's panic path does. Red run on this commit (go test ./pkg/registry/file, 16 s): FAIL TestTG1_EveryLegacyWriteSiteIsGated — all 13 sites, e.g. W9b: "insert on metadata prepared on a pool connection the write gate never owned; prepared at: WriteJSON ← ResourcesCleanupHandler.readMetadata ← cleanupNamespace" FAIL TestACG2_FlagOn — exactly the repair cells (32), each at AC-G1 (their timings, 305–331 ms, are inside the bound: a single 250 ms hold lets the busy-handler poller win at its 328 ms wake-up, which is why AC-G1, not the stopwatch, is the discriminator) FAIL TestINV4_LegacyStoreRefusesContainerProfileKeys and TestINV4_UnguardedLegacyFullReadDeletesOwnedRow — the pre-existing tests that drive legacy writes under flag-on, at AC-G1 everything else PASS, including every writeGate test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…for sharing (R1a) The gate becomes the process's, owned by neither store (write-gate-sharing §3.1, §3.6): main.go builds it beside the pool when ContainerProfileSqliteBackend is on and hands it to the ObjectStore (which no longer builds or closes one), the legacy StorageImpl and the cleanup handler (SetWriteGate; the nine legacy sites are routed in the next commits). The apiserver's pre-shutdown hook closes the store, then the gate, before Pool.Close (K-5). Config validation refuses containerProfileSqliteBackend without singleWriterEnabled (RC-6), and StorageImpl carries ErrGateRequiresSingleWriter for the runtime half. Gate API: - run(ctx, priority, path, kind, fn(ctx, conn)): fn receives a gate-marked ctx (valid for the dynamic extent of fn, R-5); a nested run through it is refused at O(1) — errGateReentrant, counted under storage_write_gate_reentrant_total, a panic under the test binary so a site that swallows the error still fails loudly (§3.4, RC-4). - kind is the caller's (resourceFromKey), no longer hard-coded to containerprofiles; storage_sqlite_write_hold_seconds gains the kind label and the legacy holdPath* constants; storage_sqlite_write_hold_step_seconds for the legacy commit's rename sub-timer (C.2). - a per-gate watchdog samples the hold (storage_write_gate_hold_age_seconds) and logs the holder with every goroutine's stack once a hold outlives gateWatchdogThreshold (PM-G2). - a create that finds the key present counts as a conflict, as the shard commit always did. - WriteGate / NewWriteGate exported for main.go. gatedWrite is the one helper every legacy site will run through: with no gate, byte-identically today's code (bare statements for W3–W9, the savepoint for W1/W2, on the caller's connection); with a gate, BEGIN IMMEDIATE … COMMIT on the gate's connection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…etes through the shared gate (R1b) W2–W8 of write-gate-sharing §3.2, each through StorageImpl.write (the gate==nil branch is today's code, byte for byte): - W2 saveObject(ctx, conn, key, obj, metaOut, checksum, priority, path): the savepoint body (writeMetadata + payload rename) is the gated fn; the rename is INV-1′'s one filesystem exemption and gets a step sub-timer. Labels: legacy_commit from CreateWithConn/GuaranteedUpdateWithConn and HealDivergence, migrate from the three gob-migration rewrites (W6b/W7b/W8). - W3 deleteLocked → deleteLockedGated: DELETE … RETURNING (raw JSON captured) [+ time_series delete] in one gated transaction; the payload Remove and the decode into metaOut after release. Delete takes Lock(key) only under the gate — no pool connection while queued (lockKey is acquireLockedConn's lock step); DeleteWithConn keeps its caller's connection for reads and ignores it for the write. - W4/W5/W6a/W7a: get()'s orphan prune, corrupt-gob delete and the two tool-failure deletes go through repairDelete (priorityLow, "repair"); the caller keeps its read lock and pool connection while queued (§3.3, cold path) and keeps swallowing the error, as today. - Create/GuaranteedUpdate return ErrGateRequiresSingleWriter when a gate is set and singleWriterEnabled is false (after the kind-ownership refusal, so a foreign key is still refused as foreign). deleteMetadataRaw is DeleteMetadata without the in-callback decode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…l connection (R1c) W1 of write-gate-sharing §3.2: with a gate, singleWriter.commit hands the job to commitGated — Lock(key), then one ticket on the submitter's ctx, then on the gate's connection the CAS read (readCurrentResourceVersion, allowed by INV-1′), the INSERT OR REPLACE and the payload rename, in one BEGIN IMMEDIATE … COMMIT. The shard holds no pool connection at all while queued (§3.3's rule for the hot path), and the CAS is atomic against every writer rather than only Lock(key)-respecting ones, which closes the cleanup-vs-shard race of §1.2. A job whose submitter gave up is no longer committed once dequeued. Two shapes on purpose: with no gate the CAS read stays an autocommit SELECT before the savepoint — inside a deferred SAVEPOINT it would become a read-to-write lock upgrade (SQLITE_BUSY_SNAPSHOT, which the busy handler does not retry) and would move the statement golden. The gate's BEGIN IMMEDIATE already holds the lock, so there the read joins the transaction. Lane 0's runOnShard leaf (CP-only, unreachable under the flag) runs with no connection under the gate: the leaf gates its own statements. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

… through the shared gate (R1d) W9a/W9b of write-gate-sharing §3.2: ResourcesCleanupHandler.write routes deleteMetadata (DELETE … RETURNING [+ time_series], decoded after release) and readMetadata's legacy-sidecar WriteJSON through the gate on the tick's ctx (RunCleanupTask's, so shutdown ends the tick with errGateClosed), one ticket per row on the low lane. The walk's one pool connection stays held for its reads across the queue wait — the bounded cold-path relaxation of §3.3 (at most two tickers: the cleanup goroutine and the CP processor's maintenance arm). With no gate, byte-identically today's statements. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

… sentinel (R1e) - T-G2: a gated fn that re-enters the store through the ctx it was handed (a W4 repair on an orphan key) is refused at O(1) on the ctx marker — as a panic under the test binary, recovered by the outer transaction into an error (gateReentrantPanics is armed in TestMain, so a site that swallows the production error still fails). The captured-pre-ticket-ctx variant queues behind its own holder until the ctx bound, and the watchdog is what names it (fired ≥ 1 at a 100 ms test threshold). - T-G5 (PM-G3): 20 readers GET distinct orphaned keys while one goroutine saturates the gate with ContainerProfile creates; every repair queues holding its pool connection and the pool never times out (0 pool-wait timeouts; slowest GET 1.5 ms on this machine). - T-G6 (PM-G7): a legacy commit queued on the high lane and a cleanup tick queued on the low lane when the gate closes both fail with errGateClosed; Close waits for the holder; the pool closes. The tick's rows live in a non-default namespace because CleanupTask drops the default namespace's error (`err = h.cleanupNamespace(...); return nil`), a pre-existing flag-off behaviour left alone here. - ErrGateRequiresSingleWriter is pinned with errors.Is; reads and deletes keep working in the refused topology. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…gate (T-G3); feature doc The backend A/B gains what write-gate-sharing §7 T-G3 asks for: two legacy writers cycling a sbomsyft object at LegacySizeKB (1 MB default) and a small vulnerability manifest through create → update → delete, through the default (DefaultProcessor) StorageImpl exactly as apiserver.go wires it, plus one cleanup tick reclaiming CleanupRows (300) seeded rows, all concurrent with the CP scenario. Under PERF_AB_BACKEND=objectstore both legacy instances and the cleanup handler share the gate. New series: hold-p99-ms/<path> (PM-G1's per-path bound), busy-wait-max-ms (AC-G3), ungated-writes (AC-G1's production counter, a hard row), the legacy classes' latencies, legacy-err-other (hard) and legacy-over-one-sec. PERF_AB_GATE_BUSY0=1 runs the gate's connection with its busy handler off, AC-G3's strong form: any hidden lock contention fails the transaction instead of waiting. Harness version 3. Measured on this machine (pinned production shape, one round each): objectstore arm: hold p99 legacy_commit 2.4 ms, legacy_delete 1.6 ms, cleanup 2.1 ms (gate: < 50 ms); ungated-writes 0; pool-wait timeouts 0; err-other 0; busy-wait p99 0.57 ms with a 25 ms max bucket — and with the busy handler off, 15,122/15,122 BEGIN IMMEDIATE succeeded: the tail is scheduling jitter, not a lock wait. legacy arm (flag off, no gate): legacy create/update p99 109/106 ms, tick p99 3.3 s (4 ticks > 1 s), busy-wait unobserved by construction. Legacy update p99 under the gate (200 ms vs 106 ms) is the wait behind the ObjectStore's consolidation hold (p99 430 ms), PM-G5's inversion across kinds — an ObjectStore hold-length matter, recorded, not this change's. docs/features/write-gate-sharing.md describes the mechanism, the invariant and how to measure it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…by tenure; legacy CP storage refuses a gated StorageImpl Review findings WF-1/WF-2/WF-3 on write-gate sharing: - WF-1: the apiserver's pre-shutdown hook no longer closes the shared gate. Pre-shutdown hooks run before in-flight requests drain, so a closed gate failed every in-flight write of every gated kind with errGateClosed during the shutdown window. The hook keeps objectStore.Close (the checkpointer); main.go closes the gate after cli.Run returns, when no request can arrive. (Pool.Close is never called outside tests, so the K-5 ordering the hook was written for does not apply in production.) - WF-2: writeGate.owned maps each connection to the write-statement sequence at which the gate took it; owns() requires seq > that, so a write recorded on a pool connection before the gate took it is judged ungated (TestWriteGate_OwnsIsBoundedByTenure). - WF-3: ContainerProfileStorageImpl.BeginTransaction and HealDivergence refuse (errCPStorageGated) when the wrapped StorageImpl shares the gate: their pool-connection transactions would make saveObject queue on the gate behind their own lock — W11/W12 are unreachable by construction now, not by convention (TestContainerProfileStorageImpl_RefusesGatedStorageImpl). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…hrough the CP store (§5.6 row 9) The full-spec ContainerProfile list behind generatednetworkpolicies went through the DEFAULT StorageImpl, whose get() deletes the shared metadata row of any CP key with no payload file - under ContainerProfileSqliteBackend, every row the ObjectStore owns (and, guarded, a refusal on every Get). NewGeneratedNetworkPolicyStorage now takes the containerprofiles resource's own storage.Interface for that list; knownservers stay on the default instance. apiserver.go passes containerProfileStorageImpl, which flag-off is the processor-wired legacy CP instance (also the instance whose lock map the CP writer uses - the latent cross-instance RLock of §5.5). Test: TestGNP_ContainerProfileReadsGoThroughTheCPStore fails on the old wiring (ownership refusal on Get) and passes on the new; the spy asserts knownservers are still read from the default querier and no CP list reaches it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…r only; rows, not files, under the flag (K-4, §5.6 row 10) initResourceToKindHandler never maps ContainerProfileKind: main.go's generic relevancy walk starts before the ObjectStore exists and cannot reach it, so the relevancy handlers (deleteMissingInstanceIdAnnotation, deleteMissingWlidAnnotation) move into ContainerProfileProcessor.cleanup(), which already holds the CP storage and runs deleteByTemplateHashOrWlid on the same interval (ResourcesCleanupHandler.ContainerProfileHandlers). Under ContainerProfileSqliteBackend the handler carries the ObjectStore (SetContainerProfileStore, wired in apiserver.go) and the CP arm enumerates the namespace's metadata rows and reclaims through ObjectStore.Delete - one gated transaction over metadata, payloads and time_series, Deleted dispatched after - and never walks CP files. The legacy file-then-row delete would have left the payloads row behind (K-2's orphan shape; the fail-before run of TestCleanup_ContainerProfileArmUsesRowsUnderTheFlag shows it). Tests: TestCleanup_GenericWalkNeverVisitsContainerProfiles (the walk runs, reclaims a deprecated kind, visits no CP path, keeps the CP file and row with relevancy on), TestCleanup_RelevancyHandlersRunFromTheProcessorCleanup (a running workload's profile without instance-id is reclaimed exactly when relevancy is on), TestCleanup_ContainerProfileArmUsesRowsUnderTheFlag (all three tables cleared, INV-2 on the kept row, zero CP file paths, a stray legacy file left alone, Deleted dispatched). All three fail on the previous commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

Review finding: gofmt -l reported these among the branch's changed files (struct-field alignment and function-body wrapping); no logic changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BkhY5wB7cKArz1dMxAoP3c

… filter Review finding on PR #402: git diff --check flagged pkg/registry/file/sqlite.go:406,438 (the is_time_series filter added for the List-exclusion fix). Normalized to pure-tab indentation matching the surrounding ORDER BY/LIMIT lines. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BkhY5wB7cKArz1dMxAoP3c

…t-suite goroutine leak runMaintenanceTasks had no stop mechanism at all -- StartMaintenance launched it with a bare `go` and no context/channel, so any caller that started it (only one test did: TestContainerProfileProcessor_ MaintenanceDoesNotStartUntilExplicit, this session's own new test) left it running forever at its configured interval, for the rest of the test binary's life. At that test's Interval=5ms, the loop kept calling ConsolidateTimeSeries (and so ListTimeSeriesWithData) against the test's pool indefinitely after the test returned. This exactly matched TestWorkBudget's own built-in cross-test contamination detector: "S1_learning_tick: contaminated: stmt:ListTimeSeriesWithData (N late observations)", reproducible only in a full `go test ./pkg/registry/file/ -count=1` run, confirmed independently three times in this investigation (including by PR #402's human reviewer). The same leaked goroutine's contention for the shared write gate also explains the separate, previously-unexplained flake in TestINV4_LegacyStoreRefusesContainerProfileKeys/Delete. Adds StopMaintenance, backed by a stop channel runMaintenanceTasks checks both between iterations and during its sleep (does not interrupt an in-flight cleanup/consolidation pass, only prevents the next one) -- a production process never calls it and exits instead, same as before; it exists so a test that starts the loop can also stop it. TestContainerProfileProcessor_MaintenanceDoesNotStartUntilExplicit now calls it via t.Cleanup. Verified: `go test ./pkg/registry/file/ -count=1` clean across 3 consecutive full-package runs (was reliably contaminated before). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BkhY5wB7cKArz1dMxAoP3c Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

github-actions · 2026-09-11T19:30:52Z

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: failure
Unit test: success
Go linting: failure

matthyx · 2026-09-11T19:31:32Z

Thanks for the thorough review — all 5 confirmed as real, fixed with regression tests that fail before / pass after each fix:

Rollback resurrection (sqliteobject_export.go) — e0661eb50. ExportContainerProfiles now removes stale legacy files with no metadata row (still decodable → removed; undecodable → left as-is, matching the main loop's existing behavior). TestExport_RemovesStaleFileOfKeyDeletedUnderTheNewStore, TestExport_DryRunLeavesStaleFiles.
Panic mid-staging commits partial work (sqliteobject_cpstorage.go) — 1cce87a05. The transaction finalizer now detects an in-progress panic (not just *errp) and discards the write set before re-panicking. TestObjectStore_ConsolidationPanicMidStaging_DoesNotCommitPartialWork, injecting a panic at BeforeProcessedDeletes.
Maintenance races cleanup wiring — e4221cb30. Split SetStorage (no side effect) from an explicit StartMaintenance(), called only after apiserver.go finishes wiring CleanupHandler.SetContainerProfileStore. -race-clean TestContainerProfileProcessor_MaintenanceDoesNotStartUntilExplicit.
Missing payload row truncates full-spec LIST — 1ac4a0ffb. Switched the join to LEFT JOIN; pagination count/continuation now reflect metadata rows scanned, not successfully joined rows. TestObjectStore_ListFullSpec_MissingPayloadRowDoesNotHideLaterRows, reproducing your exact scenario.
Filesystem errors silently mark an incomplete sweep done — 28e5654b2. Both the top-level DirExists stat error and per-entry afero.Walk callback errors now propagate instead of being swallowed. TestMigration_SweepWalkErrorDoesNotMarkDone, TestMigration_SweepTopLevelStatErrorDoesNotMarkDone.

Also addressed: gofmt/whitespace (bd52533fa, 2f249d2e3 — scoped to just this branch's changed files, confirmed nothing else in the repo's pre-existing formatting debt got touched), CodeQL's 4 integer-truncation findings (72112c737), and DCO (all commits now signed off).

On your two observations:

The isolated full-suite pkg/registry/file failure was real and is now fixed (c8f7377bd) — root cause was actually introduced by this review round itself: StartMaintenance() (finding feat: add filtered SBOMs #3's fix) had no stop mechanism, so the one test exercising it leaked a background goroutine for the rest of the test binary's life, corrupting TestWorkBudget's isolated measurement (and, it turned out, explaining a second previously-unexplained flake in TestINV4_LegacyStoreRefusesContainerProfileKeys/Delete too). Added a StopMaintenance() + stop channel; verified clean across 3+ consecutive full-package runs.
cmd/cpexport's build failure was confirmed environment-specific, not a bug — it needs a writable temp dir / Go build cache for its nested go build, which your sandbox's read-only mount didn't have. Passes cleanly here.

CI is now fully green (CodeQL, DCO, cross-platform build, Basic-Test). Ready for another look whenever you have time.

matthyx

Rechecked force-updated head c8f7377bd0367fda6034fb1de3bba91984e1e5ba against unchanged main base 6386bf23b2b37501504297839bc177a8dcde1a8b, focusing on the 15-file tree delta from my prior review at 32129967c4ac42b84102fcc7d53294bedd312a09.

The five previous blockers are fixed with relevant regression coverage: panic-time staging is discarded; missing-payload LIST pagination advances correctly; migration stat/walk errors prevent completion; cleanup ownership is wired before maintenance; and stale legacy files are reconciled in the ordinary rollback case. CodeQL's four threads are resolved, DCO/CodeQL/current CI are green, and formatting checks are clean.

Two HIGH blockers remain in the new export reconciliation:

A trailing slash in -root deletes live exported files (pkg/registry/file/sqliteobject_export.go:203; CLI input at cmd/cpexport/main.go:32,55). filepath.Join cleans /data/ to paths under /data/..., but the code slices those paths using len(root). The derived key therefore lacks its required leading /; K8sPathToKeys shifts kind/namespace/name and ReadMetadata misses the live row. decodeLegacyFileAt still opens the correct file, so lines 225–229 classify and delete that just-exported live file. After downgrade, legacy GET sees the missing payload and removes the live metadata row. Derive the key with filepath.Rel from a normalized root, restore the storage-key leading slash, and test trailing-slash/noncanonical roots with live and deleted objects.
Filesystem failures can still produce a successful but unsafe rollback export (pkg/registry/file/sqliteobject_export.go:184,209). The stale-file pass discards DirExists errors and treats every decode-helper error—including permission/I/O errors opening a valid stale file—as harmless undecodable content. The CLI exits successfully without removing or reporting that file; once readable after downgrade, a previously deleted object is served again. Propagate filesystem access errors separately from genuine content-decoding failures and add export equivalents of the migration stat/open failure regressions.

Validation: the targeted tests for all five prior fixes passed; the same recheck under -race passed in 68.364s; targeted go vet and git diff --check passed; current GitHub CodeQL, DCO, cross-platform build, and Basic-Test checks pass. A broader isolated pkg/registry/file run still exited unsuccessfully after a large goroutine dump, while cmd/cpexport passed with GOFLAGS=-buildvcs=false; a filtered rerun did not yield an actionable named test failure. I did not rerun the external performance harness.

Architectural status: BLOCK. Verdict remains request changes. GitHub still marks this PR as draft, so it is not approvable regardless.

… live exported files Round-2 review finding #1: reconcileStaleExportedFiles derives a key by slicing a Walk()-reported path (built from filepath.Join, which always cleans) at len(root). An uncleaned root with a trailing slash (e.g. the CLI's -root /data/) makes that length one too many, silently dropping the key's required leading '/'. ReadMetadata then misses the live row for a just-exported object, and the stale-file pass deletes its file -- after downgrade, the old binary's self-repair deletes the now-missing live metadata row too. Fixed at the single source: root is cleaned once at the top of ExportContainerProfiles, before it is used anywhere for length-based key derivation, so the fix covers reconcileStaleExportedFiles without touching its own logic. The same length-slicing pattern exists in sweepFiles (MigrateContainerProfiles' file sweep); cleaned there too for consistency, though production always passes the already-clean DefaultStorageRoot, so the risk was lower. TestExport_TrailingSlashRootDoesNotDeleteLiveFiles: seeds one live and one genuinely-deleted-under-the-new-backend object, exports with a trailing-slash root, confirms the live file survives (and stays readable by an old binary) while the deleted one's stale file is still correctly removed. Red before the fix (all 4 seeded files wrongly deleted, not just the 1 genuinely stale one), green after. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd

…t fail the export Round-2 review finding #2: reconcileStaleExportedFiles discarded the top-level DirExists error entirely (afero.DirExists returns exists=false on ANY stat error, not just "doesn't exist" -- the same class of bug round-1 finding #5 fixed in the migration sweep), and treated every decodeLegacyFileAt failure identically to genuinely undecodable content. A permission or I/O error opening a stale candidate's file is neither: the tool couldn't tell whether that file is safe to leave (an old binary might still resurrect it after downgrade), yet the export reported success and moved on. Adds errLegacyFileAccess, a sentinel wrapped around decodeLegacyFileAt's fs.Open failure specifically (not its decode failures, which stay "leave it" as documented -- an old binary can't resurrect content it can't decode either). reconcileStaleExportedFiles now propagates both the top-level stat error and an errors.Is(derr, errLegacyFileAccess) match as a hard export failure, leaving content-decode failures untouched. TestExport_ReconcileTopLevelStatErrorFailsExport and TestExport_ReconcileFileAccessErrorFailsExport (a new failOpenFs fault-injector, alongside migration_test.go's existing toggleFailFs) both red before the fix (silent success), green after. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd

… instead of racing it Root-caused the CI failure round-2 review flagged as "a large goroutine dump, no actionable named test failure" -- reproduced directly on GitHub's own runner (not locally, where it never fired across 27 sequential full-package runs): TestCleanup_ContainerProfileArmUsesRowsUnderTheFlag checked w.ResultChan() with a non-blocking select{default: t.Fatal}, racing the WatchDispatcher's own async delivery goroutine. On a quiet machine the event is reliably already queued by the time the select runs; under a busier CI runner's scheduling, it may simply not have been dispatched yet, so a real, working delete is on the wire. The round-1 StartMaintenance leak (c8f7377) is a separate, already- fixed bug -- confirmed 100% reproducible before that fix and 100% clean after, across many runs both locally and by a prior investigation in this session. This is a distinct flake, a timing race rather than a leak, in a different test the leak fix never touched. Bounded the wait to 5s (matches this session's other async-dispatch test patterns) instead of racing the dispatcher. 20/20 stress runs clean; full package suite clean. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd

github-actions · 2026-09-11T20:37:11Z

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: failure
Unit test: success
Go linting: failure

matthyx · 2026-09-11T20:37:34Z

Both new findings confirmed and fixed:

Trailing-slash root deletes live files — 408fcc0c7. Fixed at the single source: root is now filepath.Cleaned once at the top of ExportContainerProfiles, before any length-based key derivation. Same pattern applied to sweepFiles in the migration code for consistency (lower risk there since production always passes the already-clean DefaultStorageRoot, but same bug class). TestExport_TrailingSlashRootDoesNotDeleteLiveFiles: seeds one live + one genuinely-deleted object, exports with a trailing-slash root, confirms the live file survives while the stale one is still correctly removed. Red before (all 4 seeded files wrongly deleted) → green after.
Filesystem errors swallowed during stale-file reconciliation — 0cedf4311. Filesystem access errors (stat/open failures) now propagate distinctly from genuine content-decode failures, mirroring the pattern from round 1's migration-sweep fix. TestExport_ReconcileTopLevelStatErrorFailsExport, TestExport_ReconcileFileAccessErrorFailsExport — both red before (silent success) → green after.

On the goroutine dump: root-caused as a different bug from round 1's leak — 4ad4bb499. Reproduced directly on GitHub's own runner (never fired across 27 sequential full-package runs locally): TestCleanup_ContainerProfileArmUsesRowsUnderTheFlag checked w.ResultChan() with a non-blocking select{default: t.Fatal}, racing the WatchDispatcher's own async delivery goroutine — a timing race, not a leak, in a different test the round-1 fix never touched. Bounded the wait to 5s instead of racing the dispatcher; 20/20 stress runs clean.

CI is fully green again (CodeQL, DCO, cross-platform build, Basic-Test). Ready for another pass whenever you have time.

matthyx · 2026-09-11T20:37:38Z

Both confirmed and fixed:

Trailing-slash root (408fcc0c7) — you're exactly right on the mechanism: filepath.Join cleans the trailing slash but len(root) still counted it, dropping the key's leading /. Fixed at the single source: root is cleaned once at the top of ExportContainerProfiles, before any length-based slicing happens. Also found and fixed the identical pattern in the migration sweep (MigrateContainerProfiles/sweepFiles) for consistency — production always passes the already-clean DefaultStorageRoot there so the risk was lower, but same bug shape. TestExport_TrailingSlashRootDoesNotDeleteLiveFiles: red before (wrongly deleted all 4 seeded files, not just the 1 genuinely stale one), green after.
Swallowed filesystem errors (0cedf4311) — added errLegacyFileAccess, a sentinel wrapped specifically around decodeLegacyFileAt's fs.Open failure, so a real access/I/O error can be told apart from the file's content genuinely being undecodable (which correctly stays "leave it," matching the documented contract — an old binary can't resurrect content it can't decode either). Both the top-level DirExists stat error and an errLegacyFileAccess match now fail the export instead of silently succeeding. Two new tests (TestExport_ReconcileTopLevelStatErrorFailsExport, TestExport_ReconcileFileAccessErrorFailsExport), both red before / green after, using a new failOpenFs fault-injector alongside the existing toggleFailFs.

On the goroutine dump: I didn't just declare it stale. 27 local sequential full-package runs (26 clean, 1 I couldn't catch/characterize) didn't reproduce it reliably — but the actual CI run right after pushing the two fixes above DID fail, this time with a concrete named test: TestCleanup_ContainerProfileArmUsesRowsUnderTheFlag. Root cause: a real race between WatchDispatcher's async event delivery and a non-blocking select{default: t.Fatal} checking for it immediately after cleanup() returns — reliable on a quiet machine, not guaranteed under a busier CI runner's scheduling. This is a separate bug from round-1's StartMaintenance leak (already confirmed fixed, 100%-reproducible before/100%-clean after), not a resurgence of it — different test, different mechanism (a timing race, not a leak). Fixed with a bounded 5s wait instead of racing the dispatcher (4ad4bb499); 20/20 stress runs clean, and this time CI itself (Create cross-platform build) is green on the actual fix, not just my local runs.

All checks green: CodeQL, DCO, Basic-Test, Create cross-platform build.

matthyx

Rechecked head 4ad4bb4992e091916436389a95dcb2074ae6f490 against unchanged main base 6386bf23b2b37501504297839bc177a8dcde1a8b, focusing on the four-file delta from my prior review at c8f7377bd0367fda6034fb1de3bba91984e1e5ba.

The new commits correctly handle the reported /data/ root, propagate directory-stat and file-open failures, and replace the racy cleanup assertion with a bounded wait. The submitted regressions pass, and current DCO, CodeQL, Basic-Test, cross-platform build, GitGuardian, and CodeRabbit checks are green. However, both underlying export failure classes remain incomplete:

HIGH — accepted roots / and . still misclassify exported files (pkg/registry/file/sqliteobject_export.go:100,220; the same conversion remains at pkg/registry/file/sqliteobject_migration.go:690). filepath.Clean fixes /data/, but deriving the logical key by slicing path at len(root) assumes Walk reports the same textual prefix. For root /, the slice removes the key's required leading slash; metadata lookup misses and reconciliation can delete a live file that was just exported. For root ., Walk reports a relative path without ./, so the slice drops the first character; stale files are silently missed and can resurrect deleted profiles after downgrade. Both roots are accepted by cpexport. Use filepath.Rel(root, path), strip .g, and explicitly construct the slash-prefixed storage key in both walkers. Add live/stale regressions for /data, /data/, ., and /.
HIGH — Open succeeding followed by Read failing is still reported as a successful export (pkg/registry/file/sqliteobject_migration.go:524-549; suppression at pkg/registry/file/sqliteobject_export.go:226-235). errLegacyFileAccess wraps only fs.Open. If the file opens but its reader returns an I/O error, gob decoding returns an untagged error and reconciliation treats it as malformed content, leaves the stale file, and exits successfully. Once readable after downgrade, that deleted object can be served again. Preserve non-EOF reader failures as filesystem-access errors (and classify migration-tool execution failures deliberately), then add an export regression whose Open succeeds and Read fails.

Validation: the focused export/stat/open/cleanup tests passed; an isolated targeted -race run covering those tests plus stale-file deletion passed (ok .../pkg/registry/file 1.485s); targeted go vet and diff/format checks passed in the independent review lanes. These tests exercise the repaired examples, not the remaining /, ., and post-Open read-error cases. I did not rerun the external performance harness; its current check is skipped.

Related-history and necessity conclusions are unchanged from the prior reviews: the full-ACID backend addresses the real row/file tear tracked by merged #366 and improves on the incomplete split-commit design in closed draft #368; it is not superseded by another identified change.

Architectural status: BLOCK. Verdict: request changes. The PR is also still a draft and therefore not approvable.

…classification Two HIGH findings, both in the same reconciliation code touched by rounds 1-2 (reconcileStaleExportedFiles / sweepFiles / decodeLegacyFileAt), fixed together since their production changes land in the same functions. Finding #1 -- accepted roots "/" and "." still misclassify exported files: round 2's filepath.Clean(root) fixed the trailing-slash case, but the underlying approach -- slicing a Walk()-reported path at len(root) -- was never correct in general: root "/" overcounts the slice by one (drops the key's required leading '/'), root "." undercounts differently (Walk reports paths without a "./" prefix). Clean cannot fix either, since they're not about extra/missing separators, they're about root's byte length simply not matching what filepath.Join actually consumed. Replaced with a new keyFromPayloadPath(root, path) helper using filepath.Rel instead of byte-length slicing -- exact for every root shape, used in both reconcileStaleExportedFiles and sweepFiles. TestKeyFromPayloadPath_RoundTripsForEveryRootShape: table-driven, round-trips every key through makePayloadPath -> Walk-shaped path -> keyFromPayloadPath across 6 root shapes including both "/" and "." named in the review, plus "/data/", ".." navigation and a relative non-dot root -- proving the general fix against the whole boundary, not just the two reported cases. Finding #2 -- Open succeeding then Read failing is still reported as a successful export: errLegacyFileAccess wrapped only fs.Open's own error. A file that opens fine but fails mid-Read (a bad sector, a device/mount that opens but errors on read) surfaced only as an opaque, untagged error out of gob's Decode -- indistinguishable from the file's content genuinely being malformed, so reconcileStaleExportedFiles left it in place and reported success; after downgrade, an old binary reading that file could resurrect a deleted object. decodeLegacyFileAt now reads the whole file into memory (io.ReadAll) before decoding instead of decoding straight off the open handle, so a Read error is caught explicitly and classified with errLegacyFileAccess, same as an Open failure. The external migration tool's own exec/read failure gets the same treatment for the same reason. TestExport_ReconcileOpenSucceedsReadFailsFailsExport (new failReadFs test double: Open succeeds, Read always errors) -- red before this fix (silent export success), green after. Verification: go build clean; gofmt clean on changed files; git diff --check clean; full pkg/registry/file suite green across 9 runs (one unrelated rare flake observed in an unmodified area -- a HealDivergence write-gate-refusal error, no --FAIL line, 1/9 runs, in code this commit does not touch -- flagged, not chased, out of scope for this fix); -race clean on all TestExport_*/TestKeyFromPayloadPath_* tests (61.7s). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

github-actions · 2026-09-11T21:36:17Z

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: failure
Unit test: success
Go linting: failure

matthyx · 2026-09-11T21:36:46Z

Both findings confirmed and fixed generally this time, not just for the reported inputs:

Key derivation for any root shape — new shared helper keyFromPayloadPath(root, path) (pkg/registry/file/storage.go) replaces the len(root)-slicing in both reconcileStaleExportedFiles and sweepFiles, using filepath.Rel instead of a textual-prefix assumption. TestKeyFromPayloadPath_RoundTripsForEveryRootShape: table-driven, round-trips through 6 root shapes (/data, /data/, ., /, /data/../data, a bare relative data) × 2 keys, confirming exact key recovery in every case — not just the / and . cases named in the review.
I/O errors during Read, not just Open — decodeLegacyFileAt now reads the full file via io.ReadAll before decoding, so a Read failure after a successful Open gets the same errLegacyFileAccess classification an Open failure already had; extended to the external migration tool's exec/read path per your explicit ask. TestExport_ReconcileOpenSucceedsReadFailsFailsExport uses a fake fs/file whose Open succeeds and Read always errors — red before (silent success) → green after.

Grepped both files for the same two patterns (len(root)/len(m.root), bare .Open() beyond the named line numbers — found no other instances; the bug class is closed in these two files.

One thing worth flagging rather than burying: a full-suite run turned up a rare (1/9), unrelated flake in HealDivergence/write-gate-refusal code — neither file this fix touches. Didn't chase it since it's out of scope for this round; happy to open a separate investigation if you'd like, otherwise treating it as pre-existing background flakiness for now.

CI is green (DCO, cross-platform build; CodeQL/Basic-Test unaffected by this diff). Ready for another look.

matthyx

Rechecked head 8aa3e34ac7baa54a8e990afde98491389ab4fdcb against unchanged main base 6386bf23b2b37501504297839bc177a8dcde1a8b, focusing on the five-file delta from my prior review at 4ad4bb4992e091916436389a95dcb2074ae6f490.

Both remaining HIGH findings are resolved:

keyFromPayloadPath now derives the logical key with filepath.Rel, strips the payload suffix, and restores the leading slash. Export and migration both use the shared helper, and the table test covers /, ., trailing-slash, relative, and normalized-parent root shapes.
decodeLegacyFileAt now distinguishes a post-Open Read failure before gob decoding and tags it as errLegacyFileAccess; export propagates that category. Migration-tool execution failures are likewise classified conservatively rather than allowing export to report success without inspecting the stale candidate.

No new correctness, security, or architectural blocker was found in this delta. The temporary whole-file allocation introduced by io.ReadAll is proportional to one payload and is a performance/resource tradeoff, but processing is sequential, the migration/export scale tests pass, and no material workload regression was demonstrated.

Validation: an isolated targeted -race run covering root conversion, stale rollback deletion, stat/open/read failures, and trailing-slash handling passed (ok .../pkg/registry/file 1.533s). The independent lanes also passed the broader export/migration/key suite (including 5,000-row scale cases), full cmd/cpexport tests, targeted go vet, formatting, and diff-whitespace checks. Current DCO, CodeQL, Basic-Test, cross-platform build, GitGuardian, and CodeRabbit checks are green; the external performance check is skipped, and I did not rerun that harness. The author also reported a rare full-suite flake outside this five-file delta; there is no evidence tying it to this fix, so it is not a blocker for this recheck.

Related-history and necessity conclusions are unchanged: the full-ACID backend addresses the real row/file tear tracked by merged #366 and improves on the incomplete split-commit design in closed draft #368; no superseding change was identified within the previously reported search limits.

Independent results: code-reviewer APPROVE, architectural status CLEAR. No code changes are requested from this recheck. I am recording this as a comment, not a formal approval, because the PR remains a draft (and GitHub does not permit an author to approve their own PR). Mark it ready for review before the final approval gate; any subsequent head change requires a fresh check.

…staticcheck, unused) 13 findings from CI's --new-from-patch run on this PR's diff: - errcheck: peakRSS's deferred f.Close() error now discarded explicitly - govet (x3): reflect.Ptr -> reflect.Pointer (the modern non-deprecated name) - staticcheck QF1005 (x2): math.Pow(x, 2) -> x*x in the Tier B N' calculation - staticcheck QF1001: De Morgan's law applied to an inv2 rapid-test crash check - staticcheck QF1008 (x4): embedded-field selectors simplified (Time, LabelSelector) - unused (x2): sortedKeys and touchedTables, dead test helpers with no callers (and their now-unused sort/strings imports) Verified with the exact CI invocation (golangci-lint v2.13.2, --new-from-patch against origin/main): 0 issues, down from 13. Build, gofmt, git diff --check and the full pkg/registry/file suite all clean. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd

github-actions · 2026-09-14T06:40:46Z

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: failure
Unit test: success
Go linting: success

…ntainerProfile data Turning ContainerProfileSqliteBackend off after real usage (a config edit, no binary change) used to let the legacy StorageImpl's get() self-repair silently and irreversibly delete any ContainerProfile the ObjectStore had created or updated since the flip, whenever its legacy .g file was missing -- the object destroyed, not merely invisible, with zero confirmation and no cpexport step forced first. This went through 5 rounds of adversarial architect+critic review (.omc/plans/rollback-safety-guard.md) before reaching a verified-sound design; three earlier approaches were rejected in turn for resurrecting deleted objects, serving stale content after a crash, or corrupting data via the downstream consolidation pass. The final mechanism: - get()'s missing-file branch now serves the object from its payloads BLOB instead of deleting the row, when all four hold: the metadata row exists, rv IS NOT NULL (the row is ObjectStore/migration-owned -- every legacy write nulls both rv and uid via INSERT OR REPLACE, so this excludes anything a legacy write has touched since the flip, including a .g file whose rename was lost to a crash), is_time_series = 0 (TS rows are out of scope, unchanged behavior), and a payloads row exists. ResourceVersion/UID are stamped from the metadata row's columns, the same conversions cpexport already uses. Any condition failing, or the payloads body itself failing to decode, falls through to today's existing self-repair, unchanged. The 3 undecodable-.g-file repairDelete sites are deliberately out of scope -- no safe way to prefer a payloads body over a possibly-fresher corrupt file without decoding and comparing versions. - Both places that remove a ContainerProfile metadata row (deleteLocked's explicit delete, and repairDelete, the shared self-repair reached from all 4 get() sites) now also delete the payloads row via a new DeletePayloads helper, closing the orphan-payload leak that used to make every Create of a deleted-then-recreated key fail on the UNIQUE constraint. deleteLocked orders the payloads delete before the metadata delete so a crash mid-sequence fails safe into the existing, already-correct self-repair shape rather than creating a new orphan. - A non-Fatal advisory startup census logs how many keys are currently being served via the fallback when the flag is off -- purely informational, bounded by its own context.WithTimeout (not the untimed signal context), never blocks startup on a query error. - Create's existence check needed no change: in the default singleWriterEnabled=true configuration, the single-writer commit path's own recheck already covers the case correctly. Tests: 19 new tests covering the exact adversarial scenarios that sank earlier design iterations (a genuinely deleted key must stay NotFound; a pre-existing orphan payloads row must never be served; a legacy-touched row with a lost file rename must never serve stale content; time-series rows and the 3 undecodable-file sites must be excluded; the first write to a fallback-served key must succeed without a CAS conflict, proving the rv-column/metadata-JSON invariant the fallback depends on; a real consolidation tick against a fallback-eligible key must not trigger a divergence-heal false positive). Four pre-existing tests that pinned the old destructive behavior by name are updated to assert the new, strictly safer outcome (more assertions than before, not fewer). Verified: full pkg/registry/file suite and the new tests under -race, twice independently (once per test author, once by an architect verification pass with fresh evidence), plus a docs delta to docs/features/containerprofile-sqlite-backend.md. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd

github-actions · 2026-09-14T10:24:16Z

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: failure
Unit test: success
Go linting: failure

matthyx · 2026-09-14T10:54:12Z

One new commit on top of your round-4 recheck (8aa3e34a → 2942a88f), and I'm marking this ready for review now.

What it is: a separate hardening pass, not a response to a review finding. While walking through the operational implications of this backend with the user, we identified that simply turning ContainerProfileSqliteBackend back off (a config edit, no binary change) had no protection at all — the legacy StorageImpl's get() self-repair would silently and irreversibly delete any ContainerProfile the ObjectStore had created or updated since the flip, the moment anything tried to read it. No cpexport step was forced, no confirmation, nothing — just data loss on the next read.

We ran this through 5 rounds of the same adversarial architect+critic review process this PR itself has been through (design doc at .omc/plans/rollback-safety-guard.md, not part of this diff). Three earlier approaches were rejected in turn for real bugs the review process found: resurrecting genuinely-deleted objects, serving stale content after a crash left a .g file rename lost, and — the most serious one — silently corrupting data via the downstream consolidation pass (a NotFound payload read with a still-present metadata row drove the consolidation loop to synthesize an empty profile and persist it as authoritative, permanently, via the frozen-completed-profile gate).

Final mechanism (commit message on 2942a88f has the full writeup):

get()'s missing-file branch now serves the object from its payloads BLOB instead of deleting the row, gated on 4 conditions: the metadata row exists, rv IS NOT NULL (excludes anything a legacy write has touched since the flip — every legacy write nulls rv/uid via its INSERT OR REPLACE), is_time_series = 0 (TS rows explicitly out of scope), and a payloads row exists. ResourceVersion/UID are stamped from the metadata row's columns, same conversions cpexport already uses.
Both places that delete a CP metadata row now also clean up its payloads row (a new DeletePayloads, ordered before the metadata delete on the explicit-delete path for crash-safety), closing an orphan-payload leak that used to make a deleted-then-recreated key permanently fail the UNIQUE constraint.
A non-Fatal advisory startup log reports how many keys are currently being served via the fallback when the flag is off.

Explicitly out of scope, named rather than hidden: the 3 undecodable-.g-file self-repair sites still behave as before (no safe way to prefer the payloads body over a possibly-newer corrupt file without decoding and comparing versions); time-series rows are excluded pending a separate look; a .g file that's present but stale relative to its payload remains undetected (a pre-existing gap, not introduced or fixed here). None of these regress anything — they're the same risk surface this PR already had, just not the specific thing this commit closes.

19 new tests, including the exact adversarial scenarios that sank the earlier design iterations (deleted-key resurrection, lost-rename staleness, TS/undecodable-site exclusion, the rv-column vs. metadata-JSON invariant the fallback depends on, and a real consolidation-tick test proving no divergence-heal false positive). Verified independently twice — once during implementation, once by a full architect verification pass with fresh evidence (build, vet, git diff --check, and the whole pkg/registry/file suite under -race, 256s). CI is green (CodeQL, DCO, Basic-Test, cross-platform build).

🤖 Generated with Claude Code

https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd

matthyx

Rechecked rollback head 2942a88f3c45a31365feb5d0b80330f97c8c2c5d against unchanged main base 6386bf23b2b37501504297839bc177a8dcde1a8b, focusing on the 20-file delta from my prior review at 8aa3e34ac7baa54a8e990afde98491389ab4fdcb.

Protecting a same-binary flag rollback is needed, and the normal missing-file fallback is coherent: the joined candidate, rv IS NOT NULL guard, payload decode, and RV/UID stamping preserve an ordinary ObjectStore-owned profile. However, the new hybrid state is not safe at several legacy decision boundaries:

HIGH — fallback inspection failures trigger permanent self-repair deletion (pkg/registry/file/storage.go:1024-1032,1146-1178,1195-1200). serveFromPayloadsFallback returns the same false for query/decode failures and for positively ineligible records. get() then calls repairDelete, which now deletes both metadata and payloads and returns NotFound. A transient candidate-query failure, unsupported encoding, or conversion/decode failure can therefore destroy the only valid SQL copy. The new unsupported-encoding test passes precisely because it asserts that deletion. Return a tri-state or (served, error), propagate inspection errors without mutation, and add a query-failure regression that verifies both rows survive.
HIGH — shared self-repair now destroys native payloads at the explicitly deferred sites (pkg/registry/file/storage.go:1105-1108,1163-1165,1195-1200,1240-1242,1397-1404). Moving DeletePayloads into shared repairDelete also affects gob EOF, both migration-tool-failure paths, and excluded time-series rows. A corrupt legacy .g file beside a valid native payload—or a native TS object that intentionally has no file—now loses the previously retained SQL body merely because it was read. This contradicts the documentation claiming those sites remain unchanged and leave recoverable payload orphans (docs/features/containerprofile-sqlite-backend.md:68,111-114,126-129). Separate explicit object deletion from read-repair cleanup; an unsupported object may remain unserved without erasing its recoverable bytes.
HIGH — supported singleWriterEnabled=false lets Create overwrite a fallback-visible object (main.go:78; pkg/registry/file/storage.go:630-650,676-679,708; replacement at pkg/registry/file/sqlite.go:704-713). With the backend flag off, this configuration is permitted. A metadata+payload object with no .g file is now visible through fallback, but CreateWithConn checks only file existence, proceeds, and replaces its metadata instead of returning AlreadyExists. Make the non-single-writer path check the legitimate database-backed state while holding the existing key lock, and test both configurations.
MEDIUM — a payload-row deletion failure is logged but Delete continues and reports success (pkg/registry/file/storage.go:815-835). If DeletePayloads fails, metadata and the file are still removed, leaving an orphan while the caller sees success. Propagate the first failure and cover the real Delete path with fault injection; the current ordering test invokes DeletePayloads directly and does not verify this behavior.

Nonblocking operational gaps should also be documented accurately: flag-off cleanup walks files, so live fallback-only records are not considered; the startup census counts database eligibility without checking whether the file is absent, so it is not literally a count of records currently served by fallback; and existing stale files remain outside the guard.

Validation: an isolated targeted -race run of the new rollback/Get/Delete/census tests passed (ok .../pkg/registry/file 3.147s). Independent lanes passed broader rollback, migration/export, INV-4, work-budget, and cmd/cpexport tests plus targeted go vet, formatting, and diff-whitespace checks. Those passing tests confirm the happy path, but several explicitly encode the destructive behavior above. I did not rerun the external performance harness. GitHub has both a successful and a failed cross-platform run for this exact head; the latest failure ends with pkg/registry/file failing after a large goroutine dump without identifying a failing test, and Basic-Test is consequently skipped. I could not attribute that flaky run to this delta, so it is not the evidence basis for the blockers above.

Related-history and necessity conclusions are unchanged: the full-ACID backend addresses the row/file tear tracked by merged #366 and improves on the incomplete split-commit design in closed draft #368; no superseding change was identified within the previously reported search limits.

Code-reviewer recommendation: REQUEST CHANGES. Architectural status: BLOCK. Verdict: request changes. I am recording the verdict as a review comment because GitHub does not permit an author to formally request changes on their own PR.

Address all four findings from PR #402 round-5 review: - Separate fallback eligibility from inspection errors. Propagate query and decode failures without deleting either row, including IgnoreNotFound and GuaranteedUpdate callers that must not synthesize an empty replacement. - Restore metadata-only shared read repair. Prune payloads only at the missing-file site for a positively identified, non-time-series legacy row; preserve corrupt-file and time-series recovery bytes, even with NULL rv. - Check SQLite fallback state under the existing key lock before a legacy Create can replace metadata with singleWriterEnabled=false. - Return payload deletion failures before removing metadata or files, so explicit Delete can be retried without losing the remaining object. Add fault-injection regressions that retain live rows, compare their bytes, restore query access, and verify recovery. Cover both writer configurations, update callback suppression, time-series RV states, and Delete retry. The new regressions fail against 2942a88 using a Go source overlay. Correct the operational documentation: the census counts database candidates, flag-off cleanup misses live fallback-only objects, and stale files remain outside the guard. Simplify the fallback result contract and keep census runtime behavior unchanged. Validation: go build ./...; full go test ./pkg/registry/file/... -count=1; all 26 rollback test functions with -race; targeted go vet; golangci-lint 2.13.2 on the patch (0 issues); gofmt and git diff --check. Repository-wide go vet reports pre-existing unkeyed StackFrame literals in untouched callstack tests; go vet -composites=false ./... passes. The first full-suite run hit TestACG2_FlagOff/GetSbom/corrupt's timing floor (659ms vs 700ms); the subsequent full-suite run passed (44.220s). Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCMGT6Po2tSr1VEDVrbbYd

matthyx · 2026-09-14T12:26:44Z

All four round-5 findings confirmed and fixed in e513ec56 (on top of 2942a88f):

Inspection failures are errors, not ineligibility or absence. Replaced the boolean fallback result with an explicit outcome plus error. Candidate-query, metadata-query, decode, and conversion failures return an error without deleting either row. IgnoreNotFound does not suppress these errors. This also prevents GuaranteedUpdate from treating unreadable SQL data as an empty object and replacing it. Regressions use SQLite authorizers to fail reads while retaining both live rows, compare metadata and payload bytes afterward, and verify recovery once the fault is removed. Invalid JSON/unsupported encoding and update-callback suppression are covered, including both writer configurations.
Shared read repair is metadata-only again. repairDelete no longer calls DeletePayloads. The separate payload-pruning helper has one caller: the missing-file branch, after positively establishing a non-time-series row with rv IS NULL. Gob EOF and both migration-tool failure paths retain their SQL recovery bytes. Time-series exclusion is checked before the NULL-RV pruning decision, with regressions for both RV states.
Create checks database state with the single writer disabled. CreateWithConn checks the fallback database predicate while holding the existing per-key lock, returning AlreadyExists before writing a file or replacing metadata. Query failures also stop Create. Tests cover both supported writer configurations, preservation of the existing content, and ineligible-state controls. The default single-writer path retains its commit-time database recheck.
Delete propagates payload cleanup failures and preserves retry state. deleteLocked returns a DeletePayloads failure immediately, before deleting metadata or the legacy file. Fault injection through the real StorageImpl.Delete verifies the error, preservation of both rows, and successful deletion after removing the fault.

The operational notes are corrected too: flag-off cleanup does not visit live fallback-only records; the startup census counts database candidates, without checking file absence or decoding; existing stale files remain outside the guard. Census runtime behavior is unchanged.

Verification: independent source review found no remaining blocker. New regressions fail against 2942a88f using a Go source overlay and pass with the fix. go build ./..., the full go test ./pkg/registry/file/... -count=1 suite, all 26 rollback test functions under -race, targeted go vet ./pkg/registry/file, golangci-lint 2.13.2 on the patch (0 issues), gofmt and git diff --check pass.

Two qualifications, kept explicit:

go vet ./... reports existing unkeyed StackFrame literals in untouched callstack/callstack_test.go; go vet -composites=false ./... passes.
The first full-suite run failed TestACG2_FlagOff/GetSbom/corrupt at 659 ms against its existing 700 ms minimum. The quiet full-suite rerun passed (44.220 s). Source review confirms both versions retain one busy-timeout wait, but the observed timing failure's cause is not established; a focused baseline run also passed 10/10. I have not changed that timing test or claimed the flake is fixed.

Pushed with a regular push and the requested commit trailers. Ready for another look.

matthyx

Rechecked exact head e513ec560d37821572669c55a1c36463b776d1be against unchanged main base 6386bf23b2b37501504297839bc177a8dcde1a8b, focusing on the six-file delta from my prior review at 2942a88f3c45a31365feb5d0b80330f97c8c2c5d.

All four prior findings are resolved:

Fallback metadata-query, joined candidate-query, decode, and object-conversion failures now return errors without deleting either row. IgnoreNotFound does not collapse those errors to absence, and update callbacks do not run against unreadable SQL state.
Shared repairDelete is metadata-only again. Payload pruning is limited to the missing-file path after positively identifying an rv IS NULL, non-time-series row; corrupt-file, migration-tool-failure, and time-series paths retain recovery payload bytes.
With singleWriterEnabled=false, Create checks fallback-visible database state while holding the key lock, returns AlreadyExists, and fails closed on inspection errors. The existing single-writer commit-time metadata recheck remains intact.
A DeletePayloads failure now returns before metadata or filesystem mutation, preserving both rows for a successful retry.

The new regressions exercise public Get/Create/GuaranteedUpdate/Delete paths, inject SQLite read/delete failures, compare retained metadata and payload bytes, cover both writer configurations and both time-series RV states, and verify recovery/retry. The operational documentation now accurately distinguishes database census candidates from objects actually served and states the stale-file and flag-off cleanup limitations.

Validation: my isolated targeted rollback/Get/Create/Delete run passed under -race (ok .../pkg/registry/file 3.256s). The independent lanes also passed broader rollback/export/migration/work-budget tests, cmd/cpexport, targeted go vet, formatting, and diff-whitespace checks. Code-review recommendation: APPROVE. Architectural status: CLEAR.

One merge-gate qualification: the current cross-platform CI job is red on this exact head because TestACG2_FlagOn/PreSave(base-CP)/orphan and /corrupt completed in about 521 ms, exceeding their 500 ms timing ceiling. The functional assertions passed, and this recheck's focused tests are green; I found no evidence tying the timing-only failure to this six-file correction. CI still needs a green rerun or separate test-flake handling before merge.

The legacy Delete path still does not provide complete atomicity after payload deletion succeeds: pre-existing metadata/file error swallowing remains outside this correction. Existing stale-file precedence, time-series exclusion, and flag-off cleanup limits also remain as documented. These are watch items, not new blockers in this delta.

Verdict: approve the code delta; no code changes requested. I am recording this as a review comment because GitHub does not permit the PR author to formally approve their own PR.

matthyx added 30 commits September 8, 2026 15:56

Merge branch 'feat/step1-l-measurement-harness' into prototype/full-a…

9156513

…cid-containerprofile Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

matthyx force-pushed the prototype/full-acid-containerprofile branch from 2f249d2 to f524ae9 Compare September 11, 2026 17:15

matthyx commented Sep 11, 2026

View reviewed changes

matthyx and others added 3 commits September 11, 2026 22:19

matthyx commented Sep 11, 2026

View reviewed changes

matthyx commented Sep 12, 2026

View reviewed changes

matthyx added this to KS PRs tracking Sep 13, 2026

matthyx moved this to WIP in KS PRs tracking Sep 13, 2026

matthyx marked this pull request as ready for review September 14, 2026 10:53

matthyx commented Sep 14, 2026

View reviewed changes

Uh oh!

Conversation

matthyx commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation (Tier B A/B measurement, hack/perf-ab.sh)

How to test

Rollout

AI Review

Ticket

Uh oh!

github-actions Bot commented Sep 11, 2026

Uh oh!

matthyx commented Sep 11, 2026

Uh oh!

matthyx left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 11, 2026

Uh oh!

matthyx commented Sep 11, 2026

Uh oh!

matthyx commented Sep 11, 2026

Uh oh!

matthyx left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 11, 2026

Uh oh!

matthyx commented Sep 11, 2026

Uh oh!

matthyx left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

matthyx commented Sep 14, 2026

Uh oh!

matthyx left a comment

Choose a reason for hiding this comment

Uh oh!

matthyx commented Sep 14, 2026

Uh oh!

matthyx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matthyx commented Sep 11, 2026 •

edited

Loading

Validation (Tier B A/B measurement, `hack/perf-ab.sh`)