Repository navigation
fix: protect presenter launch reconciliation - #361
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRun provisioning now records an owner process and phase. Launch reserves ownership before agent startup, validates durable transitions, reconciles snapshots, preserves live provisioning runs, and cleans up workspaces after launch races or failures. ChangesProvisioning lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunModule
participant Store
participant Agent
participant Workspace
RunModule->>Store: reserve provisioning ownership
RunModule->>Agent: start agent
Agent-->>RunModule: return launch result
RunModule->>Store: verify generation and transition to running
RunModule->>Workspace: close workspace after failed transition
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v2/src/run/index.test.ts (2)
522-576: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression for a failing agent launch.
This test covers the lost durable transition, where
fail()already clearedprovisioningOwner. It does not coverlaunchAgentthrowing while the record staysprovisioning. That path leaves the reservation in thelaunchingphase, as noted on v2/src/run/index.ts Lines 367-377. Add a test that makes agent startup fail, then asserts that a secondlaunchcall does not throwis already launching.🤖 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 `@v2/src/run/index.test.ts` around lines 522 - 576, Add a regression test alongside the existing lost-transition test that forces agent startup in launch to throw while the run record remains provisioning, then verifies a subsequent launch succeeds without an “is already launching” error. Use the existing RunModule launch setup and failure-injection fixture mechanisms, and assert the reservation is no longer stuck in the launching phase.
399-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared presenter fixture setup.
The five affected tests repeat the temporary state root, presenter state files, fake-bin
PATH, and environment setup. Use one helper for the common fixture and preserve test-specific environment variables.🤖 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 `@v2/src/run/index.test.ts` around lines 399 - 424, Extract the repeated temporary presenter fixture setup from the affected tests into a shared helper, including the state root, presenter state files, fake-bin PATH, and base environment. Update each test to use the helper while merging its test-specific environment variables, and preserve the existing RunModule setup and behavior.Source: Coding guidelines
🤖 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 `@v2/src/run/index.ts`:
- Around line 331-377: In v2/src/run/index.ts lines 331-377, update the
launchAgent catch path to conditionally reset provisioningOwner to the preparing
phase when the runId, provisioning state, and provisioning generation still
match, before closing the workspace and rethrowing. In v2/src/run/index.test.ts
lines 522-576, add coverage that forces agent startup failure and verifies a
subsequent launch does not throw “is already launching”.
- Around line 1185-1196: Update provisioningOwnerIsAlive and the
provisioning-owner identity data it consumes to validate more than PID liveness:
persist and compare the OS boot identifier, plus a platform-specific
process-start identity to detect same-boot PID reuse. Return false when any
recorded identity does not match the current process, while preserving the
existing ESRCH handling. Do not limit startedAt comparison to owners whose
processId equals process.pid.
---
Nitpick comments:
In `@v2/src/run/index.test.ts`:
- Around line 522-576: Add a regression test alongside the existing
lost-transition test that forces agent startup in launch to throw while the run
record remains provisioning, then verifies a subsequent launch succeeds without
an “is already launching” error. Use the existing RunModule launch setup and
failure-injection fixture mechanisms, and assert the reservation is no longer
stuck in the launching phase.
- Around line 399-424: Extract the repeated temporary presenter fixture setup
from the affected tests into a shared helper, including the state root,
presenter state files, fake-bin PATH, and base environment. Update each test to
use the helper while merging its test-specific environment variables, and
preserve the existing RunModule setup and behavior.
🪄 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: CHILL
Plan: Pro
Run ID: 85975874-2a69-4348-b6a2-a93301107f7a
📒 Files selected for processing (2)
v2/src/run/index.test.tsv2/src/run/index.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ClipboardHealth/cbh-core(manual)
Review-body findingsMendral reported LGTM and raised no actionable finding. Agree
Disagree
🤖 |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
LGTM
The new reservation-release logic at v2/src/run/index.ts:369-384 is correctly guarded by the same CAS predicates (runId, state, generation) used elsewhere, ensuring it's a no-op if the run has already moved on. The test exercises the full retry path through a real RunModule instance. No new issues introduced.
What this PR does
Adds a launch-reservation release in the failure path: when launchAgent or the durable "running" transition fails, the provisioningOwner phase is CAS-reset from "launching" back to "preparing" before closing the presented workspace. This allows a subsequent retry (same or new process) to re-acquire the launch reservation. Includes a regression test verifying end-to-end retry after a presenter open failure.
Tag @mendral-app with feedback or questions. View session
1 participant
Why
Concurrent Groundcrew CLI processes can reconcile a healthy provisioning run against an empty or stale cmux snapshot while the owning process is preparing or launching it. That can durably fail a healthy run or leave a live agent workspace untracked. This has no direct customer impact; it improves dispatch reliability and prevents operator cleanup work.
Summary
Validation
Notes
Findings: fnd_sig-feat-cli-command-012418b101-_c2c978fbb3 and fnd_sig-feat-cli-command-49a9766219-_65990038f3.
Agent session:
codex resume 019fecf3-bf31-73b2-92d8-01afb9524089🤖
cb-ship:created v1 skill@1.0.2