Repository navigation
fix(execution): stop in-flight workflow runs when an account is deleted, removed or banned - #9145
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
All reported issues were addressed across 15 files
This PR changes concurrency-sensitive code such as locks, queues, or retries. Ultrareviews find 2.4x more serious bugs than standard reviews. Comment @cubic-dev-ai ultrareview to run one.
Turn on auto-fix | Re-trigger cubic
[Critical impact] The PR appears safe to merge; no new blocking issue was found. SummaryThis PR adds workflow cancellation to account deletion, admin removal, and bans. It records stops for later retries, signals running executions, and cancels workspace jobs through both queue backends.
|
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
…ed, removed or banned No removal path stopped workflow executions. Account deletion cascades the doomed workspaces' log rows away, after which no cancel path can find the run, so it kept spending hosted provider keys for an account that no longer existed until it crashed on foreign keys at finalization. A ban archived the workspaces and stopped nothing; an admin removal cascaded them the same way. - Account deletion reads the running executions (and claimed resumes, which run under their own id) of the doomed workspaces in its transaction, while the workspace rows are locked, and records one outbox stop per workspace with the erasure. After commit the stops are delivered a few at a time; a stop that cannot be delivered stays in the outbox for its worker, and the committed deletion is never reported as failed. - A stop sets the cancellation flag each engine watches and sweeps the job queue for the workspace: a new JobQueueBackend.cancelByWorkspace cancels queued, delayed and running workflow jobs by the workspaceId tag on Trigger.dev (pages of 100, ten cancels at a time, 1,000 runs per attempt, continued without spending an attempt) and by metadata.workspaceId on the database queue, so queued runs no longer fail preprocessing later. - A missing Redis completes the stop: the flag is process-local by design and no retry could reach further. A refused Redis write or a failed sweep retries. - A ban stops the runs of the workspaces it archived; the admin remove-user hooks read the runs before the bare user delete and record them after it commits. Both sit in page module graphs that cannot load the delivery, so they signal in-flight runs at once and leave the queue sweep to the outbox worker.
…paths - Account deletion no longer awaits the full stop delivery before purging storage. A slow Trigger.dev listing could hold the response for minutes and widen the window in which a restart orphans erased objects. It now signals in-flight runs fire-and-forget right after commit, ahead of every other post-commit step, under a 10-second deadline; the committed outbox stops repeat the signal and sweep the job queue in the outbox worker. - A failure to record a ban's stops is logged instead of thrown. The ban and archives have committed, so a 500 helped no one and a retry finds no unarchived workspace to stop. - Claimed resumes are found from the scope's running and paused log rows and joined through resume_queue's parent index, not by scanning resume_queue for claimed rows inside the deletion transaction. - Tests pin the database queue's job-type filter, so a workspace sweep never cancels a non-workflow job such as log cleanup.
- A ban records and signals each workspace's stops as soon as its archive commits, through the archive's post-commit stop hook, instead of after every archive's socket and webhook cleanup. It also stops runs in the owner's workspaces archived before the ban, which an archive leaves running. - The outbox handler re-reads a workspace's running executions when it runs. An archived workspace keeps its rows, so a run admitted before the ban that logged after its stop was recorded is still signalled. - The admin remove-user hook handlers move to lib/auth/admin-removal-stops.ts, and signal the removed runs even when recording their stops fails. - The database queue's workspace sweep works in batches of 100 up to the caller's cap and checks its abort signal between them, like the Trigger.dev sweep, rather than one unbounded update reported as complete.
- disableUserResources reads owned workspaces once and stops earlier-archived ones up front, dropping the post-archive read that could fail the ban - Trigger.dev cancelByWorkspace reuses scanAndCancelTriggerRuns; workspace tag shares one builder with enqueue - database queue shares abortInlineJob with cancelJob and returns completeness from control flow - removal stop handler parses its payload with zod and signals while it sweeps - drop the unused event-id return and redundant bucketing state
- The removal stop handler loads both job queues independently and sweeps whichever loaded, so a primary queue that fails to load no longer skips the database queue's sweep. The failure is still rethrown for the outbox to retry. - A claimed resume is found under a parent log row in the legacy `paused` status too, one of the statuses a resume claims from. - The database queue re-checks for a matching active job after a short batch, since a selected job that finished before the update reached it shrinks the batch while later jobs still match. - The ban's completion log no longer reports its runs stopped when recording their stops failed.
770804e to
cd54396
Compare
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…an skip them The signal deadline bounds only the Redis flag writes. Aborting a run this process holds needs no Redis, so every execution is aborted first, even after the deadline has passed. Corrects the ban's note on why a failed stop record is logged: banning again records the stops again.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 18 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/workflows/lifecycle.ts">
<violation number="1" location="apps/sim/lib/workflows/lifecycle.ts:348">
P2: The archived-workspace stop path can cancel a successor owner's runs after a concurrent ownership transfer. Revalidate and lock ownership in the same transaction before recording stops, skipping workspace IDs no longer owned by `userId`.
(Based on your team's feedback about rechecking ownership during ban archival.) [4566fdf2-4a7b-4c10-8836-e8222b12fa54]</violation>
</file>
<file name="apps/sim/lib/core/async-jobs/backends/trigger-dev.ts">
<violation number="1" location="apps/sim/lib/core/async-jobs/backends/trigger-dev.ts:698">
P2: `cancelByWorkspace` charges `maxRuns` when a run is selected, not when it is cancelled. If the first `maxRuns` listed runs keep failing `runs.cancel`, every retry re-selects them and later active runs are never reached. Only count runs whose cancellation succeeded, or skip already-failed IDs on later pages.</violation>
</file>
- A ban records stops only for workspaces the user still owns, re-read in the recording transaction and held FOR SHARE until it commits, so a concurrent ownership transfer waits rather than having its new owner's runs stopped. Only workspace rows are locked, so no lock order is inverted. - Trigger.dev's workspace sweep counts only cancelled runs toward its cap, with a separate bound on failures, so runs that keep refusing cancellation no longer hold every retry at the front of the listing. cancelByExecution is unchanged.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 18 files
Confidence score: 3/5
removal-stop-records.tscan miss executions created between the pre-delete read and the user delete, leaving records behind after an admin deletion. Synchronize the collection and delete in one transaction, or use an equivalent deletion/run-admission lock.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/execution/removal-stop-records.ts">
<violation number="1" location="apps/sim/lib/execution/removal-stop-records.ts:127">
P2: Admin deletion can miss executions created between this pre-delete read and the user delete. Synchronize collection with the delete in one transaction or add an equivalent deletion/run-admission lock so every execution erased by the cascade is recorded before commit.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
The workspace sweep searched only runs created in the last 14 days, so an older run still queued or waiting survived the removal. It now passes only an upper time bound, which Trigger.dev treats as no lower bound, and falls back to the retention window only if Trigger.dev refuses that search as too expensive before returning a run. cancelByExecution is unchanged.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 18 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
Summary
No removal path stopped in-flight workflow executions; #9089 stops Copilot runs only. On 2026-10-16 a prod run kept spending a hosted fal.ai key for five minutes after its owner deleted their account, then crashed on foreign-key errors. Banned accounts' runs also kept spending until their timeout.
One mechanism for every removal path
lib/execution/removal-stop-records.ts(light, safe for page graphs):runninglog rows, plus claimed resumes, which run under their own execution id;execution.removal-stopoutbox events, one per workspace;markExecutionCancelled+abortManualExecution, 10 at a time with a 10 s deadline, and never throws.lib/execution/removal-stops.ts(outbox handler): signals again, then sweeps both job queues for the workspace.JobQueueBackend.cancelByWorkspace:runs.liston the existingworkspaceId:<id>tag, filtered to the five workflow task types (workflow, schedule, webhook, resume, workflow-group cell) and active statuses, with each result's tags rechecked exactly. Pages of 100, 10 concurrent cancels, capped.UPDATE async_jobsfiltered on status, the same five job types, andmetadata->>'workspaceId'; aborts this process's inline controllers.Wired into
FOR UPDATE, so a rollback enqueues nothing. After commit the signal is fired and forgotten before any other step. The sweep runs in the outbox worker. The deletion response and the storage purge never wait on Trigger.disableUserResources): after the archives commit, stops are recorded for the workspaces the ban archived and signalled immediately. A failure to record them is logged, not thrown, so the committed ban never 500s. Bans are admin-only./admin/remove-user:user.delete.beforecollects the user's owned workspaces and the workflows they created; both cascade with the user..afterenqueues and signals the stops.Runs stopped this way still log a noisy ending until #9144 lands; that PR makes it quiet.
Type of Change
Testing
enqueueactually sends. They prove:database.test.tspins the job-type filter.removal-stops.integration.ts(real Postgres + Redis) covers:cleanup-logsjob and a bystander's run untouched;check-tool-registry-boundary --check: all graphs within baseline.Checklist