fix(runs): replay worker retries only for the writing lease; fence retention handoffs

Merged
TE
17d ago
+816-16

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_events and run_artifacts record which lease wrote each row: its generation and the SHA-256 of its token. The new migration is 20261019120000_run_writer_lease. It is additive with nullable columns, so it stands alone.

  • ingestRunEvent and recordRunArtifact apply 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.

  • scheduleAttemptRetention needs the worker's lease token and generation and a terminal attempt. A stale worker's finally block 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 createVm is in flight bumps the lease generation before the worker has stored the VM id. The worker's attempt.started ingest and its fenced scheduleAttemptRetention handoff were then both rejected, so no row pointed at the VM and the reaper never destroyed it. Now, when the fenced handoff is rejected, scheduleAttemptRetention still records the provider VM id for immediate reaping (terminalRetainUntil = now). It does this only when all of these hold:

    • the attempt is terminal and its lease generation is newer than the caller's;
    • the attempt has no VM id, retention deadline or cleanup lease;
    • no event for the attempt ever carried a 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 on feat-cmux-next in #21337, and web/services/runs hasn't changed there since. This PR is the lease-fencing delta on top of it, now rebased onto current origin/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:

  • for events: kind, attempt sequence and payload, compared by jsonb equality;
  • for artifacts: digest, size and URI.

Any other reuse is a 409:

  • run_event_conflict or run_artifact_conflict when the caller is the writer or holds the live lease;
  • otherwise 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:

CaseResult
Same holder retries an identical terminal event after its expiry was cleared, and again after the retention handoff cleared the tokenStored row, attempt stays succeeded, no new journal rows
Stale holder sends a different event (new id) after a replacement took the leaserun_lease_conflict
Stale holder reuses an id the replacement wroterun_lease_conflict
The writer retries an identical non-terminal event after being replacedStored row, no new journal rows
The writer retries the same id with different contentrun_event_conflict
Replacement holder sends an identical copy of the old lease's attempt-startedrun_event_conflict; attempt stays leased
Row with no recorded writerNobody gets the stored row
ArtifactsSame rule

Counterargument: I asked a review agent to argue against the first draft. These changes came out of it:

  • Dropped "the live holder may replay any identical row": that clause let a replacement worker's identical attempt-started return the old lease's row. The attempt then stayed leased while the new worker believed it had started.
  • Payload compared in SQL: jsonb equality, so key order and number formatting don't break a retry.
  • Writer columns on every lease-created row, including the attempt.artifact journal row.
  • Identity is token hash plus generation: all workers share the worker token, and generations are guessable.

Open points it raised, not changed here:

  • Fixed per-attempt ids can block a replacement worker. A replacement worker that reuses ids such as attempt-failed with 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.
  • Replies don't say the lease is gone. A stale writer whose retry succeeds isn't told its lease is gone; it finds out on its next renew. That's harmless to data.

Validation

  • Locally, on huge-blue with Postgres 16 in podman, migrated with bun scripts/db-migrate.mjs:
    • CMUX_DB_TEST=1 bun test tests/run-retention-db-behavior.test.ts:
      • Before the fix (e966878b7a7, test commit only), schedules cleanup for a VM whose create returned after cancellation fenced the worker fails with stale or expired attempt lease.
      • After the fix (d7fe72e9565), all 5 pass.
    • The guard case, does not hand a stale worker's VM back to retention after the reaper destroyed it, passes both before and after.
    • Full bun run test:db:behavior on head: exit 0, no failures.
    • tsgo --noEmit: no errors in web/services or web/tests/run-*. The only errors are from workers/ sources, which that partial copy didn't have. CI web-typecheck is green.
  • Commits put tests before fixes: 1c0ece6d38d (tests), then a8498dd0335 (fix); 99c25ae938e (tests), then 2ced057fdac (fix); e966878b7a7 (tests), then d7fe72e9565 (fix).

Proof

  • Not UI: web Run repository and DB behavior tests only (web/services/runs, web/tests, one additive migration). No app, webview or Swift code.
  • Runtime test: 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) and run-repository-db-behavior.test.ts (the lease-fencing retry cases), in web / web-db-migrations on d7fe72e9565: https://github.com/manaflow-ai/cmux/actions/runs/39344503727/job/118374018262

Changelog

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