Skip to content

fix(orchestration): refuse Task re-open under a live worker; allow stop re-issue on a stranded row - #19551

Merged
Jinwoo-H merged 1 commit into
mainfrom
lifecycle-stop-guard
Sep 8, 2026
Merged

Jinwoo-H merged 1 commit into
mainfrom
lifecycle-stop-guard

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​122 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​122
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​29 $\color{#cf222e}{\Huge{\mathbf{−}}}$​10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​19

Split out of #19347 (the SSH exit-certification half is being redesigned separately).

What

  • Task re-open guard. updateTaskStatus already refuses completed/failed while a live supervised worker holds the Task. It now runs the same check when a Task is moved back to dispatched from any other status. A no-op re-assert of dispatched stays legal.
  • worker-stop re-issue on a stranded row. beginWorkerStop accepts a stopping row only when its runtime_epoch is not this runtime's, meaning the runtime that started the stop died mid-flight. Same-epoch stays refused so an in-flight stop's exit event is still read as that stop succeeding, not as a crash. Same predicate as failActiveDispatchOnExit.

Why

A task-update --status dispatched under a stopping worker left the Task pointing at a worker whose own lifecycle could no longer settle its report; the throw then rolled back the coordinator's whole check batch. No workflow issues that command, so this is defensive.

The stop re-issue: a runtime dying between marking stopping and closing the terminal leaves the row stranded. Main already has one escape (worker-show marks a foreign-epoch stopping row stop_unknown, then worker-abandon works); this adds the direct one, where re-running worker-stop actually attempts the close and lands on an honest stopped or stop_unknown.

Not included

The no-throw settlement path, isLegalLifecycleTransition, worker_not_settleable, and the state-space enumeration test from #19347. With the guard in place the bad triple is unreachable, and no shipped database realistically holds it.

Review note

CodeRabbit asked to fence settleWorkerStop/markWorkerStopUnknown by epoch against a re-issue racing the old runtime's pending settle. The runtime id is a per-process UUID; a new epoch exists only after the old process is gone, so its pending settle cannot run. No change.

Verification

  • db-stopping-worker-task-guard.test.ts: 7 cases. A/B on unpatched main: 3 fail (both guard cases, the re-issue), 4 controls pass. 7 of 7 with the fix.
  • Orchestration + RPC + CLI orchestration suites: 1,800 tests pass. tc:node, changed-code quality (0 findings), max-lines ratchet, git diff --check.

Source: 3 files, +29 / -10.

@pullfrog

pullfrog Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

This run was cancelled 🛑

The workflow was cancelled before completion. Please check the link below for details.

Pullfrog  | View workflow run | via Pullfrog | 𝕏

@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

📝 Walkthrough

Walkthrough

updateTaskStatus now rejects reopening a task while an active supervised worker is stopping or has an unknown stop outcome. Valid dispatched transitions and status reassertions remain accepted. beginWorkerStop now permits recovery of stopping workers stranded by another runtime epoch. Same-runtime stop reissues remain rejected. Tests cover task status guards, cross-runtime recovery, same-runtime rejection, and stop settlement.

Merge Risk: 🟡 Moderate · up to e9ded

Cross-runtime recovery can prevent stranded worker stops, but a late result from the prior runtime may overwrite the replacement stop's outcome and leave worker lifecycle state incorrect. Fence stop settlement and unknown outcomes by runtime epoch before merging.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the changes, rationale, exclusions, and verification results, but it omits several required template sections, including ELI5, Linked Issue in the required format, Visual Proo… Add all missing template sections. Provide the required issue link using Fixes #..., state N/A with a reason for Visual Proof if there is no UI or interaction change, complete the Testing and checklist items, and address AI disclosure, …
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes both primary changes: preventing Task re-opening under a live worker and allowing stop re-issue for stranded rows.
Full details: Description check

Explanation

The description explains the changes, rationale, exclusions, and verification results, but it omits several required template sections, including ELI5, Linked Issue in the required format, Visual Proof or N/A, Testing checklist, AI Disclosure, Review, Agent skill upstream boundary, Notes, and the main Checklist.

Resolution

Add all missing template sections. Provide the required issue link using Fixes #..., state N/A with a reason for Visual Proof if there is no UI or interaction change, complete the Testing and checklist items, and address AI disclosure, review, upstream-boundary, compatibility, security, and performance requirements.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 4db8c16b-e62b-4d3b-8cd9-de8f7b72b953

📥 Commits

Reviewing files that changed from the base of the PR and between 15adbd9 and e9deda1.

📒 Files selected for processing (4)
  • src/main/runtime/orchestration/db-stopping-worker-task-guard.test.ts
  • src/main/runtime/orchestration/db/tasks/task-status-transition.ts
  • src/main/runtime/orchestration/db/worker-dispatch/worker-dispatch-stop.ts
  • src/main/runtime/rpc/methods/orchestration/worker/worker-stop.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

@Jinwoo-H
Jinwoo-H merged commit d346d6f into main Sep 8, 2026
51 of 77 checks passed
nwparker added a commit that referenced this pull request Sep 8, 2026
…e main static analysis

The 7 auto.components.settings.TerminalPane.minimumContrast.* entries added by #18126 have zero call sites; the shipped component reads settings.contrast.*. Because the runtime-required catalog classifies any key with no literal-default call site as required, the orphans broke 'Verify runtime-required localization catalog' on main and red-lit every PR in the repo.

Deleting them is the root-cause fix: regenerating would instead add dead strings to the boot bundle. On main+delete, --fix regenerates a byte-identical catalog and the CI step exits 0.

Merged with 'test / tests node 24 3/8' red: that failure is an unrelated main break from the #19542/#19551 collision, not from this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant