Repository navigation
fix(runs): replay worker retries only for the writing lease; fence retention handoffs - #21634
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Bead |
|
On hold behind the same |
|
Blocked: it needs the run owner (the Huge Blue run lane, or Leo) to decide whether a worker retrying a terminal event after its lease expiry was cleared should get a 409, as this PR does, or the stored duplicate, as today. If 409 is right, this rebases onto feat-cmux-next and goes for review. If not, close it. Nothing else depends on it. |
A worker whose lease was replaced can still replay an event or artifact it sent before, and can still schedule terminal retention over the replacement worker's VM handles. These DB behavior tests expect both to be rejected with RunLeaseConflictError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ingestRunEvent and recordRunArtifact now check the caller's lease before returning a stored duplicate, so a worker whose lease was replaced can no longer get a success reply for an event or artifact it sent earlier. scheduleAttemptRetention now requires the worker's lease token and generation and a terminal attempt. A terminal event keeps the lease token (only its expiry is cleared) until this handoff consumes it, so a stale worker's finally block cannot overwrite the replacement worker's VM, workspace and terminal handles or its retention deadline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cx-q1lnl) 134 main commits since the last sync (3e7ed5a). Full rename detection (merge.renameLimit=100000). manaflow-ai#21310 (0d5316d, export npm_config_prefix) is now on main; it is a superset of feat's manaflow-ai#21311 copy, so the resume workflows keep one version (main's). No Swift, Rust, Go or package manifest change survives the merge (daemon/remote/go.mod unchanged). Conflict choices: - 209 modify/delete and 1 rename/delete in the legacy app, legacy CLI, cmuxTests, legacy Swift packages and Resources/Localizable.xcstrings: kept deleted. - cmux.xcodeproj/project.pbxproj, cmux.xcworkspace/contents.xcworkspacedata, docs/cli-contract.md, tests/test-execution.toml: feat-cmux-next's side. - ci-macos.yml, seed-derived-data.yml, restore-app-host-test-product.sh, package-test-lane.sh (CmuxTextActions), tests/test_ci_change_areas.py (app-host admission block): feat's side; main's rsync --no-times edits are for CmuxAgentJournal framework staging that feat deleted. Main's new merge-gate change-area test merged in. - scripts/ci/compile-app-host-test-product.sh: main's logic (the legacy cmux target builds without the compilation cache on every Xcode); the test (test_ci_test_compilation_cache_seed.sh) keeps feat's one-scheme checks with main's every-Xcode loop. - nightly.yml and tests/test_nightly_universal_build.sh: feat's side (feat already had manaflow-ai#21761's empty DerivedData start via manaflow-ai#21762, plus the nightly-next build-state prepare and its no-replay check; manaflow-ai#21544's promotion change was already on feat via manaflow-ai#21533). Feat's WRITER gate, live HOLD read, recovery guard, legacy-identity split, Mach-O floor, macOS 15 launch job and bash 3.2 parse hardening kept. - scripts/gh-merge-green: main's version (multiple freeze tokens manaflow-ai#21584, merge-tree merged head manaflow-ai#21576, missing Swift path refusal manaflow-ai#21560, risk evidence manaflow-ai#21692) plus feat's "cmux-next Swift canary" required check, freeze expiry and owner/pr/head metadata, and the canonical merged path. - scripts/install-cmux-tui-client.sh: union of the header comments (main's total download budget manaflow-ai#16305 and feat's attestation retry text). - web/data/cmux-shortcuts.ts: feat's generated table. - web/services/vms/workflows.ts: feat's doc comment on requireUserVm. - Run API: feat's version (manaflow-ai#21337, open lanes manaflow-ai#21634/manaflow-ai#21305 build on it). web/services/runs/*, web/app/api/runs/*, web/db/schema.ts run tables: feat's side. Main's parallel Run API (manaflow-ai#21444, manaflow-ai#21468, manaflow-ai#21473, manaflow-ai#21477, manaflow-ai#21484, manaflow-ai#21488, manaflow-ai#21492, manaflow-ai#21495, manaflow-ai#21467, manaflow-ai#21517, manaflow-ai#21573, manaflow-ai#21596) is NOT ported: its runs services, cron route and vercel.json cron, admission probe, run migrations 20261017150000/20261018000000/20261018110000/ 20261018120000 and their tests are removed here, and cloud-vm-dogfood.yml / dogfood-journey.mjs stay feat's (main's staging dogfood deploy changes manaflow-ai#21556-manaflow-ai#21617 and manaflow-ai#21424-manaflow-ai#21428 not ported). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Covers the retry rule for reused event ids and artifact names. The writer's identical retry gets the stored row after its lease was replaced, after its own terminal event cleared the expiry, and after the retention handoff cleared the token. A changed retry is a content conflict. A stale holder gets a lease conflict. A replacement holder never gets another lease's row, and a row with no recorded writer matches nobody. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wrote it Event and artifact rows now record the writing lease's generation and the SHA-256 of its token (migration 20261019120000_run_writer_lease). A request that reuses a stored id gets that row back, with no write, only when the caller is that lease and the request is identical (kind, attempt sequence and jsonb payload for events; digest, size and URI for artifacts). Otherwise it is a 409: a content conflict for the writer or the live holder, and a lease conflict for anyone else. A new id still needs the live lease. Worker responses drop the writer columns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d002617 to
2ced057
Compare
teamleaderleo
changed the title
|
Retry decision: a reused event id or artifact name gets the stored row back only for the lease that wrote it, and only for an identical request. Every other reuse is a 409. The rule, the cases and the counterargument are in the Decision section of the PR description. Head |
|
all web/size checks green |
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Passes: CI passes on CI passes on Written by
|
|
Agents: |
|
Found 1 test failure on Blacksmith runners: Failure
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light.svg)
|
CI on Passed
Red, also on
|
…-run-lease-fencing
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
 [Medium impact] The changes reach the durable Run worker and VM cleanup workflow. Fix the late-created VM cleanup gap before merging. FindingsSummaryThis PR ties event and artifact retries to the lease that wrote each row.
A replacement worker's VM-less failure can still block cleanup of a late-created VM. teamleaderleo explicitly defers the existing fixed-id/sequence collision issue and accepts successful retries not reporting lease loss. Rows without a recorded writer intentionally match nobody. Review used code inspection; no tests were executed. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[First worker starts VM creation] --> B[Lease expires]
B --> C[Replacement takes lease]
C --> D[Same create key reports creation in progress]
D --> E[Replacement records failure without VM id]
E --> F[Terminal event sets retention deadline]
A --> G[First worker receives VM id]
G --> H[Event writes are fenced]
H --> I[Retention handoff tries adoption]
F --> I
I --> J[Non-null deadline blocks adoption]
J --> K[No VM id saved for cleanup]
Reviews (3) · Last reviewed commit: "Schedule cleanup for a VM created while ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| externalId: text("external_id"), | ||
| kind: runEventKind("kind").notNull(), | ||
| payload: jsonb("payload").$type<Record<string, unknown>>().notNull().default(sql`'{}'::jsonb`), | ||
| /** Lease that wrote this event. A retry of it is answered only for this lease or the live holder. */ |
There was a problem hiding this comment.
This comment says the live holder can replay another lease's event, but assertStoredRetry rejects that request even when the content matches. Remove “or the live holder” so the schema documents the writer-only rule. The artifact comment points here too. Leaving this wording could lead a teammate to restore the behavior this PR deliberately removes.
| /** Lease that wrote this event. A retry of it is answered only for this lease or the live holder. */ | |
| /** Lease that wrote this event. An identical retry is answered only for this lease. */ |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| eq(runAttempts.leaseToken, input.leaseToken), | ||
| eq(runAttempts.leaseGeneration, input.leaseGeneration), | ||
| inArray(runAttempts.status, ["succeeded", "failed", "cancelled"]), |
There was a problem hiding this comment.
If the user cancels while createVm is still running, finalizeAttemptCancellation clears the lease before the VM is recorded. When creation returns, both event writes fail, and these new checks also reject the finally block's scheduleAttemptRetention call. That rejection is only logged. The VM stays running without a providerVmId or retention deadline on the attempt, so attempt cleanup cannot find it.
Keep the fence, but add a safe cleanup path for a VM created after its worker was cancelled.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A cancel during createVm fences the worker before it can store the VM id, so its retention handoff was rejected and no row pointed at the VM. The fenced handoff now records the id for immediate reaping when no lease ever stored a VM for the attempt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| gt(runAttempts.leaseGeneration, input.leaseGeneration), | ||
| inArray(runAttempts.status, ["succeeded", "failed", "cancelled"]), | ||
| isNull(runAttempts.providerVmId), | ||
| isNull(runAttempts.terminalRetainUntil), |
There was a problem hiding this comment.
adoptFencedAttemptVm rejects a late-created VM when a replacement worker failed without a VM. If the first worker's lease expires during createVm, a replacement can take the lease and receive VmCreateInProgressError for the same create key. Its attempt.failed event sets terminalRetainUntil even though providerVmId is still null.
When the first worker receives the VM, this null-deadline check blocks adoption. Its event writes are also fenced, so the attempt never records the VM and the reaper cannot find it. Allow this handoff for a terminal attempt that never recorded a VM, even when its terminal event already set a deadline.
|
Shipped in:
|
… from #21305) (#22408) * cmux-next: add the Run board webview and its Run API client Rebuilt from #21305 onto the landed Run API (#21337, #21634). The board reads a durable run's snapshot and replays its events by cursor, mints and redeems a one-use attach ticket, and asks the App to open the worker terminal (run.attach). Gallery entries: the base board, the Compare and Timeline variants, and the top-two tournament judge. Adapted to current lints: type-scale and layer tokens instead of raw sizes and z-index, an output element for attach status, and a narrow fetch type so tests pass plain functions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * cmux-next: open a redeemed Run attempt's terminal from the agent pane (run.attach) Rebuilt from #21305. The pane decodes run.attach (ids, a live trusted-carrier ws/wss endpoint, the worker session) and, with the person's fresh gesture in the pane, asks the App to open the attempt's terminal as a remote-terminal tab. The App opens it only on a connected remote worker daemon whose session is the one the ticket names; the route is never dialed and no token is accepted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * web: reap retained Run attempt VMs and accept worker artifact manifests The landed Run API schedules a short terminal retention window after an attempt finishes and its comment says a cron reaper destroys the VM after it, but no route called reapExpiredCloudVmAttempts, so retained VMs were never destroyed by it. Add /api/cron/run-attempt-retention (monitored, every minute) and the worker artifacts route over the landed recordRunArtifact. Port #21305's tests for the run model, provider view, worker auth and the live-database route suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(web): expect a changed event retry to conflict in the Run route suite #21634 made a retry with the same event id and different content a 409; the suite ported from #21305 still expected the first event back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(web): a promoted attempt's VM is not claimed by the retention reaper Fails on the base: claimExpiredAttemptCleanups claims any finished attempt whose retention expired, including one promoted to a persistent runtime, so the retention cron would destroy the runtime's live VM. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): keep promoted runtimes' VMs out of the retention reaper claimExpiredAttemptCleanups now skips attempts linked to a runtime: promotion hands the attempt's VM to a persistent runtime without clearing the retention window, so the new cron would destroy it. A VM the reaper finds already gone now counts as cleaned instead of failing every minute and starving older rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(cmux-next): poll the current run and gate Attach on the Run view The board rebuilds its replay session when the run id changes, reads the latest snapshot without a stale closure (vp check's exhaustive-deps), takes the session id from the view's placement and disables Attach when the view says the attempt is not attachable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(cmux-next): run.attach opens only on a connected Cloud machine daemon(machine:) also resolves SSH hosts and paired servers; the ticket names a Cloud VM, and the page's fields are not verified natively, so the lookup is now Cloud-only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * cmux-next: keep CmuxNextApp from growing with run.attach The size cap refuses any source growth in CmuxNextApp. The run.attach closure and its onRunAttach hook are joined into fewer lines, and four one-statement functions and three guard blocks in the same two files are written on one line each. No behavior change. Agent-Session: 01a15ba2-c7ad-7850-9e71-5b51f1c9ed9f * cmux-next: run.attach's public init imports Foundation publicly Base now builds CmuxNextAgentPane with InternalImportsByDefault, so AgentPaneRunAttach's public init(params:now: Date = Date()) failed to compile (fleet compile-check 3476a896 at 208bdff). Agent-Session: 01a15ba2-c7ad-7850-9e71-5b51f1c9ed9f * test(web): the retention reaper claims an attempt on its own ephemeral runtime (failing) Review of #22408: launchCloudVmAttempt gives every attempt an ephemeral runtime, so skipping any attempt with a runtime_id skips every real Run VM and none is ever destroyed. The promoted-VM test now inserts its runtime as persistent explicitly. Agent-Session: 01a15ba2-c7ad-7850-9e71-5b51f1c9ed9f * fix(web): the retention reaper skips only attempts promoted to a persistent runtime Every launched attempt is attached to its own ephemeral runtime before its VM exists, so the claim now left-joins cloud_runtimes and keeps an attempt whose runtime is ephemeral; the row lock stays on run_attempts (FOR UPDATE cannot lock the nullable side of an outer join). Agent-Session: 01a15ba2-c7ad-7850-9e71-5b51f1c9ed9f * test(web): promotion is refused while the reaper holds the attempt's cleanup lease (failing) Review of #22408: a promotion that commits after the reaper's claim but before destroyVm still loses its VM. Agent-Session: 01a15ba2-c7ad-7850-9e71-5b51f1c9ed9f * fix(web): refuse to promote an attempt while the reaper is cleaning up its VM Agent-Session: 01a15ba2-c7ad-7850-9e71-5b51f1c9ed9f --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
A Run worker can now retry an event or artifact it already wrote and get the stored row back, even after its lease expired, was replaced, or was cleared by its own terminal event. A stale lease holder can no longer get a success reply for someone else's work or overwrite the replacement worker's state.
What changed:
run_eventsandrun_artifactsrecord which lease wrote each row: its generation and the SHA-256 of its token. The new migration is20261019120000_run_writer_lease. It is additive with nullable columns, so it stands alone.ingestRunEventandrecordRunArtifactapply the retry rule below before they check the lease. A request with a new id still needs the live lease.Worker responses drop the writer columns.
scheduleAttemptRetentionneeds the worker's lease token and generation and a terminal attempt. A stale worker'sfinallyblock therefore can't overwrite the replacement's VM, workspace and terminal handles or its retention deadline. A terminal event clears only the lease expiry and keeps the token until this handoff consumes it.Cancel during VM create no longer leaks the VM. A cancel that lands while
createVmis in flight bumps the lease generation before the worker has stored the VM id. The worker'sattempt.startedingest and its fencedscheduleAttemptRetentionhandoff were then both rejected, so no row pointed at the VM and the reaper never destroyed it. Now, when the fenced handoff is rejected,scheduleAttemptRetentionstill records the provider VM id for immediate reaping (terminalRetainUntil = now). It does this only when all of these hold:providerVmId.Nothing else can own that VM, because create is idempotent per attempt. A VM the reaper already destroyed and cleared is never handed back to it. No other field of the fenced worker is written.
Carved from #21305 (commits
bdbf27bd05,4d9248ff18,f8b5cc631d,585deed335). The durable Run schema and repository landed onfeat-cmux-nextin #21337, andweb/services/runshasn't changed there since. This PR is the lease-fencing delta on top of it, now rebased onto currentorigin/feat-cmux-next.main's separate Run implementation (#21444 and its follow-ups) has no worker leases, so nothing there supersedes this.Decision
Rule: a request that reuses a stored event id or artifact name gets the stored row back (200, nothing written) only when the caller's lease token hash and generation match the lease that wrote that row and the request is identical:
Any other reuse is a 409:
run_event_conflictorrun_artifact_conflictwhen the caller is the writer or holds the live lease;run_lease_conflict.A stale caller gets the same reply for a reused id as for a new one, so it learns nothing about stored rows. Rows with no recorded writer match nobody.
Cases, each a DB behavior test in
web/tests/run-repository-db-behavior.test.ts:succeeded, no new journal rowsrun_lease_conflictrun_lease_conflictrun_event_conflictattempt-startedrun_event_conflict; attempt staysleasedCounterargument: I asked a review agent to argue against the first draft. These changes came out of it:
attempt-startedreturn the old lease's row. The attempt then stayedleasedwhile the new worker believed it had started.attempt.artifactjournal row.Open points it raised, not changed here:
attempt-failedwith low attempt sequences collides with the old lease's rows and with per-attempt sequence uniqueness, so it can't record a terminal event for that attempt. This was already true before this PR. Fixing it needs ids and sequences scoped by generation, and that's a separate change.Validation
bun scripts/db-migrate.mjs:CMUX_DB_TEST=1 bun test tests/run-retention-db-behavior.test.ts:e966878b7a7, test commit only),schedules cleanup for a VM whose create returned after cancellation fenced the workerfails withstale or expired attempt lease.d7fe72e9565), all 5 pass.does not hand a stale worker's VM back to retention after the reaper destroyed it, passes both before and after.bun run test:db:behavioron head: exit 0, no failures.tsgo --noEmit: no errors inweb/servicesorweb/tests/run-*. The only errors are fromworkers/sources, which that partial copy didn't have. CIweb-typecheckis green.1c0ece6d38d(tests), thena8498dd0335(fix);99c25ae938e(tests), then2ced057fdac(fix);e966878b7a7(tests), thend7fe72e9565(fix).Proof
web/services/runs,web/tests, one additive migration). No app, webview or Swift code.run-retention-db-behavior.test.ts(schedules cleanup for a VM whose create returned after cancellation fenced the worker,does not hand a stale worker's VM back to retention after the reaper destroyed it, plus the existing retention cases) andrun-repository-db-behavior.test.ts(the lease-fencing retry cases), inweb / web-db-migrationsond7fe72e9565: https://github.com/manaflow-ai/cmux/actions/runs/39344503727/job/118374018262Changelog
Fixed: Cloud Run workers can safely retry their own events and artifacts, a replaced worker can no longer overwrite the new worker's state, and cancelling a Run while its VM is being created no longer leaks the VM.
🤖 Generated with Claude Code