fix(agent): defer finalized receiver SWM cleanup to idle GC - #1996
fix(agent): defer finalized receiver SWM cleanup to idle GC#1996Bojan131 wants to merge 63 commits into
Conversation
Bojan131
changed the title
Bojan131
changed the title
|
Final live confirmation is complete on commit |
Review — #1996
|
Architecture feedback: keep finalized-SWM cleanup out of the write pathI am not comfortable with the current coupling of anti-resurrection cleanup to the SWM materialization path. To be precise, this PR reuses the existing per-KA writer-lock map rather than introducing a global mutex. My concern is that it significantly expands the lock-held critical section on an already store- and CPU-heavy flow. In
Marking those store operations as The deterministic post-catch-up path has a similar coupling: it awaits I would prefer an eventual-convergence design:
This deliberately changes the guarantee from “a late snapshot can never temporarily recreate finalized SWM” to “a late snapshot is removed after the next safe low-load GC window.” I prefer that tradeoff to adding more synchronous reads, hashing, lock time, and cleanup work to the network’s hot synchronization path. The validation should then prove both sides of that contract:
The PR already contains much of the safe cleanup logic in |
|
@Jurij89 Addressed every item from your review in
Validation: full corrected unit lane 149 files / 2,030 passed / 5 skipped, agent build/type/package-root checks passed, and GitHub is now 56 passed / 0 pending / 0 failed. |
The previous commit asserted all three predicates inside one parametrized case. That proves none of them leaks, but it cannot show the coverage DISCRIMINATES: every single-predicate mutant killed the same two cases, and an identical kill set is equally consistent with one assertion doing all the work. Same shape as the bug being fixed — one fixture standing in for three predicates. Split them, so each mutant reddens cases the other two leave green. Kill sets, captured by test name: drop finalizedSwmCleanupRoot -> 2 shared + the 2 root cases drop finalizedSwmCleanupMarkedAt -> 2 shared + the 2 markedAt cases drop finalizedSwmCleanupHeadFingerprint -> 2 shared + the 2 fingerprint cases The lanes are pinned the same way. Removing the filter from ONE reader reddens only that reader's cases, which is what shows a per-lane regression actually reaches the suite rather than being masked by a sibling lane: readSwmMetaRowsPage (legacy, TTL-disabled) -> only ttl=0 cases readFreshSwmMetaRowsPageFromPlan (TTL lane) -> only ttl=5000 cases Removing the predicate from the in-process set alone still SURVIVES, as before: every lane also filters at the store, so that set is defence in depth no test can pin without weakening the store-side clause. Also fixes the it.each title, which printed its arguments in the wrong order and labelled the predicate as the TTL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
The responder re-typed the finalized-cleanup predicate and task-type IRIs as local literals off its own DKG prefix, while the marker writer imports them from dkg-agent-constants. The values match today — verified — so this is drift risk, not a live leak. It is worth closing because of where it sits. If a predicate IRI ever changed, the writer would follow and the responder would keep filtering the old string, so local GC bookkeeping would be advertised to peers, silently. And the mutation matrix on this filter shows in-process-only drift SURVIVES testing: every lane also filters at the store, so no test can catch it. An unguarded silent peer-leak path is worth four lines. Derive all four, keeping the short aliases so the SPARQL templates stay readable. graph-plan.ts already imported from dkg-agent-constants, so this adds names to an existing import rather than a new module edge. Behaviour is unchanged and measured, not argued: the same store-side mutants kill the same cases through the indirection, per predicate, and the task-type clause mutant produces a result identical to the pre-change baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
| <http://dkg.io/ontology/assertionGraph> ?assertionGraph . | ||
| OPTIONAL { ?task <http://dkg.io/ontology/subGraphName> ?subGraphName } | ||
| } | ||
| } ORDER BY ?task LIMIT ${limit}`, |
There was a problem hiding this comment.
🔴 Bug: Preserved cleanup tasks can permanently starve later finalized SWM cleanup
What's wrong
The cleanup slice is bounded by LIMIT, but candidates that intentionally return preserved remain in place and are selected first again on the next sweep. If enough preserved tasks sort before valid tasks, the worker repeatedly spends its whole candidate budget on the same uncleanable rows and never reaches cleanable finalized SWM copies later in the ordering.
Example
With the default candidate budget of 4, if tasks a through d are preserved because their VM/SWM verification no longer matches, and task e is a valid cleanup candidate, every sweep re-reads a through d and never reaches e. The backlog metric still reports work, but the cleanable finalized SWM copy behind those preserved tasks is never deleted.
Suggested direction
Do not always restart candidate selection at the first task without a way to advance past fail-closed preserved entries. Track a cursor or rotate candidates within each meta graph while keeping the safety checks before deletion.
For Agents
Look at FinalizedSwmCleanupService.cleanupMetaGraph. Preserve the fail-closed behavior for tasks whose VM/SWM no longer matches, but add per-meta-graph pagination/cursoring, task aging, or another fair scheduling mechanism so preserved candidates cannot monopolize every bounded sweep. Add a test with more preserved tasks than maxCandidatesPerSweep followed by a cleanable task and prove the later task is eventually cleared.
There was a problem hiding this comment.
Confirmed, and tracked as #2020 — deliberately not fixed in this PR.
Independently found in review here, and your diagnosis matches: preserved candidates are re-selected first every sweep, so the node-wide budget never reaches cleanable tasks behind them.
Fixed at one level: task selection within a meta graph now uses a keyset cursor (650174547), taking a fixture from 4/12 to 12/12 distinct tasks examined. Not fixed across meta graphs and context graphs, and that is the honest state.
Why it went to a follow-up rather than into this PR: separating the deletion traversal from the measurement rotation reaches 30/30 in a fixture, but couples three contracts — wall-clock split, rotation-cursor persistence, and pressure classification — where fixing the second breaks the third. An attempt that reached 30/30 regressed four tests, one of them the rotation starvation guard fixed earlier in this same PR. Two intermediate patches both stalled at 15/30, which is the signal it is not patchable level-by-level.
#2020 carries the full spec: the evidence table, both traps that re-created the bug class while fixing it, the three coupled contracts as an acceptance spec, and rebuild notes. Your acceptance-test shape — more preserved tasks than maxCandidatesPerSweep followed by a cleanable one, asserting it is cleared rather than merely examined — is recorded there as the fairness half, complementary to the coverage assertion.
Note no green test contradicts the limitation: the rotation suite seeds backlog-only markers that discovery can never select, so deletion is never exercised there.
There was a problem hiding this comment.
🟡 Issue: Task-level cleanup pagination is not verified
What's wrong
The new cleanup service has three cursor layers, and the tests cover the context-graph and meta-graph rotation behavior, but not the innermost task cursor. That leaves a real backlog-stranding regression unguarded: the worker could keep scanning the same ordered task prefix forever while later cleanup tasks are never discovered.
Example
A test could seed one SWM meta graph with maxCandidatesPerSweep = 4, four early cleanup tasks that are preserved or otherwise uncleanable, and a fifth task that is clearable. Two sweeps should reach and clear the fifth task. If the cursor assignment/filter is removed, every sweep re-reads the same first four tasks and the fifth is never exercised.
Suggested direction
Add a focused regression test for a full candidate page whose prefix cannot be deleted, proving later tasks in the same meta graph are reached on a following sweep.
For Agents
Add coverage in packages/agent/test/finalized-swm-cleanup-rotation.test.ts or finalized-swm-cleanup-sweep.test.ts for full-page task discovery inside a single meta graph. Preserve the current behavior that full pages advance by task subject and short pages wrap. The test should fail if taskCursor is not persisted or afterTaskSubject is not applied.
| } | ||
|
|
||
| /** Run one bounded, idle-only maintenance slice. */ | ||
| async runSweep(): Promise<FinalizedSwmCleanupSweepResult> { |
There was a problem hiding this comment.
🟡 Issue: The cleanup service centralizes too many sweep responsibilities in one stateful method
What's wrong
The new service is dedicated, which is good, but its core method still carries several independent state machines at once. The repeated yield logic and mutable rotation fields make future changes expensive because a reader has to prove metric freshness, cursor progress, and destructive cleanup safety together.
Example
Changing the yield policy now requires touching several underPressure() / deadlineSignal.aborted / yieldRotation(...) branches and understanding how each affects rotationPending, stale backlog metrics, and deleted counts.
Suggested direction
Separate rotation/budget mechanics from candidate cleanup and SPARQL repository operations so the service reads as orchestration over smaller concepts.
Confidence note
This is a structural review finding; the behavior may be well covered, but the maintainability burden is visible from the new service shape.
For Agents
In packages/agent/src/finalized-swm-cleanup-service.ts, split the sweep into a pure rotation cursor/backlog accumulator and a candidate executor/repository. Consider a small SweepBudget helper that centralizes pressure/deadline classification. Preserve the current worker-facing result shape and rotation semantics.
There was a problem hiding this comment.
🟡 Issue: Split the finalized-SWM sweep state machine before it becomes permanent debt
What's wrong
This is a new 900-line service, and its central method is carrying several independent state machines through shared mutable fields. The implementation is careful, but the design is brittle: the next maintenance change has to reason about too many coupled cursors and accumulators at once.
Example
A future change that adds one more yield/fault point inside runSweep() must remember to update rotationPending, maybe metaGraphResumeCursor, maybe taskCursor, and avoid publishing partial backlog totals. The long comments around the method are already documenting that hidden protocol rather than letting the structure enforce it.
Suggested direction
Make cursor advancement a dedicated abstraction, ideally with pure transition functions for defer, completeMetaGraph, completeContextGraph, and closeRotation. Then keep runSweep() as orchestration over those transitions instead of the place where every cursor invariant is manually maintained.
For Agents
Refactor packages/agent/src/finalized-swm-cleanup-service.ts: extract a small rotation/cursor planner with explicit states and a cleanup executor that only processes one (contextGraphId, swmMetaGraph, afterTask) unit. Preserve current pressure/budget behavior and assert the existing rotation tests against the planner plus service integration.
There was a problem hiding this comment.
🟡 Issue: The finalized cleanup service is a monolithic state machine
What's wrong
The new service concentrates too many independent responsibilities in one class and one large method. The long comments are carrying invariants that should be encoded in smaller abstractions, so the implementation is fragile even if the current behavior is correct.
Example
A future sweep exit path has to coordinate rotationPending, metaGraphResumeCursor, taskCursor, rotationBacklogDepth, and stale backlog reporting. The current code already has separate cursor persistence in yieldRotation, the task page branch, and the catch block.
Suggested direction
Split the scheduler/cursor mechanics from task deletion and metric aggregation. A small CleanupRotation or async iterator with explicit commitContextGraph / defer operations would delete much of the ad-hoc state coupling and make new yield points much safer to add.
For Agents
Refactor FinalizedSwmCleanupService by extracting a pure rotation/cursor component, a single-meta-graph task cleaner, and a backlog metric accumulator. Preserve current pressure/budget/stale semantics and keep the existing rotation/pressure tests green, but make runSweep read as orchestration instead of owning all mutable state transitions directly.
| || expectedHead.shareOperationId !== shareOperationId | ||
| || expectedHead.assertionGraph !== assertionGraph | ||
| ) { | ||
| await this.retireStaleTask({ |
There was a problem hiding this comment.
🟡 Issue: Stale cleanup-task retirement is not covered
What's wrong
The PR adds a new cleanup path that deletes durable finalized-SWM cleanup tasks when their workspace head is absent or superseded. That is a meaningful lifecycle transition, but the added tests mostly cover exact cleanup, pressure/budget gates, and preservation races. Without a regression test for this branch, a change that never retires stale tasks, or one that retires a task while a payload is still live, can leave the backlog permanently noisy or strand a resurrected SWM copy without a cleanup task.
Example
Seed a finalized-cleanup task for assertion v1, then make the workspace head point at assertion v2 before cleanupKnownMetaGraph(). The test should prove the v1 task is retired while the v2 head/SWM data remains untouched. A separate headless case should prove a task is retired only when both the head and payload are gone.
Suggested direction
Add focused regression coverage for stale-task retirement, including the negative case where a headless payload is still present and the task must not be retired.
For Agents
Add service-level tests around FinalizedSwmCleanupService.cleanupKnownMetaGraph that enter retireStaleTask: superseded head, absent head with absent payload, and absent head with payload still present. Preserve live/newer SWM and assert task deletion only in the stale cases.
A yield inside a context graph's meta-graph loop leaves that graph at the head of `rotationPending`. Once every other graph has completed, the pending list is a single element, and the stall escape at resumeRotation required `pending.length > 1` — correct in itself, since rotating a one-element list is a no-op, but it left no escape at all for a sole head. `rotationPending` then never returned to null, so the rotation never re-seeded, and every OTHER context graph on the node was never swept again. Because the cursor is in-memory, that persisted until restart: #1996 reintroduced node-wide by the subsystem that exists to close it. Reproduced against the compiled service (3 CGs, slow one last with 12 meta graphs, 120ms budget): slices 1-7 entered only the slow graph and the other two were served 0 times. After the fix they are served on every third slice as the rotation re-seeds. On a sole stalled head, close the rotation and re-seed from the current context-graph list. The partial accumulator is discarded rather than published: an incomplete rotation must never become a whole-node total, so `lastKnownBacklogDepth` keeps the last complete measurement and slices keep reporting `stale` until a rotation genuinely finishes. A node whose budget cannot fit one context graph reports an honestly unknown backlog instead of a confident wrong one. The warning now also fires on this path. Previously it lived inside the deferral that could not trigger for a sole head, so the one case that stranded the node was the one case that logged nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
Closing the rotation for a sole stalled head restores node-wide progress, but the stalled graph itself still restarted its meta-graph walk at index 0 every slice and yielded at the same point. Everything past that point was never cleaned — the rotation cursor's own failure mode, one level down, and #1996 for those meta graphs. Measured on the repro (12 meta graphs, 120ms budget): 4 of 12 distinct meta graphs were ever reached, no matter how many slices ran. Now 12 of 12. Carry a per-graph resume offset and rotate the walk by it, wrapping. The walk still covers every meta graph in one pass, so a context graph still contributes to the rotation total only when fully measured and the no- double-count invariant is untouched — only the starting point moves. The cursor clears whenever the graph completes, so a graph that fits in one slice always starts at the top. Separate from the sole-head fix so it can be dropped independently: that one closes a node-wide strand, this one a per-graph one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
A terminal fault mid-rotation rethrew without touching `rotationPending`. Context graphs completed earlier in the same sweep were already credited to `rotationBacklogDepth`, and resumeRotation only zeroes the accumulator when `rotationPending` is null — which a throw never produces. The next sweep resumed from the stale tail, re-measured those graphs and added them again, and the inflated sum eventually published as `stale: false`: presented as a trustworthy fresh whole-node measurement. Reproduced on three context graphs of 10 markers each: true total 30, reported 30 -> 40 after one fault. Requires the rotation to have been resumed (a prior yield left `rotationPending` non-null); a fault during a fresh rotation is already safe because resumeRotation zeroes the accumulator on entry. Catch around the whole loop rather than at each of the four throw sites, so a raise site added later cannot miss the requirement — enumerating sites is how three of the four pressure conversions went unpinned earlier in this change set. The loop body is re-indented into the try; `git diff -w` shows the change is four lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
runSweep derives two deadlines from wallClockBudgetMs: one from the injected clock, and one from AbortSignal.timeout on real wall time that is threaded into the store and aborts its queries. Only the first is controllable from a test, and these tests injected a clock while setting a 100ms budget — so any slice taking longer than 100ms of real time yielded at a boundary the injected clock never chose. Reproduced deterministically before fixing: with the injected clock FROZEN, so the injected deadline can never fire, a 150ms store query against a 100ms budget still returned budgetExhausted: true after 158ms real. The yield came exclusively from the real timer. That matters more than ordinary flakiness here, because these are the tests pinning the cursor's forward-progress guarantee — the property that turned out to be broken. A non-deterministic test on a guarantee that failed is close to no test at all, and on a loaded machine or one running several worktrees it reddens for reasons unrelated to the code. The budget now exceeds any plausible real slice duration and the tick is expressed as a fraction of it, so the injected clock alone decides every deadline yield. The header records why, since the obvious tidying is to shrink these back to small round numbers. All five rotation mutants still die, so the tests remain load-bearing rather than merely stable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
Discovery was ORDER BY ?task LIMIT n with no cursor, so every sweep re-selected the same prefix. A task that can never be acted on — a marker armed from VM state whose live SWM diverges, which finalization-handler reaches whenever VM verifies — returns `preserved` on every sweep and is charged to the node-wide deletion budget every time. Four such rows at the ORDER BY head are enough that no other meta graph gets a candidate query again. The charging is not the bug and is deliberately unchanged: examination is the expensive operation (a head resolve, plus two verifyExactGraphScopedLayer passes per matching row), so counting only deletions would uncap exactly the work the budget bounds. Selection is the bug. Carry one keyset position, advancing while the page is full and clearing when it is short — without the wrap this trades a stuck prefix for a permanently stranded suffix, the same starvation with the opposite sign. A single entry is bounded by construction rather than by an eviction policy: charging per examined candidate means a full page spends the rest of the budget, so at most one meta graph can hold an unfinished page. Only runSweep supplies the cursor, so the cleanupKnownMetaGraph seam and its ~dozen drainFinalizedSwm callers keep selecting from the top unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
6501745 states that without the short-page wrap a stuck prefix is traded for a permanently stranded suffix. Mutation testing refutes that: a mutant that advances the cursor on short pages too is NOT killed, because an empty page carries no last subject and clears the cursor anyway. Dropping the wrap costs one wasted empty query per cycle; it does not strand anything. The wrap is kept — one query per cycle is worth the line — but described as the optimisation it is. I asserted the stronger claim in a commit message and a code comment without testing the negative, which is the same failure this change set has been correcting elsewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
The sweep loop keeps growing rotation-cursor persist sites — a fault path, then a task-selection cursor — and a mutant that enumerates them weakens silently every time one is added. The enumeration is complete when written, the code grows underneath it, and nothing announces that the mutant now neutralises only part of what it names. That is harder to notice than ordinary under-enumeration, because there is no point at which anyone did anything wrong. Records the site-independent form instead: neutralise the cursor at its single declaration with a getter/setter pair that discards writes. It cannot be outgrown by new assignment sites, so it stays valid across restructures of the loop rather than needing maintenance alongside them. Verified against the current shape, which already has two persist sites plus a clear: the declaration mutant kills the resume test on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
The header now tells the next person to neutralise the rotation cursor at its declaration, and that instruction is easy to over-apply: "one mutation covering everything" is exactly the failure this change set spent its time removing, where four conversion sites were killed by one test and three predicates were asserted in one case. The two are not the same. A blanket mutant stands in for many properties, so its kill says something broke without saying what. A site-proof mutant covers one property and is merely robust to that property gaining implementation sites. Records the check that separates them: whether the kill set stays narrow and specific. The declaration mutation kills the resume test and nothing else. A site-proof mutant that starts reddening half the suite has become blanket, and that is a signal to split it rather than a stronger result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
markFinalizedGraphScopedSwmForCleanup wrote the tombstone with a bare store.insert while every other writer of that operation subject takes swmKaWriteLockKey. Catch-up REPLACES the same subject under that lock: it reads the tombstone, deletes the subject, then re-inserts from the snapshot. An unlocked marker lands inside that window — it runs at default priority while the replace's reads and deletes are queued at background, so it is admitted ahead of them — and the re-insert restores a snapshot taken before it existed. The tombstone is then gone while the independent task subject survives, so the GC cleans the lifecycle once and retires the task, and the next catch-up re-materializes the SWM copy with nothing left to re-arm cleanup from. That is a permanent resurrection of exactly what this PR removes. Taking the lock is compatible with keeping finalization foreground work O(1) because that constraint is on WORK, not on waiting. This path already reads and hashes the entire SWM payload via verifyExactGraphScopedLayer before reaching the marker write, so a bounded wait behind one KA's replace is smaller than what it has already done, and it adds no cleanup discovery, verification, deletion, or GC wait. The insert is a leaf operation, so the lock cannot nest and cannot deadlock. writeLocks returns to FinalizationHandler injected and READ. The field deleted earlier in this branch was dead — assigned, never used — which is worse than absent because it advertised a serialization that never happened, and is why this race survived review. Rejected alternatives: re-reading the tombstone later inside the lock only narrows the window, and replaceSubjectAtomicallyOrFallback does not close it either, since the payload is still snapshotted before the atomic boundary. Moving the tombstone to a subject catch-up never deletes is the better end state but is a persisted-shape change with an upgrade path, filed separately. The test injects the interleaving rather than racing for it: the marker is started as catch-up is about to delete the operation subject and given twenty event-loop turns. Unlocked it completes every time; locked it cannot complete at all, because the replace holds the lock — so the assertion turns on mutual exclusion, not timing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
retireStaleTask is the only path that removes a cleanup task whose head has moved on, and nothing reached it — not the function, not its call site. Its guards were therefore free to be removed without any test noticing. The load-bearing case is the negative one: an absent head with the payload still present must NOT retire. That is not a finished lifecycle, it is a resurrected SWM copy whose head has not been rebuilt, and retiring its task strands that copy with nothing left to collect it — the resurrection this change set exists to prevent, arriving through the retirement path rather than the deletion path. Removing that guard ships green today. The superseded case asserts the newer assertion survives intact, not merely that the stale task went: a change that retires v1 and damages v2 would pass a task-count assertion. Retirement is also pinned as inert when the store cannot report write generations, which is production behaviour on a backend that ships no tracker. Pinning the inertness is deliberate — a later decision that retiring on non-write-gen evidence is safe should have to turn this red rather than pass silently. Task presence is asserted directly because retirement is not reflected in the drain's return value, which counts reclaimed lifecycles only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
retireStaleTask re-resolves the head under the writer lock and declines to retire when the lifecycle it names has become current again. Nothing reached that branch: the four retirement cases all leave the head mismatched throughout, so the guard could be removed with every one of them still green. It is the twin of the re-check in clearIfStillExact, on the other destructive path. Narrower — this removes a marker rather than payload, so the damage is a finalized SWM copy left with nothing to collect it rather than data destroyed — but the same class, and reachable only by changing the head BETWEEN the two reads, which no static fixture produces. The seam is positional rather than by source: both head resolutions on this path carry the same discover source, so the fixture counts occurrences. That is recorded in the test, because inserting another query with that source ahead of these would retarget the seam silently and the test would keep passing while no longer exercising the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
Merge-readiness review — round 3, at
|
| Level | Symptom | Status |
|---|---|---|
| context-graph rotation | re-walks the same CG prefix; later CGs never swept | fixed |
| meta-graph walk | re-walks the same meta prefix; later meta graphs never cleaned | fixed |
| task selection | re-walks the same task prefix; successors never examined | fixed (keyset cursor) |
| cross-level (deletion traversal) | node-wide budget always lands at the front | open — documented |
All four are no cursor, restart at the beginning, successors never reached. Levels 2–4 were found
only because level 1 was investigated. The fourth is not patchable level-by-level: two cascade
attempts both stalled at 15/30 distinct tasks examined, and a full separation reached 30/30
but regressed four tests — including the level-1 starvation guard fixed earlier in this same PR.
Root cause: the GC borrows one traversal for two jobs with opposite requirements. Measurement must
cover every context graph for the backlog total to be whole-node; deletion must resume exactly where
its budget ran out and ignore rotation shape. Separating them couples three contracts — wall-clock
split, rotation-cursor persistence, and pressure classification — where fixing the second breaks the
third. That is a design task, not a patch, and it goes to a follow-up with a full spec rather
than being converged on by trial at the end of a fix round.
Fixed and verified by me at the frozen tip
| Fix | How I verified it |
|---|---|
| rotation strand — every other CG starved forever | own probe: cg-b/cg-c served every third slice, was never |
| intra-CG meta-graph walk | own probe: 12/12 meta graphs measured, was 4/12 |
| marker write unlocked → tombstone erased → SWM resurrected | own single-site mutant: kills exactly 1 test, named for the behaviour |
terminal fault double-counted backlogDepth as stale:false |
own probe: publishes 30, the true total, was 40 and growing |
| responder GC-row leak, per predicate and per lane | own store-side mutant: kills the 2 shared + exactly the 2 named cases |
| responder IRIs retyped rather than derived | same mutant kill set through the indirection; 16 passed alongside |
Restores proven by git diff 0 lines plus a positive grep for the real construct — not sha1,
which is unreliable here because LF→CRLF normalization changes bytes while content is identical.
Known unguarded — real, not blocking
- Cross-level deletion starvation (above). On a node whose slice budget cannot cover a full
traversal, deletion favours the front. Independently raised as a 🔴 by the review bot from a cold
read. No green test contradicts this — the rotation suite seeds backlog-only markers that
discovery can never select, so deletion is never exercised there. - Peer-planted cleanup tasks — an authorized peer can write subjects the GC treats as its own
work list, occupying its budget. Availability, not integrity: every guard inclearIfStillExact
recomputes from local state, so a plant selects which KA is considered, never whether deletion is
safe. - Retirement is silently inert without write-generation tracking.
retireStaleTask's absent-head
branch returns whenpreflightWriteGen === undefined.GraphWriteGenTrackeris present in the
oxigraph and sparql-http adapters but not inblazegraph.ts— a supported backend. An operator
there experiences it as unexplained budget starvation withlastError: null. - The rotation tests inject a clock the abort deadline does not use.
deadlineis injectable;
AbortSignal.timeoutis real. Mitigated by a budget large enough that the real timer cannot fire
first, with a header recording why the numbers must not shrink back. The injectable seam is the
real fix and is deferred. - Retirement is not counted as progress —
deletedItemscounts only'cleared', so a sweep that
only retires reports zero and the worker's fast re-drain never arms; work waits for the 15-minute
backstop. - Agent test files are never typechecked;
ka-graph-finalization-recovery.test.tsis absent
fromvitest.unit.config.tsand silently skips when named. It exercises this PR's changed code
indirectly — viaDKGAgent, so a grep forFinalizationHandlerin it returns zero and proves
nothing. It was run explicitly in the freeze pass: 5 passed, so no bisect was needed. The
wiring defect stands on its own; it was not covering a rotting suite. spawnHardhatEnvport cleanup is a no-op on Windows, and the two vitest configs pin different
ports (9545 and 9547), so a manifest checking one can collide on the other.
What this round cost me, stated because it changed decisions
- I proposed a fix that would have been a regression. "Charge the budget for work done" would let a
meta graph with 16 preserved rows run ~32 layer verifications for zero budget spend — uncapping the
work the budget exists to bound. Examination is the expensive operation. - I mis-framed the planted-task finding as integrity — "security boundary", "peer can plant deletion
tasks". It is availability. That phrasing would have priced the PR wrong for anyone reading only the
summary. - My write-up of the double-count omitted a precondition — it needs a prior yield; a fault during
a fresh rotation is already safe. A repro without the yield would have cleared a real finding. - My first reproduction of the confirmed HIGH failed and the bug was real. My probe gave the slow CG
no meta graphs, so its iteration completed and the rotation closed — the harness could not have shown
the failure. Had I stopped there I would have reported it refuted. - My review apparatus overran its agent budget 3× (43 vs 12–15): one refuter per finding, uncapped,
so agent count was set by how much the lenses found. Several refuters also verified against a stale
worktree; one finding's evidence was stale as a result, and only re-checking at the real head caught it. - I treated a status report as a decision and issued a consequence of it, churning a teammate through
drop → hold → drop.
The most transferable output — verification that lies
Four ways a mutation result gets fabricated, all found this round, all producing confident output:
- a run that never executed (Hardhat port collision) reporting as a kill;
- a mutation left on disk by a failed apply, contaminating the next result;
- a
git checkoutrestore silently discarding the uncommitted change under test; - the validity check itself failing silently —
grep -E "^ *Tests "matches nothing because of the
ANSI prefix, so an empty result is indistinguishable from a clean run.
A mutant result counts only if the run emitted
Tests … passed, the tree was provably clean
beforehand, and the change under test is still present afterwards — with a matcher you have proven
can match.
And two beyond mutation:
- A rewritten commit keeps its subject. "The branch still shows the commit I verified" is not a
check. Compare blobs of the artefacts you touched — sha inequality says something moved; blob
equality says your evidence survives. - A check that can't fail is not a property of tests. It is a property of any artefact that reads as
a guarantee while nothing enforces it. Five instances this round: an untested negative in a code
comment and commit message; a proposed cardinality bound in a doc comment (later proved false); a
documented "callers MUST fail open" contract one caller quietly violates; a commit message describing
a mechanism about to be removed; and an approved warning about a call being inside a lock it was two
lines outside. A false warning rots exactly like a false guarantee.
Usable form: "the system guarantees X" stays out of code unless there is a fixture; "don't change
this to Y" belongs in it.
And one thing stronger than any mutant. The measurement-rotation tests were backed all round by
mutation matrices, which establish sensitivity, not usefulness. Then a real change arrived and one went
red for the right reason before it shipped. Mutants prove a test can fail; a genuine regression proves
it catches one.
Follow-ups
- Deletion traversal starvation — full spec, evidence table, both traps, rebuild notes.
- Peer-planted cleanup tasks — canonical-subject gate plus a retirement path that terminates.
- Blazegraph write-generation gap — retirement inert; the store-abstraction hole is broader than this PR.
- Inbound predicate allowlist on
validateOperationRows— the root cause behind (2). - Tombstone on its own subject — closes the resurrection race with no lock and removes the tombstone read from the ingest path.
GraphManager.listContextGraphsowner/name defect — pre-existing, already user-reported as GH SWM WorkspaceOperation writes prov:wasAttributedTo as PeerId literal instead of agent DID URI #748, four callers remain.
| <${DKG_NS}publicQuadsCount> ?count . | ||
| FILTER(STR(?version) = ${JSON.stringify(input.scope.assertionVersion)}) | ||
| } | ||
| } ORDER BY ?shareId LIMIT 16`, |
There was a problem hiding this comment.
🔴 Bug: Immutable snapshot recovery can permanently miss candidates after the first 16 share IDs
What's wrong
This fallback is meant to recover after finalized SWM cleanup removes the live SWM copy, but the fixed LIMIT 16 makes the answer depend on lexicographic share-operation ordering rather than on whether a matching immutable snapshot exists. Once the matching operation is outside that first page, retries are deterministic and keep missing the same valid snapshot, so recovery is stranded.
Example
If a KA has 17 same-version operation snapshots with the same public triple count, and the matching snapshot's shareOperationId sorts 17th, verifyImmutableGraphScopedSnapshot never examines it. Every retry re-runs the same ORDER BY ?shareId LIMIT 16, returns undefined, and chain reconciliation cannot restore the VM from the immutable snapshot.
Suggested direction
Do not truncate the search to the first 16 lexicographic share IDs. Page through candidates deterministically, or derive a stronger indexed key when evidence includes a digest, while still bounding repeated payload verification by digest memoization.
For Agents
In FinalizationHandler.verifyImmutableGraphScopedSnapshot, replace the fixed first-page lookup with a complete paged/keyset scan, or otherwise continue discovery until all matching metadata candidates have been considered. Preserve the content-digest rejection memo so duplicate-content candidates do not cause repeated payload reads. Add a regression where the matching share operation sorts after at least 16 nonmatching candidates and reconciliation still promotes from the immutable snapshot.
There was a problem hiding this comment.
Fixed in bb2efd5a9, shipped in 46243351a. Confirmed as filed — verified that verifyImmutableGraphScopedSnapshot has zero occurrences at the merge base, so this PR introduced the path, and the fallback fires whenever the live SWM no longer verifies, which after the GC has run is the normal case for a late receipt.
Both discriminating tests moved into the SPARQL WHERE, so LIMIT truncates among matching candidates rather than all candidates. The content-digest rejection memo is untouched — it bounds repeated payload verification, which is work; this was always the search.
The count filter is deliberately numeric-or-lexical and guarded by Number.isSafeInteger so it fails open: a pre-filter stricter than the JS check it precedes would be the same missed-discovery bug from the other direction.
Fail-before proven — reverting the filters to post-LIMIT reddens exactly finds a matching snapshot that sorts past the discovery limit with expected undefined to be 'verified', one test, no collateral. Detail in the addendum comment on the PR.
| * tombstone read it already performs, so calling both duplicates a | ||
| * lock-held store read per KA for no added guarantee. | ||
| */ | ||
| ensureFinalizedCleanupTask( |
There was a problem hiding this comment.
🟡 Issue: Hide cleanup re-arming behind the materializer instead of making callers remember branch rules
What's wrong
The interface creates a temporal coupling: callers must know which materializer methods implicitly re-arm cleanup and which paths need a separate call. That is exactly the kind of special-case branching this PR should be eliminating, and it will be easy to regress as more snapshot decisions are added.
Example
Adding another early-return branch in runSharedMemorySync() after readStoredHead() but before replaceHeadMetadata() now requires the caller to remember both to call ensureFinalizedCleanupTask() and to add the descriptor metadata keys to replacedGraphScopedMetaKeys. That is lifecycle policy leaking into orchestration.
Suggested direction
Collapse ensureFinalizedCleanupTask, replaceHeadMetadata, and the caller-side replacedGraphScopedMetaKeys bookkeeping into a single higher-level materializer workflow for one descriptor. The sync caller should ask for an outcome, not coordinate cleanup-task policy itself.
For Agents
Look at packages/agent/src/sync/requester/shared-memory-sync.ts and packages/agent/src/sync/requester/swm-snapshot-materializer.ts. Move the finalized-cleanup rearm decision and metadata-suppression bookkeeping behind one materializer operation or return a typed materialization outcome that carries the metadata rows to insert/skip. Preserve all four current branches and their counters.
verifyImmutableGraphScopedSnapshot bound every same-version operation and took ORDER BY ?shareId LIMIT 16, then applied the triple-count and digest tests in JS to whatever survived. So LIMIT truncated the CANDIDATE SET, not the work: sixteen same-version operations with the wrong count filled the window and the matching snapshot was never examined. That is not a flake that clears on retry. The ordering is deterministic over stable store state, so every attempt re-derives the identical sixteen and misses the same snapshot, and chain reconciliation can never restore the VM. Reaching it needs only a KA re-shared repeatedly at one version. This path is also new in this branch and load-bearing: the fallback fires whenever the live SWM layer no longer verifies, which after the idle GC has run is the normal case for a late receipt. The branch both created the path and made it the common one. Push the discriminating tests into the WHERE clause so LIMIT truncates among MATCHING candidates. With a digest supplied the match count is ~1 and the limit stops mattering; without one, count-filtering alone collapses the field. ?count is compared numerically first so a non-canonical typed literal still matches, with the lexical form as a fallback for an untyped one. The filter must never be stricter than the JS check it precedes — one that rejects a candidate the caller would have accepted is the same missed-discovery bug wearing different clothes. The content-digest rejection memo is unchanged. It bounds repeated payload verification, which is work; this was always about the search. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxoLY1KYH1cRrsqStKCsaH
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: retry_exhausted
Addendum — head moved to
|
4 participants
Summary
/api/sloThe exact finalized KA is already cleared unconditionally on the publisher side.
clearSharedMemoryAftercontrols only the separate family-wide sweep, so this change makes receiver behavior consistent without changing that opt-in.Architecture
Finalization and background cleanup
sequenceDiagram participant Chain participant Finalizer as Receiver finalizer participant Store participant GC as Finalized SWM GC worker Chain->>Finalizer: Finalized assertion Finalizer->>Store: Promote exact assertion to VM Finalizer->>Store: Write fixed-size cleanup task + tombstone Finalizer-->>Chain: Return without cleanup discovery or payload reads Finalizer->>GC: Non-blocking wake GC->>Store: Check ACK, health, and normal-lane pressure alt Store is busy GC-->>GC: Stop this slice and retry later else Store is idle GC->>Store: Discover at most 4 candidates (background priority) GC->>Store: Verify exact VM and SWM outside the KA lock GC->>Store: Acquire existing per-KA writer lock GC->>Store: Re-read task and head; conditionally delete exact SWM graph/head GC->>Store: Retire completed task endLate or repeated snapshot
sequenceDiagram participant Peer participant Sync as SWM snapshot materializer participant Store participant GC as Finalized SWM GC worker Peer->>Sync: Late/repeated finalized SWM snapshot Sync->>Store: Materialize through the normal per-KA writer lock Sync->>Store: Read immutable operation tombstone and re-arm cleanup task Sync-->>Peer: Complete without VM/SWM verification or deletion Sync->>GC: Catch-up performs a non-blocking wake GC->>Store: On the next safe idle slice, verify and remove only the exact duplicateThe worker is single-flight, checks pressure before
listContextGraphs, before SWM-meta discovery, and between candidates, uses background scheduler priority, yields between items, and stops after 4 candidates or 10 seconds. A periodic wake is the restart/lost-wakeup backstop; TTL expiry remains independently disabled whensharedMemoryTtlMsis0.Safety
Cleanup is deliberately eventually consistent. Under sustained foreground load an exact SWM+VM duplicate can remain until the next low-load window, but the worker performs no graph discovery or payload scanning while pressure is active.
Validation
/api/slocleanup metrics: 9/9 passedgit diff --check origin/testnet-canary...HEAD: passedThe regression coverage includes: zero discovery while pressure is active; catch-up wake without awaiting GC; late/repeated snapshot re-arm and eventual idle convergence; task retirement; exact VM digest preservation; and fail-closed behavior for missing VM, changed/newer SWM, and corrupt content.
Historical blackbox context
The original failure was reproduced with real two-node harness runs as 40 correct VM KAs plus the same 40 KAs remaining in SWM. Those runs establish the workload and failure mode; the current worker revision is validated by the updated build and regression suites above and should be followed by a fresh harness run that allows the low-load GC window before asserting final SWM parity.
Current PR status
testnet-canarybase and resolved the combined-base attribution conflict