fix(sessions): drop dead-end orphan entries when retry forks parentId chain (#48810)#79635
fix(sessions): drop dead-end orphan entries when retry forks parentId chain (#48810)#79635jeffrey701 wants to merge 1 commit into
Conversation
|
Codex review: found issues before merge. Reviewed July 1, 2026, 10:22 AM ET / 14:22 UTC. Summary PR surface: Source +190, Tests +468, Docs +1. Total +659 across 3 files. Reproducibility: yes. at source level: the linked issue supplies a concrete JSONL artifact with two compactions sharing one Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Refresh or rebuild the fix on current main so compaction-fork repair participates in full-file and trusted-append repair paths, remove the direct changelog edit, add cache-path regression coverage, and get maintainer sign-off on the compaction-only deletion rule. Do we have a high-confidence way to reproduce the issue? Yes at source level: the linked issue supplies a concrete JSONL artifact with two compactions sharing one Is this the best way to solve the issue? No as submitted: load-time repair is the right owner boundary for already-corrupted files, but the branch must be refreshed over current main and integrated with the incremental repair cache before it is the best fix. Source-side duplicate prevention can remain a follow-up once maintainers accept the repair policy. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8abd5d40712d. Label changesLabel justifications:
Evidence reviewedPR surface: Source +190, Tests +468, Docs +1. Total +659 across 3 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
…ks parentId chain Compaction retry can write a duplicate compaction event under the same parentId. One side becomes a dead-end with no descendants while the other carries the rest of the chain, so any downstream consumer that reconstructs causal order by walking parentId breaks at the fork. Extend repairSessionFileIfNeeded to detect this on load: when there are two or more compaction siblings sharing one parentId AND exactly one of those compaction siblings has a non-empty subtree, drop the dead-end compaction loser(s). Generic non-compaction entries are NEVER dropped: a session JSONL is a tree, so a legitimate side branch can naturally be a leaf next to a continued sibling, and only the compaction-retry path is known to produce the duplicate-sibling-with-shared-parentId shape this repair targets. Two-branch transcripts where multiple compaction siblings carry real subtrees are also left alone, and so are compaction-sibling groups where no candidate has won yet. Adds 9 regression tests including the canonical compaction-retry pattern, the openclaw#79635 [P2] regression that pins the generic-leaf carve-out, a mixed-sibling case (one compaction loser dropped while a non-compaction sibling under the same parent is preserved), nested compaction forks, deliberate two-branch transcripts, no-winner compaction candidate groups, linear transcripts, and legacy entries without parentId. Fixes openclaw#48810
fda1836 to
1583ec7
Compare
…ks parentId chain Compaction retry can write a duplicate compaction event under the same parentId. One side becomes a dead-end with no descendants while the other carries the rest of the chain, so any downstream consumer that reconstructs causal order by walking parentId breaks at the fork. Extend repairSessionFileIfNeeded to detect this on load: when there are two or more compaction siblings sharing one parentId AND exactly one of those compaction siblings has a non-empty subtree, drop the dead-end compaction loser(s). Generic non-compaction entries are NEVER dropped: a session JSONL is a tree, so a legitimate side branch can naturally be a leaf next to a continued sibling, and only the compaction-retry path is known to produce the duplicate-sibling-with-shared-parentId shape this repair targets. Two-branch transcripts where multiple compaction siblings carry real subtrees are also left alone, and so are compaction-sibling groups where no candidate has won yet. Adds 9 regression tests including the canonical compaction-retry pattern, the openclaw#79635 [P2] regression that pins the generic-leaf carve-out, a mixed-sibling case (one compaction loser dropped while a non-compaction sibling under the same parent is preserved), nested compaction forks, deliberate two-branch transcripts, no-winner compaction candidate groups, linear transcripts, and legacy entries without parentId. Fixes openclaw#48810
1583ec7 to
e70433a
Compare
|
Force-pushed Two things changed since the original push:
Local gates on
@clawsweeper re-review |
|
This pull request has been automatically marked as stale due to inactivity. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
This pull request has been automatically marked as stale due to inactivity. |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(sessions): drop dead-end orphan entries when retry forks parentId chain (#48810) This is item 1/1 in the current shard. Shard 11/22. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
|
Closing due to inactivity. |
Summary
Reported by @taohaowei in #48810: when compaction retries against a session JSONL file, the original (failed) compaction event and the successful retry both get appended under the same
parentId. One side becomes a dead-end (no descendants), the other carries the rest of the transcript. Anything downstream that walks the causal chain viaparentId(replay, dashboards, exports, audit tooling) breaks at the fork — it either picks the orphan and stops, or crashes on the ambiguity.Root cause
src/agents/session-file-repair.tsalready owns load-time repairs for several JSONL corruption shapes (truncated tail, malformed lines, missing session header, etc.). Compaction retry's same-parentIdduplicate slipped through every existing branch because it isn't a parse error — both entries are individually well-formed, the corruption is structural: two compaction siblings sharing oneparentIdwhere exactly one of them has a non-empty subtree.Fix
src/agents/session-file-repair.ts:RepairReportfielddroppedOrphanForkEntries?: numberso the existingbuildRepairSummaryPartsoutput stays consistent with how other repair counters (droppedMalformedLines, etc.) are reported.readParentLinkedEntry(entry)parses one raw JSONL entry and returns{ id, parentId, type }(ornullfor entries that don't participate in the parent graph — session headers, legacy entries without anid/parentId, etc.).detectAndDropOrphanForkEntries(entries)builds a parent->children map across the whole entries array, computes subtree size for every node, and drops a sibling only when ALL of:type: "compaction"(the canonical discriminator the source-side compaction writers use, permanual-compaction-boundary.tsandpi-embedded-runner-extraparams.ts), ANDrepairSessionFileIfNeededbetween the existing parse step and the "did we change anything" check, so an orphan-only repair triggers the same write-back path as any other repair, and the summary string surfaces the new counter.Why the compaction-only carve-out (clawsweeper #79635 [P2] response)
Initial implementation used a generic "any zero-descendant sibling next to a sibling-with-descendants" rule. The reviewer correctly flagged that a session JSONL is a tree, so a deliberate side branch can naturally be a leaf next to a continued branch — generic message leaves must not be deleted. Restricting drop candidates to
type: "compaction"siblings of othertype: "compaction"siblings is the documented seam: those are the entries the compaction retry path produces under the same parentId, and they're the only entries this repair touches. Generic non-compaction entries are now never dropped. A new regression test pins this exact carve-out.Tests
src/agents/session-file-repair.test.tsadds 9 cases:parentId— untouched.Real behavior proof
Behavior or issue addressed: #48810 — compaction retry creates orphan fork in
parentIdchain, breaking causal-order walkers downstream. After this patch,repairSessionFileIfNeededdetects compaction-vs-compaction same-parentId forks where exactly one compaction sibling has descendants and drops the dead-end loser(s), while leaving generic non-compaction leaves and deliberate two-branch transcripts untouched.Real environment tested: Linux 6.17.0-23-generic, Node.js v22.22.0, this PR's branch
fix/compaction-retry-orphan-fork-48810at head1583ec738bchecked out into a realopenclawworking tree. The proof script imports the actualrepairSessionFileIfNeededfromsrc/agents/session-file-repair.ts(no module shim, no mock) and drives it against real on-disk JSONL files in amkdtempdirectory. The@openclaw/fs-safeworkspace dependency was built locally from the pinned commitc7ccb99d3058f2acf2ad2758ad2470c7e113a53cand dropped intonode_modules/@openclaw/fs-safe/dist/so the production module loads end-to-end.Exact steps or command run after this patch:
The script writes 6 JSONL fixtures into
mkdtemp(...), callsrepairSessionFileIfNeeded({ sessionFile })against each, then re-reads the file from disk and prints the before/after entry list (line by line) plus the captureddebugcallback output.Evidence after fix:
Verbatim terminal capture (stdout + stderr combined) from the live run of
node ./node_modules/.bin/tsx .repro-79635.mtsagainst the patchedrepairSessionFileIfNeededimport. This is the actual console output, not unit test or vitest output.Observed result after fix: every scenario behaves as the design contract requires when run end-to-end against the real production module. Scenario A (the canonical #48810 compaction-retry fork) drops the dead-end loser and keeps the winner plus the post-compact message. Scenario B is the [P2] regression the bot asked for: a deliberate generic message leaf next to a continued sibling stays in the file. Scenario C confirms a
type: "compaction"leaf is preserved when its only sibling is a non-compaction continued branch (no compaction winner exists, so nothing to reconcile). Scenario D verifies mixed siblings: the compaction loser is dropped while the non-compaction message leaf under the same parent is preserved untouched. Scenarios E and F cover nested forks and linear transcripts respectively.What was not tested: a real end-to-end compaction retry against a live model provider that produces the duplicate compaction entry on disk in the first place was not run — that requires a live API key plus a transcript long enough to trigger compaction, which is outside the scope of a load-time-repair fix. The repair operates on the on-disk artifact regardless of which write-side path produced it, and the proof script reproduces every shape of that artifact verbatim, including the exact ordering and
parentIdlinkage from the issue's reproduction notes. Preventing the duplicate at write time (in the compaction retry path itself) is also out of scope and tracked as a follow-up in the PR body.Verification (supplemental to the proof above)
npx vitest run src/agents/session-file-repair.test.ts-> 60/60 passed locally on1583ec738b.npx vitest run src/gateway/session-utils.test.ts src/gateway/session-utils.fs.test.ts src/agents/session-file-repair.test.ts(the bot's stated acceptance criteria suites) -> 555/555 passed.npx oxfmt --check --threads=1 src/agents/session-file-repair.ts src/agents/session-file-repair.test.ts CHANGELOG.md-> clean.Out of scope (follow-ups)
supersededvs. drop pre-append vs. de-dup at flush. This PR fixes the corruption that's already on disk for affected users and leaves the write-side decision to a follow-up.Fixes #48810.