fix(tests): model watchdog restart lifecycle - #647
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@coderabbitai review |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c9d83052-2f89-47f7-8c1e-3d45125faadc) |
|
@codex review |
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
✅ Action performedReview finished.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe throughput watchdog test runner now simulates watcher lifecycle transitions. Status checks report the current PID and state. SIGKILL stops the watcher. Kickstart assigns a new PID and restores the running state. ChangesWatchdog lifecycle testing
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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 `@tests/test_throughput_watchdog.py`:
- Around line 723-725: Strengthen the kickstart test around the launchctl
handling branch by recording process["pid"] before each replacement and
asserting it differs afterward. Update the assertions in the surrounding
watchdog test so removing the process["pid"] += 1 behavior would fail, while
preserving the existing running-state checks.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6a1aa1af-02bc-4068-999b-4fd2a34131d3
📒 Files selected for processing (1)
tests/test_throughput_watchdog.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🧰 Additional context used
📓 Path-based instructions (1)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Runpytestbefore claiming behavior changes are safe.
Tests must not refresh the production backup heartbeat log; useBRAINLAYER_BACKUP_LOG_PATHandBRAINLAYER_BACKUP_LOG_PROVENANCE=pytest.
Files:
tests/test_throughput_watchdog.py
🔇 Additional comments (1)
tests/test_throughput_watchdog.py (1)
712-725: 🎯 Functional CorrectnessRun the focused pytest check before approval.
This file matches
tests/**/*.py. Run the test with the required test-only backup-log settings before claiming the behavior is safe:tmpdir="$(mktemp -d)" trap 'rm -rf "$tmpdir"' EXIT export BRAINLAYER_BACKUP_LOG_PATH="$tmpdir/backup-heartbeat.log" export BRAINLAYER_BACKUP_LOG_PROVENANCE=pytest pytest -q tests/test_throughput_watchdog.py -k 'test_alert_framing_pages_once_per_episode_not_per_kickstart'Source: Coding guidelines
| elif args[:3] == ["launchctl", "kickstart", "-k"]: | ||
| process["pid"] += 1 | ||
| process["running"] = True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert the replacement PID.
process["pid"] += 1 models a replacement process, but the test never checks it. If Line 724 is removed, the assertions at Lines 747-764 still pass. Record the PID before each kickstart and assert that it changes afterward.
🤖 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/test_throughput_watchdog.py` around lines 723 - 725, Strengthen the
kickstart test around the launchctl handling branch by recording process["pid"]
before each replacement and asserting it differs afterward. Update the
assertions in the surrounding watchdog test so removing the process["pid"] += 1
behavior would fail, while preserving the existing running-state checks.
The 7 admin merges of 2026-08-04 (#645 #644 #643 #641 #633 #605 #602) bypassed CI, landing import-order and formatting drift on main. Every PR's lint job inherits it. Rebased onto #647 (949334a), which cleared the watchdog test blocking this push. ruff check --fix + ruff format only; no behavioural change.
1 participant
Summary
Root cause
The test's fake
launchctl printreturnedstate = runningwith PID 4321 forever, including after the mocked SIGKILL. Production correctly refused to kickstart while that PID appeared alive, waited the real 45-second safety timeout, and returnedrecovery_failed. This reproduces alone on cleanb95e113d, so it is a real #602 test regression rather than an item-24 concurrency artifact.Verification
watcher pid 4321 survived SIGKILL for 45sBrainLayer test gate passed; isolated hook-routing leg 40 passed, Bun leg 1 passed, FTS5 determinism shell regression passed9f0795703b5074c77edaa7729ff9dd597d0a3b9aReview notes
Known base-branch state
Repo-wide Ruff is still red only in the three files already fixed by the separate, unlanded
7214eec5commit. This PR does not duplicate or reformat that work.Worker endpoint is PR plus review responses. Do not merge from this lane.
Note
Low Risk
Single test mock change only; no production watchdog or recovery logic modified.
Overview
Test-only fix for
test_alert_framing_pages_once_per_episode_not_per_kickstart, which was timing out (~45s) because its fakelaunchctl printalways reported PID 4321 as running, even after a mocked SIGKILL—matching production behavior that blocks kickstart until the old PID is gone.The test’s
command_runnernow keeps a small process state:launchctl printreflects running vs exited and the current PID,/bin/kill -9marks the process dead, andlaunchctl kickstart -kbumps the PID and marks it running again. Alert-framing expectations (one page per episode, suppression within an episode, re-page after sustained health) are unchanged; only the mock matches the recovery sequence other tests already assert.Reviewed by Cursor Bugbot for commit 9f07957. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix watchdog restart lifecycle simulation in
test_alert_framing_pages_once_per_episode_not_per_kickstartThe test's
command_runnermock previously returned a fixedrunningstate and pid regardless of commands issued. It now tracks process state across the full lifecycle:launchctl printreturnsstate = runningorstate = exitedbased on current state,/bin/kill -9 <pid>sets the process to stopped, andlaunchctl kickstart -kincrements the pid and marks the process as running again.Macroscope summarized 9f07957.
Summary by CodeRabbit