ci: a job stuck with no runner no longer cancels jobs running on a mini - #16479
Conversation
Fails on main: the rescue cancels the whole cmux-next run, including a swift test already running on a mini (#16463). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rescue cancels the whole run to move a job that never got a runner, so a sibling already running on a mini died with it and re-ran on Blacksmith (#16463, #16468). The stuck job now waits until no job of the run is running on a persistent runner; if the watch ends first it stays queued for the mini. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughAssessment now defers rescue of a stuck queued job while another job in the run is active on an owned runner outside setup. Tests cover the wait, subsequent rescue, and cases where refusals or setup-waiting jobs remain actionable. ChangesOwned-pool rescue
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change makes owned-pool rescue wait for a running sibling job before cancelling a stuck queued job. No merge-blocking risk was identified; the author documents the trade-off that some rescues may wait or remain queued. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change protects active persistent-runner jobs without expanding cancellation privileges or weakening run eligibility checks. No introduced security finding was established. Recovery after watch timeout remains partly unverified because the cmux-next workflow that supplies the recovery marker was unavailable. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
Full details: Description checkExplanation The description provides detailed problem, behavior, trade-offs, and test results. However, it does not follow the repository template because it omits the required Summary, Testing, Changelog, Demo Video, and Checklist sections. Resolution Restructure the description using the repository template. Add explicit Summary and Testing sections, include a Changelog line such as "none" for this internal CI change, address the Demo Video requirement or explain why it does not apply, and complete the Checklist with the review and testing status.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
The hold for a stuck job returned before the refused and held-in-setup checks, so a refusal next to a running mini job was never re-run and a held job was not rescued at the watch's end. Judge those first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
teamleaderleo
changed the title
|
Review: two independent subagent reviews. ae11ab6: fix first. The new hold returned before the held-in-setup and refused checks. A refusal next to a running mini job was never re-run, and a held job wasn't rescued at the watch's end. 8370637 (exact head): ship. It checked:
Fixed:
Left: the held-at-end and refused paths can still cancel a job running on a mini, by design. Neither is a job that never got a runner. |
CI failure attributionCI failed on
Not re-run automatically: Written by |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/ci/owned_pool_rescue.py">
<violation number="1" location="scripts/ci/owned_pool_rescue.py:582">
P2: A stuck job held back by a mini-running sibling has no recovery path once the watch's deadline passes. The watch loop returns "stop: watch limit reached" with nothing cancelled, and later sweepers skip this run as soon as its marker passes SWEEP_MAX_AGE_SECONDS (150 min; sweep() drops marked runs whose `created` is older than `oldest`). If the pool stays busy past that point the shard waits indefinitely and is never rescued or re-run. The narrower new case this hold also enables: if the run reaches "completed" while the held job is still queued with no runner (a third sibling fails, or a newer push/concurrency cancels the run), the loop's `finished and not any(refused(job)...)` stop fires and the queued shard ends up cancelled without any re-run — an outcome the "assessed before this hold" carve-outs (refused, held-in-setup) don't cover, and one a plain budget rescue previously prevented. Consider letting a held stuck job survive the deadline (re-adopt in-progress runs regardless of marker age, or treat the hold like the refusal path and finish it within a grace window), so a sibling that finishes late still gets its shard rescued.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if turned_away: | ||
| names = ", ".join(sorted(str(job.get("name") or job.get("id")) for job in turned_away)) | ||
| return Look("refused", f"{names} refused by {job_pool(turned_away[0])} at job start or Xcode selection") | ||
| if stuck: |
There was a problem hiding this comment.
P2: A stuck job held back by a mini-running sibling has no recovery path once the watch's deadline passes. The watch loop returns "stop: watch limit reached" with nothing cancelled, and later sweepers skip this run as soon as its marker passes SWEEP_MAX_AGE_SECONDS (150 min; sweep() drops marked runs whose created is older than oldest). If the pool stays busy past that point the shard waits indefinitely and is never rescued or re-run. The narrower new case this hold also enables: if the run reaches "completed" while the held job is still queued with no runner (a third sibling fails, or a newer push/concurrency cancels the run), the loop's finished and not any(refused(job)...) stop fires and the queued shard ends up cancelled without any re-run — an outcome the "assessed before this hold" carve-outs (refused, held-in-setup) don't cover, and one a plain budget rescue previously prevented. Consider letting a held stuck job survive the deadline (re-adopt in-progress runs regardless of marker age, or treat the hold like the refusal path and finish it within a grace window), so a sibling that finishes late still gets its shard rescued.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/ci/owned_pool_rescue.py, line 582:
<comment>A stuck job held back by a mini-running sibling has no recovery path once the watch's deadline passes. The watch loop returns "stop: watch limit reached" with nothing cancelled, and later sweepers skip this run as soon as its marker passes SWEEP_MAX_AGE_SECONDS (150 min; sweep() drops marked runs whose `created` is older than `oldest`). If the pool stays busy past that point the shard waits indefinitely and is never rescued or re-run. The narrower new case this hold also enables: if the run reaches "completed" while the held job is still queued with no runner (a third sibling fails, or a newer push/concurrency cancels the run), the loop's `finished and not any(refused(job)...)` stop fires and the queued shard ends up cancelled without any re-run — an outcome the "assessed before this hold" carve-outs (refused, held-in-setup) don't cover, and one a plain budget rescue previously prevented. Consider letting a held stuck job survive the deadline (re-adopt in-progress runs regardless of marker age, or treat the hold like the refusal path and finish it within a grace window), so a sibling that finishes late still gets its shard rescued.</comment>
<file context>
@@ -566,6 +579,9 @@ def assess(jobs: Sequence[Mapping[str, Any]], *, now: dt.datetime, budget_second
if turned_away:
names = ", ".join(sorted(str(job.get("name") or job.get("id")) for job in turned_away))
return Look("refused", f"{names} refused by {job_pool(turned_away[0])} at job start or Xcode selection")
+ if stuck:
+ return Look("watch", f"{names} queued on {job_pool(stuck[0])} with no runner, but "
+ f"{len(on_mini)} job(s) of the run are running on a persistent runner", waiting=True)
</file context>
|
Merge receipt for
Labeled |
github-actions
Bot
added
the
merged-unverified
984179e fix: two more crossed-merge compile breaks on main (sidebar test seam, Cloud agent launch) (manaflow-ai#16490) b1b876c fix(ios): prevent composer shortcut strip edge snapping (manaflow-ai#16128) 337861c ci: keep janitor sweeps green on refused cancellations (manaflow-ai#16488) e0dc415 fix(ci): install Go before iOS App Store archive (manaflow-ai#16486) c247a88 rename the duplicate node options resume test so ci's selector check passes (manaflow-ai#16481) bbb73b6 ci: a job stuck with no runner no longer cancels jobs running on a mini (manaflow-ai#16479) 28ed45d fix(cli): repair main seed compile errors (manaflow-ai#16485) # Conflicts: # .github/workflows/ios-appstore-upload.yml
1 participant
The owned-pool rescue cancels a whole run when one of its jobs has waited past its budget with no runner, then re-runs the failed and cancelled jobs on Blacksmith. Since #16435 it also covers cmux-next.yml runs. On #16463 and #16468, release-compile waited for a mini while swift-test was already running on one. The rescue cancelled the attempt, which killed a swift test three minutes into its run on a mini and moved it to Blacksmith as attempt 2. That works against minis-first.
Now
assess()holds a stuck job while any job of the run is running on a persistent runner (an owned label, in progress, past glaeda's setup hook). The look stayswatchwithwaiting=True:Refused jobs and jobs held in setup are judged before this hold. A refusal next to a running mini job still gets its failed jobs re-run (at once on main, otherwise when the run finishes or at the watch's end). A job held in setup is still rescued at the watch's end. Those two paths can still cancel a job running on a mini. They aren't jobs that never got a runner, so they're outside this change.
Trade-offs:
Both follow from minis-first.
Tests:
test_a_stuck_side_job_never_cancels_a_sibling_running_on_a_mini, in its own first commit, fails on main with'cancel' unexpectedly found.test_a_sibling_on_a_mini_does_not_hide_a_refusal_or_a_held_jobcovers the ordering. It came from the exact-head review of ae11ab6, and failed there with'watch' != 'refused'.python3 tests/test_ci_owned_pool_rescue.py: 121 tests OK.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Stops the owned-pool rescue from cancelling a sibling job already running on a mini when another job of the same run is stuck waiting for a runner. The rescue previously cancelled the whole run and re-ran failed and cancelled jobs on Blacksmith, which killed in-flight mini jobs (e.g. a cmux-next swift test three minutes into its run). A stuck job now waits while any sibling runs on a persistent runner: if that job finishes within the watch, the rescue cancels only the stuck one; if the watch ends first, nothing is cancelled and the job stays queued for the mini that frees up. This applies to every watched workflow. Refusals and jobs held in glaeda's setup hook are still acted on first, so a sibling on a mini no longer hides them.
Written for commit 8370637. Summary will update on new commits.
Summary by CodeRabbit