fix: stop wiki summary worker crash on long sessions#298
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds shared wiki offset helpers and updates multiple wiki workers to resume from persisted sidecar counts, process only new session rows, cap JSONL input size, and avoid uploading summaries after failed executions. Related tests were extended for the new resume, skip, and failure paths. ChangesWiki worker offset resumption
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 6 files changed
Generated for commit 3cbeb7c. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/hooks/wiki-offset.ts (1)
21-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate regex literals risk drift.
OFFSET_REand the regex insideparseOffsetencode the same pattern separately. If one changes without the other,stampOffset's detection andparseOffset's extraction desync, breaking the round-trip contract the tests assert ("Verifies that stamping an existing “JSONL offset” line changes the offset value and that parseOffset(out) returns the same number (round-trip contract).").♻️ Proposed consolidation
-const OFFSET_RE = /\*\*JSONL offset\*\*:\s*\d+/; +const OFFSET_RE = /\*\*JSONL offset\*\*:\s*(\d+)/; export function parseOffset(summary: string): number | null { - const m = summary.match(/\*\*JSONL offset\*\*:\s*(\d+)/); + const m = summary.match(OFFSET_RE); return m ? parseInt(m[1], 10) : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/wiki-offset.ts` around lines 21 - 28, The offset-matching logic in OFFSET_RE and parseOffset is duplicated and can drift, breaking the stampOffset/parseOffset round-trip. Consolidate the pattern in src/hooks/wiki-offset.ts so parseOffset reuses OFFSET_RE (or a single shared capture regex) instead of hardcoding its own regex, and keep the existing symbols parseOffset and OFFSET_RE aligned with the worker read/write contract.tests/codex/codex-wiki-worker.test.ts (1)
248-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this log assertion specific to the expected offset/count.
toContain("no new events since last summary")is broader than the repo test rule and would still pass if the numeric details regressed. Assert the exact message for thelastSummaryCount = 3/eventCount = 3case instead. As per path instructions,tests/**: "Prefer asserting on specific values (paths, messages) over generic substrings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/codex/codex-wiki-worker.test.ts` around lines 248 - 249, The wiki worker test assertion is too broad and can miss regressions in the numeric details. Update the `codex-wiki-worker.test.ts` check around `readFileSync`/`expect(log)` to assert the exact log message for the `lastSummaryCount = 3` and `eventCount = 3` case, rather than using a generic substring match. Use the existing `wiki.log` assertion in this test to locate the spot and make the expected offset/count explicit.Source: Path instructions
tests/claude-code/wiki-worker.test.ts (1)
352-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full log messages here.
Both checks only match generic substrings, so they won't catch regressions in the offset/total values or the skip-upload suffix. Assert the exact emitted messages for these scenarios instead. As per path instructions,
tests/**: "Prefer asserting on specific values (paths, messages) over generic substrings."Also applies to: 367-368
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/claude-code/wiki-worker.test.ts` around lines 352 - 353, The wiki-worker log assertions are too generic and can miss regressions in the emitted offset/total values and skip-upload suffix. Update the checks around the wiki.log reads in the relevant test cases to assert the full expected log messages exactly instead of using substring matches, so the scenarios covered by the wiki worker logging stay precise.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/codex/wiki-worker.ts`:
- Around line 236-245: The offset upload logic in wiki-worker’s summary handling
should not advance on a failed codex exec merely because tmpSummary changed. In
the block around raw/summaryChanged and the stampOffset path, gate finalizing
jsonlLines on execSucceeded, and for failed runs only upload when there is a
confirmed valid summary state rather than a partial write. Keep the existing
readFileSync/tmpSummary flow, but make the condition in the summaryChanged check
and the subsequent stampOffset handling in wiki-worker.ts prevent persisting a
new offset after timeout/non-zero exit.
In `@src/hooks/cursor/wiki-worker.ts`:
- Around line 223-231: The offset handling in cursor-worker should not finalize
or advance after a failed cursor-agent exec just because tmpSummary changed.
Update the logic around summaryChanged, execSucceeded, and stampOffset so only
successful executions can stamp and persist a new offset, while failed runs with
partial raw output are ignored or retried without slicing jsonlLines. Use the
cursor-agent --print flow in wiki-worker to locate the guard and keep the
persisted summary authoritative only on success.
In `@src/hooks/hermes/wiki-worker.ts`:
- Around line 230-238: Do not finalize or advance the offset when
`execSucceeded` is false, even if `summaryChanged` is true, because a failed
hermes run may have only produced a partial `tmpSummary`. Update the flow in
`wiki-worker.ts` around `summaryChanged`, `stampOffset`, and the upload path so
offset stamping and persistence only happen after a successful exec, and failed
runs simply skip upload without consuming `jsonlLines`.
In `@src/hooks/pi/wiki-worker.ts`:
- Around line 229-237: The offset update in wiki-worker’s summary handling is
too permissive: a failed pi --print run can still set summaryChanged and advance
lastSummaryCount even when the output is only a partial tmpSummary. In the logic
around execSucceeded, summaryChanged, and the stampOffset/raw upload path, gate
any offset advancement on execSucceeded being true (or otherwise confirm a valid
new summary) so failed runs never persist a higher lastSummaryCount. Keep the
skip-upload behavior for failed executions without a real summary change, and
ensure the code path that writes the stamped summary does not advance the offset
after failure.
In `@src/hooks/wiki-worker.ts`:
- Around line 210-216: The tail-cap logic in `capLinesByBytes` still allows one
oversized newest row to slip through, so `tmpJsonl` can exceed
`WIKI_JSONL_MAX_BYTES`. Update the handling in `wiki-worker.ts` around
`newRows`, `capLinesByBytes`, and `writeFileSync` so any single line larger than
the limit is rejected or truncated before being written, rather than being
unconditionally accepted as the first kept entry. Keep the existing
summary/logging flow, but ensure the final JSONL buffer always stays within the
byte cap.
- Around line 198-205: The offset handling in wiki-worker should not trust
readState(cfg.sessionId)?.lastSummaryCount unless the prior summary was actually
loaded from SELECT summary. In the logic around prevOffset and newRows, add a
guard so the stored lastSummaryCount is only applied when the summary seed was
successfully retrieved; otherwise fall back to 0 and process from the start. Use
the existing readState and prevOffset/newRows flow in src/hooks/wiki-worker.ts
to locate the change.
---
Nitpick comments:
In `@src/hooks/wiki-offset.ts`:
- Around line 21-28: The offset-matching logic in OFFSET_RE and parseOffset is
duplicated and can drift, breaking the stampOffset/parseOffset round-trip.
Consolidate the pattern in src/hooks/wiki-offset.ts so parseOffset reuses
OFFSET_RE (or a single shared capture regex) instead of hardcoding its own
regex, and keep the existing symbols parseOffset and OFFSET_RE aligned with the
worker read/write contract.
In `@tests/claude-code/wiki-worker.test.ts`:
- Around line 352-353: The wiki-worker log assertions are too generic and can
miss regressions in the emitted offset/total values and skip-upload suffix.
Update the checks around the wiki.log reads in the relevant test cases to assert
the full expected log messages exactly instead of using substring matches, so
the scenarios covered by the wiki worker logging stay precise.
In `@tests/codex/codex-wiki-worker.test.ts`:
- Around line 248-249: The wiki worker test assertion is too broad and can miss
regressions in the numeric details. Update the `codex-wiki-worker.test.ts` check
around `readFileSync`/`expect(log)` to assert the exact log message for the
`lastSummaryCount = 3` and `eventCount = 3` case, rather than using a generic
substring match. Use the existing `wiki.log` assertion in this test to locate
the spot and make the expected offset/count explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54992d03-de5d-4b5d-95bc-9b8a1714dbae
📒 Files selected for processing (13)
src/hooks/codex/wiki-worker.tssrc/hooks/cursor/wiki-worker.tssrc/hooks/hermes/wiki-worker.tssrc/hooks/pi/wiki-worker.tssrc/hooks/wiki-offset.tssrc/hooks/wiki-worker.tstests/claude-code/wiki-worker-plugin-version.test.tstests/claude-code/wiki-worker.test.tstests/codex/codex-wiki-worker.test.tstests/cursor/cursor-wiki-worker.test.tstests/hermes/hermes-wiki-worker.test.tstests/pi/pi-wiki-worker.test.tstests/shared/wiki-offset.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/hermes/wiki-worker.ts (1)
147-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t treat summary lookup failures as “no existing summary.”
Line 159 swallows any
queryfailure and then continues withprevOffset = 0. If the canonical summary exists but the lookup temporarily fails, the worker can proceed, cap to the newest rows, and upload a replacement summary without the previous base.Proposed safe failure handling
- } catch { /* no existing summary */ } + } catch (e: any) { + wlog(`existing summary lookup failed: ${e.message}; skipping to avoid overwriting an unknown base summary`); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/hermes/wiki-worker.ts` around lines 147 - 170, The summary lookup in wiki-worker is treating any query failure as if no existing summary were found, which can incorrectly reset the offset and overwrite a valid canonical summary. Update the try/catch around the summary fetch in the logic that reads from query(), tmpSummary, and hasExistingSummary so only a confirmed empty result sets hasExistingSummary false; if query fails, preserve the existing offset state and avoid falling back to prevOffset = 0. Keep the existing summary path handling in the same section so stale-sidecar logic still applies only when the summary was actually loaded.
🧹 Nitpick comments (1)
tests/codex/codex-wiki-worker.test.ts (1)
265-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full failure log message.
This only checks a fragment, so a weaker or different failure message could still pass. Prefer the exact expected message text here.
Proposed test tightening
- expect(log).toContain("failed after a partial summary write"); + expect(log).toContain("codex exec failed after a partial summary write; skipping upload to avoid advancing the offset");As per path instructions,
tests/**: Prefer asserting on specific values (paths, messages) over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/codex/codex-wiki-worker.test.ts` at line 265, The test in codex-wiki-worker should assert the complete failure log message instead of a substring, since checking only “failed after a partial summary write” allows weaker messages to pass. Update the expectation in the relevant codex-wiki-worker test to match the exact log text produced by the failure path, using the existing log assertion around log so the test verifies the full message precisely.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hooks/hermes/wiki-worker.ts`:
- Around line 147-170: The summary lookup in wiki-worker is treating any query
failure as if no existing summary were found, which can incorrectly reset the
offset and overwrite a valid canonical summary. Update the try/catch around the
summary fetch in the logic that reads from query(), tmpSummary, and
hasExistingSummary so only a confirmed empty result sets hasExistingSummary
false; if query fails, preserve the existing offset state and avoid falling back
to prevOffset = 0. Keep the existing summary path handling in the same section
so stale-sidecar logic still applies only when the summary was actually loaded.
---
Nitpick comments:
In `@tests/codex/codex-wiki-worker.test.ts`:
- Line 265: The test in codex-wiki-worker should assert the complete failure log
message instead of a substring, since checking only “failed after a partial
summary write” allows weaker messages to pass. Update the expectation in the
relevant codex-wiki-worker test to match the exact log text produced by the
failure path, using the existing log assertion around log so the test verifies
the full message precisely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 55c4f0e1-680b-4f60-8d88-adc678bacf5d
📒 Files selected for processing (9)
src/hooks/codex/wiki-worker.tssrc/hooks/cursor/wiki-worker.tssrc/hooks/hermes/wiki-worker.tssrc/hooks/pi/wiki-worker.tssrc/hooks/wiki-offset.tssrc/hooks/wiki-worker.tstests/claude-code/wiki-worker.test.tstests/codex/codex-wiki-worker.test.tstests/shared/wiki-offset.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/shared/wiki-offset.test.ts
- src/hooks/pi/wiki-worker.ts
- tests/claude-code/wiki-worker.test.ts
- src/hooks/cursor/wiki-worker.ts
- src/hooks/wiki-worker.ts
- src/hooks/codex/wiki-worker.ts
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
khustup2
approved these changes
Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Stop hook's background wiki-summary worker crashed on long sessions (reported on a ~4480-event Codex session):
ENOBUFS(the agent's stdout exceeded Node's 1 MB defaultexecFileSyncbuffer) andETIMEDOUT(120s) because the worker re-fed the entire session to the summarizer on every run.Root cause: the offset that should bound each run to only the new rows was never applied — no
rows.slice()existed, so the full session was always written to the JSONL; and the offset value was parsed by regex from the LLM-generated summary text (fragile) instead of the reliable sidecar counter. When the summary failed,finalizeSummarynever ran, so the offset stayed at 0 and every run reprocessed everything.Fix (all 5 workers: claude-code, codex, cursor, hermes, pi)
maxBuffer: 64 MBon the summarizerexecFileSync→ eliminates ENOBUFS (the summary is written to a file, not read from stdout).readState().lastSummaryCount), with the regex on the stored summary kept only as a cross-machine fallback.rows.slice(prevOffset)→ only new rows reach the agent; the run is skipped entirely when nothing is new.src/hooks/wiki-offset.ts:stampOffset— the worker writes the offset into the summary itself, removing the dependency on the LLM echoing a bookkeeping line.capLinesByBytes— 4 MB tail cap safety net (keeps the newest rows).execSucceededguard to cursor/hermes/pi/claude-code (codex already had it) so a failed run on a resumed session never advances the offset past rows that were never summarized.OpenClaw is N/A (pure HTTP/WebSocket gateway — does not run the wiki-worker daemon). MCP/Cowork reuses the base worker.
Known tradeoff
When a single increment exceeds 4 MB (degenerate case: first summary of an already-huge backlog with offset 0, or a lone giant row) the byte cap keeps the newest rows and permanently skips the oldest ones (logged, not silent). Intentional — documented in the
capLinesByBytesdoc comment. In normal incremental operation increments are tiny and nothing is ever dropped.Tests
tscclean; 77 worker/helper tests pass. New coverage: offset slicing feeds only new rows, byte-cap keeps the tail and reports drops, no-new-rows skip,maxBufferpresence, and the failure guard that prevents offset advance (data loss) on a failed resumed run.Summary by CodeRabbit