fix: unflake the concurrency test; escalate a livelock reroute that stopped working (#30) - #37
Merged
Merged
Conversation
CI failed at 964ms against a 250ms ceiling. That ceiling had already been raised once — from 180ms, after it flaked at 182ms — and raising it again does not work: any threshold generous enough to survive a loaded runner is also generous enough to pass a fully serial run, whose baseline is 200ms. The test would keep passing and stop meaning anything. So it no longer times the loop. `_TrackingHost` counts dispatches in flight and the test asserts the peak: greater than 1 (they genuinely overlapped) and no greater than the configured concurrency. Immune to runner load, and strictly stronger than the duration check — it also catches *over*-concurrency, which elapsed time never could, plus a dispatch leaking past the end of the run. Verified by mutation: with `concurrency=1` the new assertion fails with "dispatches ran serially — concurrency had no effect". The old assertion would have passed that same mutation on a fast machine, which is the whole problem. The `asyncio.sleep(0.05)` stays — it creates the window in which overlap can be observed — but no assertion depends on how long it takes. 1498 tests pass; ruff, pyrefly, and `nightly verify` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Issue #30 reports involuntary mid-chain stops that leave only a RESPAWN_REQUESTED marker. Its attached keepalive log contains the cause: **130 consecutive identical livelock reroutes**, one every 10-30 seconds, context growing ~1700 tokens per turn. The agent emits a sentence and stops; the hook blocks and injects byte-identical text; repeat. That spin is not merely unproductive — it is the proximate cause of the reported symptom. Burning the host's without-progress budget on 130 no-op turns is what gets the session killed. The v0.0.11 reroute detects a repeated pick but never notices that its own remedy has failed, so it re-sends an instruction that has demonstrably not worked, indefinitely. After `_LIVELOCK_REPICKS + _LIVELOCK_ESCALATE_AFTER` reroutes for the same pick, the injected prompt changes instead of repeating. It names the specific stuck pick — the generic text never said *which* one — and gives the agent a way to discharge it: 1. Verify the deliverable exists (rule 13's pre-flight check). 2. If it does, tick the box and commit the reconciliation. 3. If it is unactionable for another reason, record why. 4. Only then pick different work. Step 2 matters most. "Treat it as `nothing`" told the agent to ignore an item it believed was real, without offering any action that would make it go away — so the next boundary produced the same pick. A reconciliation commit is real work *and* it removes the pick from the cascade. The log line now distinguishes the escalated case, so this is diagnosable from `keepalive.log` alone next time. Deliberately unchanged: the session is never released. Only human disk markers terminate a run, and a spinning session is still a session. 1498 tests pass (5 new); ruff, pyrefly, and `nightly verify` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe keepalive hook now escalates repeated livelock reroutes with a verification and reconciliation prompt. Keepalive messages report escalation status. Driver concurrency coverage now measures actual overlapping dispatches instead of elapsed time. ChangesKeepalive livelock escalation
Concurrency test validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StopHook as Stop-hook decision
participant Cascade as Cascade choice
participant Agent
StopHook->>Cascade: inspect repeated eligible pick
StopHook->>Agent: inject escalation verification and reconciliation prompt
Agent-->>StopHook: continue with updated decision
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@packages/nightly-core/src/nightly_core/keepalive_hook.py`:
- Around line 265-274: Update the escalation guidance in the keepalive prompt
construction around the existing reconciliation steps so it branches by pick
source: retain RFC checklist/commit reconciliation for RFC picks, and provide
github_issue picks with an appropriate issue/evidence reconciliation action that
can discharge repeated work without RFC paperwork. In
packages/nightly-core/tests/test_keepalive_context.py lines 293-305, add a
github_issue case and assert the source-appropriate reconciliation guidance.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5d06180-4b44-4f1d-8e39-5fe067a77c42
📒 Files selected for processing (3)
packages/nightly-core/src/nightly_core/keepalive_hook.pypackages/nightly-core/tests/test_driver.pypackages/nightly-core/tests/test_keepalive_context.py
Comment on lines
+265
to
+274
| "1. Verify the pick is genuinely done or not actionable \u2014 check the " | ||
| "symbol/file exists, check `git log` and any unmerged `nightly/*` " | ||
| "branch, check open PRs. (Rule 13, pre-flight verification.)", | ||
| "2. If it IS already done: tick the checklist box and commit " | ||
| "`docs(rfc-NNN): tick <ITEM> \u2014 already implemented in <SHA>`. That " | ||
| "reconciliation is real work and it stops this loop.", | ||
| "3. If it is NOT actionable for another reason: record why in the task " | ||
| "plan or an approval note, so the next turn has evidence instead of " | ||
| "re-deriving it.", | ||
| "4. Only then pick different work.", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make escalation reconciliation source-aware.
The prompt also runs for github_issue picks, but it unconditionally requires an RFC checklist and docs(rfc-NNN) commit. A repeated issue with no RFC cannot follow that discharge path, so it can remain stuck. Use source-neutral guidance or branch by source, then cover both paths.
packages/nightly-core/src/nightly_core/keepalive_hook.py#L265-L274: givegithub_issuepicks an appropriate evidence/issue-reconciliation action instead of requiring RFC paperwork.packages/nightly-core/tests/test_keepalive_context.py#L293-L305: add agithub_issuecase and assert its source-appropriate reconciliation guidance.
📍 Affects 2 files
packages/nightly-core/src/nightly_core/keepalive_hook.py#L265-L274(this comment)packages/nightly-core/tests/test_keepalive_context.py#L293-L305
🤖 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 `@packages/nightly-core/src/nightly_core/keepalive_hook.py` around lines 265 -
274, Update the escalation guidance in the keepalive prompt construction around
the existing reconciliation steps so it branches by pick source: retain RFC
checklist/commit reconciliation for RFC picks, and provide github_issue picks
with an appropriate issue/evidence reconciliation action that can discharge
repeated work without RFC paperwork. In
packages/nightly-core/tests/test_keepalive_context.py lines 293-305, add a
github_issue case and assert the source-appropriate reconciliation guidance.
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
1 participant
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.
Two fixes, both independent of the RFC 007/012 follow-on work still sitting on a local branch.
1. CI:
test_run_loop_concurrency_actually_parallelno longer times the loopThe test asserted
elapsed < 0.25and CI failed at 0.964s. That ceiling had already been raised once — from 0.18s, after a 0.182s flake — and raising it again does not work: any threshold generous enough to survive a loaded runner is also generous enough to pass a fully serial run, whose baseline is 0.2s. The test would keep passing and stop meaning anything.It now measures concurrency structurally.
_TrackingHostcounts dispatches in flight; the test asserts the peak is greater than 1 (they genuinely overlapped) and no greater than the configured concurrency. That is immune to runner load and strictly stronger than the duration check — it also catches over-concurrency, which elapsed time never could, plus a dispatch leaking past the end of the run.Mutation-verified: with
concurrency=1the new assertion fails with "dispatches ran serially — concurrency had no effect". The old assertion would have passed that same mutation on a fast machine, which is the whole problem.2. Issue #30: the livelock reroute now escalates when it stops working
Closes #30.
The issue reports involuntary mid-chain stops leaving only a
RESPAWN_REQUESTEDmarker. Its attached keepalive log contains the cause: 130 consecutive identical livelock reroutes, one every 10-30 seconds, context growing ~1700 tokens per turn. The agent emits a sentence and stops; the hook blocks and injects byte-identical text; repeat.That spin is not merely unproductive — it is the proximate cause of the reported symptom. Burning the host's without-progress budget on 130 no-op turns is what gets the session killed. The v0.0.11 reroute detects a repeated pick but never notices that its own remedy has failed, so it re-sends an instruction that has demonstrably not worked, indefinitely.
After
_LIVELOCK_REPICKS + _LIVELOCK_ESCALATE_AFTERreroutes for the same pick, the injected prompt changes instead of repeating. It names the specific stuck pick — the generic text never said which one — and gives the agent a way to discharge it:Step 2 matters most. "Treat it as
nothing" told the agent to ignore an item it believed was real, without offering any action that would make it go away — so the next boundary produced the same pick. A reconciliation commit is real work and it removes the pick from the cascade.The log line now distinguishes the escalated case, so this is diagnosable from
keepalive.logalone next time.Deliberately unchanged: the session is never released. Only human disk markers terminate a run, and a spinning session is still a session.
Verification
1167 tests pass (5 new).
nightly verifyclean on all five checks: ruff-check, ruff-format, pyrefly, make-lint, make-check.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests