fix(summary): back off after a failed summary run instead of respawning per event - #332
Conversation
A summary run that fails never reaches finalizeSummary: wiki-worker.ts returns early when `claude -p` exits non-zero or writes no summary file, then releases the spawn lock in its `finally`. lastSummaryCount therefore stays 0, and shouldTrigger's first-summary clause (lastSummaryCount === 0 && totalCount >= 10) keeps evaluating true on every subsequent captured event — one full `claude -p` process per tool call for the rest of the session. Track attempts separately from successes: markSummaryAttempt stamps lastAttemptAt + attemptsSinceSuccess before a run is spawned, finalizeSummary resets the counter, and shouldTrigger holds off for an exponential window (1 -> 30 minutes) while a failure is outstanding. Refs #331
All four periodic triggers (claude_code, codex, cursor, hermes) win the spawn lock and then spawn without recording that a run started. Call markSummaryAttempt between the two so a run that fails is visible to shouldTrigger's back-off instead of being invisible bookkeeping. Refs #331
The regression test replays the real loop — bump the counter per event, spawn when shouldTrigger fires, never finalize — over 30 events. With the back-off removed it produces 21 spawns; with it, 1. Also pins the back-off schedule (1/2/4/8/16 min, capped at 30), that a clean session is unaffected, that markSummaryAttempt does not advance lastSummaryCount, and that finalizeSummary clears the failure counter. The capture-hook suites mock summary-state with explicit factories, so markSummaryAttempt has to be declared there or the trigger throws and silently skips the spawn; they now also assert it fires before the spawn.
HIVEMIND_SUMMARY_EVERY_N_MSGS, HIVEMIND_SUMMARY_EVERY_HOURS, HIVEMIND_WIKI_WORKER and HIVEMIND_GRAPH_ON_STOP all exist in the bundle but were undocumented, so the only remedy a user could find for background worker load was disabling the whole plugin. Refs #331
markSummaryAttempt ran between tryAcquireLock and the spawn try/catch, so a throwing state write left the spawn lock held until the 10-minute stale reclaim — the session would stop summarizing for that whole window. Found by codex review.
The pi extension ships as standalone TypeScript with its own copy of the summary-state logic, so it kept the infinite-refire behavior after a failed run. It is also the only writer that rewrites the whole state object read from the shared dir, so without the new fields in its read path it would strip them from state the shared worker finalizes against. Found by codex review.
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 5 files changed
Generated for commit 2719ec7. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPeriodic summary execution now records failed attempts, applies capped exponential retry backoff, stamps attempts before worker spawning across capture paths, and resets failure tracking after successful summaries. Tests cover persistence, ordering, lock handling, and trigger behavior. ChangesPeriodic summary backoff
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CaptureHook
participant SummaryState
participant WikiWorker
CaptureHook->>SummaryState: Check shouldTrigger
SummaryState-->>CaptureHook: Allow or suppress retry
CaptureHook->>SummaryState: Record markSummaryAttempt
CaptureHook->>WikiWorker: Spawn periodic summary worker
WikiWorker->>SummaryState: Finalize successful summary
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
…hooks Only claude_code and codex have functional trigger tests; cursor and hermes never mock summary-state, so their copies of maybeTriggerPeriodicSummary are unreachable from a functional test and showed up as the branch-coverage dip on this PR. Pins the two ordering properties a refactor can silently break: the stamp lands before the spawn, and inside the try that releases the lock. Verified against both regressions — removing the stamp fails the first assertion, moving it outside the try fails the second.
The previous commit stamped in maybeTriggerPeriodicSummary, before spawnWikiWorker acquires the summary lock, and wrote back the whole state object read at trigger time. Two consequences: a concurrent worker that finalized in between would have its success overwritten by stale counters, and a spawn suppressed by the lock was still recorded as a failed attempt. Move the stamp inside spawnWikiWorker, after the lock is won, and build it from a fresh read. Periodic only — the final session-shutdown summary gets no back-off, matching summary-state.ts. Found by codex review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Line 324: Update the Pi extension’s background session-summary worker path in
hivemind.ts to check HIVEMIND_WIKI_WORKER and skip spawning periodic workers
when it is set to 1, while preserving existing HIVEMIND_CAPTURE behavior and
capture/recall functionality.
🪄 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: 75a3ecdb-87d8-4aad-b0ab-3074ec0afdd8
📒 Files selected for processing (12)
README.mdharnesses/pi/extension-source/hivemind.tssrc/hooks/capture.tssrc/hooks/codex/capture.tssrc/hooks/cursor/capture.tssrc/hooks/hermes/capture.tssrc/hooks/summary-state.tstests/claude-code/capture-hook.test.tstests/claude-code/summary-state.test.tstests/codex/codex-capture-hook.test.tstests/pi/pi-extension-source.test.tstests/shared/summary-attempt-stamp-source.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@harnesses/pi/extension-source/hivemind.ts`:
- Around line 829-844: Update the periodic summary flow around readSummaryState,
writeSummaryState, and spawnWikiWorker to fail closed: if state cannot be read
or the post-lock attempt stamp cannot be persisted, release the acquired summary
lock and abort without spawning the worker. Ensure write failures are detected
rather than swallowed, while preserving normal spawning only after successful
state update.
🪄 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: 22f6ed47-c27e-4c23-aa4f-894a87a86fad
📒 Files selected for processing (2)
harnesses/pi/extension-source/hivemind.tstests/pi/pi-extension-source.test.ts
…witch Two review findings, both on the pi extension: writeSummaryState swallowed every write error, so a stamp that never reached disk still let the worker spawn — and the next captured event refired immediately, which is the exact behavior the back-off exists to prevent. It now reports failure, and the periodic path releases the lock and skips the spawn when the state cannot be read or the stamp cannot be persisted. The README documents HIVEMIND_WIKI_WORKER=1 as disabling background summaries, but pi only checked HIVEMIND_CAPTURE and kept spawning periodic workers. Honor it in the trigger, which doubles as the same recursion guard the bundled hooks use. Found by CodeRabbit; the first also matches codex's "pi's stamp is best-effort" finding.
|
Both findings fixed in c2ef660. Major — fail closed on an unpersisted stamp. Minor — the documented off switch. You're right that the README over-promised. Rather than narrow the docs, pi now honors Covered by three new assertions in On docstring coverage: not addressed. The 40% figure is dominated by pre-existing functions in the files this PR touches, and adding docstrings to unrelated functions would inflate the diff. |
…e lock Two blocking review findings. HIVEMIND_WIKI_WORKER=1 only guarded the periodic trigger, so session_shutdown still spawned the final worker — the README line I added in this PR promises it disables background summaries entirely, which was false. Move the guard into spawnWikiWorker, ahead of the lock, so it covers both reasons and a disabled worker never takes a lock. The lock was released on stamp failure but not on config-write failure, sync spawn failure, or the async 'error' event that carries ENOENT/EPERM. pi's tryAcquireSummaryLock has no stale reclaim, unlike summary-state.ts, so any one of those held the lock for the rest of the session and silently skipped every later periodic and final spawn. Found by codex review.
1 participant
Problem
Reported in #331: on Windows a headless
claude -pspawns roughly every 5-6 seconds during normal tool work, flashing a console window, burning model API calls, and holding Defender at ~19.6% CPU. #256 hid the window; this fixes the spawning.The reporter attributed it to "one spawn per capture event by design". It isn't — the periodic trigger is gated at 50 events / 2 hours (
summary-state.ts), and the spawn lock is held for the duration of a run, so a healthy 15-60s summary run physically cannot re-spawn at that cadence.It's a hot loop on the failure path:
wiki-worker.tsswallows a failingclaude -pand setsexecSucceeded = falsefinalizeSummaryis never calledfinallyreleases the spawn lock immediatelylastSummaryCountis therefore still0, soshouldTrigger's first-summary clause (lastSummaryCount === 0 && totalCount >= FIRST_SUMMARY_AT) stays true foreverOnce any summary run fails, every subsequent captured event spawns a fresh
claude -pfor the rest of the session. One missing counter update produces all three symptoms.Fix
Track attempts separately from successes:
markSummaryAttemptstampslastAttemptAt+attemptsSinceSuccessbefore a run is spawnedfinalizeSummaryresets the counter on successshouldTriggerholds off for an exponential window (1, 2, 4, 8, 16, capped at 30 minutes) while a failure is outstandingWired into all five periodic triggers: claude_code, codex, cursor, hermes, and the standalone pi extension.
A healthy session is unaffected — a successful run clears the counter before the next trigger is ever evaluated.
Verification
The regression test replays the real loop (bump per event, spawn when
shouldTriggerfires, never finalize) over 30 events. It fails against the unfixed code, which is the point:Also covered: the back-off schedule, that a clean session is unaffected, that
markSummaryAttemptdoes not advancelastSummaryCount, thatfinalizeSummaryclears the counter, and that a throwing stamp releases the spawn lock.tsc --noEmitclean. Three pre-existing failures in the full suite (flush-memoryneeds network,cli-bundle-runtimeneeds a built bundle,cli-updatefails only under parallel load and passes 35/35 alone) reproduce identically onmainwithout this branch.Also here
Documents
HIVEMIND_SUMMARY_EVERY_N_MSGS,HIVEMIND_SUMMARY_EVERY_HOURS,HIVEMIND_WIKI_WORKERandHIVEMIND_GRAPH_ON_STOP, which all existed in the bundle but were undocumented — issue suggestion #4. Without them the only discoverable remedy was disabling the whole plugin.Review notes
Codex reviewed this branch and confirmed the root cause against the source. Two of its findings are fixed here (the lock leak when the stamp throws; the pi extension keeping the old behavior and stripping the new fields from shared state). Three it raised are pre-existing and deliberately not in scope:
SessionEndskips when a periodic worker holds the lock, so a final summary can still be lost if that worker then failsfinalizeSummaryleaves the session retrying; with this change that is a capped 30-minute cycle instead of a per-event oneNot verified on Windows
The mechanism is confirmed by reading the code and by the simulated-loop test, but nobody has run this on a real Windows box. Worth asking @jzferrell26 to re-test once this and #256 ship rather than closing #331 on our own say-so.
Refs #331
Summary by CodeRabbit
New Features
Bug Fixes
Tests