Skip to content

Cloud workspace: remaining provisioning races (duplicate job delivery, undo during the job's delete) #9457

Open

Description

@Bruno-rose

Where did this happen?

Web app (app.superset.sh)

Version

Source build, main at 19db630

What happened?

Two races are left after the provisioning ABA fix:

  1. Duplicate delivery of one provisioning job (for example a QStash retry): if one delivery throws, its catch deletes the sandbox and marks the row failed, even when the other delivery has just made the same sandbox ready. This contradicts the "Safe to run twice on the same row" note in provision.ts.
  2. Undo between the job's "deleted" read and its deleteSandbox call: the undo sees the sandbox still exists and marks the row ready, then the job deletes the sandbox. The next wake throws SandboxUnavailableError and the workspace becomes failed.

Expected: a job tears down a sandbox only when no row still needs it.

Steps to reproduce

Case 1: deliver the same provisioning job twice and make one delivery fail late.

Case 2:

  1. Delete a workspace while its sandbox is being created.
  2. Click undo just as the job finds the row deleted.
  3. See a ready workspace with no sandbox.

Logs, screenshots, or recordings

Found by model checking every order of events. Case 2 was reproduced as a failing test against the real code; case 1 was seen only in the model. Not fixed yet.

Implementation notes

Files

  • packages/trpc/src/router/cloud-workspace/provision.ts: the !ready branch and the catch block.
  • packages/trpc/src/router/cloud-workspace/cloud-workspace.ts unarchive.

Approach

Model checking showed that the simple fixes (stop instead of delete, catch transitions first) each leave or create another bad state. A correct fix probably needs the job to claim the row (a guarded write that renames or marks the sandbox) before it tears the sandbox down, and the resumable unarchive transition guarded on the sandbox it checked.

Activity

  1. github-actions commented on Nov 7, 2026

    @github-actions
    Contributor

    Case 2 is clearly real from the code. Case 1 is plausible as described. Both come from the same flaw: provisioning a workspace and undoing its delete each decide to delete or revive the sandbox based on a status they read earlier, and they never check that status again when they act. Two deliveries of the same job, or a job and an undo, use the same sandbox name. So when one side tears the sandbox down, it can destroy a sandbox the other side has just marked ready. The row then says ready (or failed) while the sandbox is gone, and the next wake turns the workspace failed.

    Affected code

    • packages/trpc/src/router/cloud-workspace/provision.ts:124-149: the !ready branch. It does a separate, unguarded read (:139-142), then calls deleteSandbox(providerSandboxId) (:144).
    • packages/trpc/src/router/cloud-workspace/provision.ts:180-209: the catch block. It tears the sandbox down by name first (:196-203), then moves the row to failed with from: ["provisioning", "ready"] (:204-209).
    • packages/trpc/src/router/cloud-workspace/cloud-workspace.ts:779-820: unarchive. It checks sandboxExists(row.providerSandboxId) (:786), then moves deleted → ready guarded only on status (:790-797).
    • packages/trpc/src/router/cloud-workspace/transition.ts:13-30: the only guard available is status IN (from). It cannot check which sandbox the row points at.
    • packages/trpc/src/router/cloud-workspace/cloud-workspace.ts:146-160: a wake that gets SandboxUnavailableError calls markSandboxUnavailable, which is how a ready row with no sandbox ends up failed.

    Suspected root cause

    Case 1 (duplicate delivery): Both deliveries read status = provisioning at provision.ts:92 and use the same providerSandboxId. provisionSandbox reuses the sandbox if it already exists (lib/sandbox/vercel.ts:214-215), so they share one sandbox.

    • Delivery A gets through and moves the row to ready.
    • Delivery B fails at any step: authorizeBoot, provisionSandbox hitting a sandbox that is still being created, or settleSandbox.
    • B's catch deletes the shared sandbox (:196). Its transition then succeeds, because "ready" is in from (:206). This overwrites A's ready with failed.

    The comment "Safe to run twice on the same row" (:64-65) only holds for the success path. The catch block has no idea whether this delivery is the one that marked the row ready.

    Case 2 (undo during the job's delete): Order of events:

    1. The job's ready transition loses to delete.
    2. The job reads status = deleted (:139-143).
    3. unarchive runs sandboxExists → true, so it takes the resumable path and moves the row to ready (cloud-workspace.ts:783-797).
    4. The job calls deleteSandbox (provision.ts:144), still acting on the deleted it read in step 2.

    The row is now ready with no sandbox. The next wake throws SandboxUnavailableError and the workspace is marked failed.

    Reproduction

    • Case 2 (fits a bun:test): Seed a provisioning row and run provision() with a fake sandbox layer. Make provisionSandbox resolve only after delete has run, so the ready transition fails. Pause between the job's status read and deleteSandbox (for example a deferred promise in the fake getSandbox or delete), call unarchive there, then let the job continue. The row should come out ready with the fake sandbox deleted.
    • Case 1: Run provision() twice at once on the same row. Let A resolve normally, and make B's authorizeBoot or provisionSandbox reject after A's ready transition. The row should end up failed and the sandbox deleted.
    • Per the repo test rules, pass the sandbox and meter calls in as parameters or use a local server. Don't mock.module lib/sandbox, because other test files import it for real.

    Fix direction

    I agree with the issue that the fix has to be a guarded claim on the row, not a change of order:

    1. Claim before tear-down. In both the !ready branch and the catch, replace "read, then delete" with a conditional update that claims the sandbox, for example WHERE id = ? AND status = 'deleted' AND providerSandboxId = ? setting providerSandboxId to a tombstone or the next name. Delete the sandbox only if that update changed a row. transitionCloudWorkspace needs an optional extra condition (such as providerSandboxId) to support this.
    2. Guard the resumable unarchive on the sandbox it checked. Add providerSandboxId = row.providerSandboxId to the deleted → ready transition. If the job claimed the sandbox in between, the transition fails, and unarchive should fall back to the provisioning path with nextSandboxNameFor instead of returning unarchived: false.
    3. The catch should move the row only out of a state this delivery owns. Include "ready" in from only when this delivery's own ready transition succeeded. Otherwise use ["provisioning"] and tear down only after winning that transition. A real per-delivery lease (a token column checked in each transition) would also stop a slower duplicate failing a row whose first delivery is still settling.
    4. Update or remove the "Safe to run twice" comment at provision.ts:64-65 to match whatever guarantee the fix gives.

    I found no existing test that covers either order; cloud-workspace.test.ts and reap.test.ts are the closest suites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions