Skip to content

Commit 85d2cd2

Browse files
committed
fix(execution): guard ban stops against transfers and refusing runs
- 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.
1 parent e6fb280 commit 85d2cd2

5 files changed

Lines changed: 88 additions & 16 deletions

File tree

‎apps/sim/lib/core/async-jobs/backends/trigger-dev.test.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -924,3 +924,27 @@ describe('TriggerDevJobQueue workspace cancellation', () => {
924924
)
925925
})
926926
})
927+
928+
describe('TriggerDevJobQueue workspace cancellation past refusing runs', () => {
929+
beforeEach(() => {
930+
mockList.mockReset()
931+
mockCancel.mockReset()
932+
})
933+
934+
it('reaches later runs when the first ones keep refusing cancellation', async () => {
935+
const listed = Array.from({ length: 15 }, (_, index) => ({
936+
id: `run-${index}`,
937+
tags: ['workspaceId:removed'],
938+
}))
939+
mockList.mockImplementation(() => createListPage(listed))
940+
const refusing = new Set(['run-0', 'run-1', 'run-2'])
941+
mockCancel.mockImplementation(async (runId: string) => {
942+
if (refusing.has(runId)) throw new MockApiError(500, { message: 'refused' })
943+
})
944+
const queue = new TriggerDevJobQueue()
945+
946+
await expect(queue.cancelByWorkspace('removed', { maxRuns: 5 })).rejects.toThrow('refused')
947+
948+
expect(mockCancel).toHaveBeenCalledWith('run-9')
949+
})
950+
})

‎apps/sim/lib/core/async-jobs/backends/trigger-dev.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -675,7 +675,6 @@ export class TriggerDevJobQueue implements JobQueueBackend {
675675
): Promise<WorkspaceJobCancellationResult> {
676676
const workspaceTag = buildWorkspaceTag(workspaceId)
677677
const state: CancellationScanState = { cancelledJobs: 0, failureCount: 0 }
678-
let inspectedRuns = 0
679678
let complete = true
680679

681680
await scanAndCancelTriggerRuns({
@@ -688,16 +687,24 @@ export class TriggerDevJobQueue implements JobQueueBackend {
688687
from: new Date(Date.now() - JOB_PENDING_RETENTION_HOURS * 60 * 60 * 1000),
689688
limit: WORKSPACE_CANCELLATION_LIST_PAGE_SIZE,
690689
}),
690+
/**
691+
* Only cancelled runs count toward the cap, so runs that keep refusing cancellation cannot
692+
* hold every retry at the front of the listing; a separate bound on failures keeps one call
693+
* finite. Both are counted per chunk, so a call may overshoot `maxRuns` by under a chunk.
694+
*/
691695
shouldStop: () => {
692-
if (inspectedRuns < options.maxRuns && !options.signal?.aborted) return false
696+
if (
697+
state.cancelledJobs < options.maxRuns &&
698+
state.failureCount < options.maxRuns &&
699+
!options.signal?.aborted
700+
) {
701+
return false
702+
}
693703
complete = false
694704
return true
695705
},
696-
selectCandidate: (run) => {
697-
if (!run.tags.includes(workspaceTag)) return undefined
698-
inspectedRuns += 1
699-
return { id: run.id, verifyPayload: false }
700-
},
706+
selectCandidate: (run) =>
707+
run.tags.includes(workspaceTag) ? { id: run.id, verifyPayload: false } : undefined,
701708
state,
702709
})
703710

‎apps/sim/lib/execution/removal-stop-records.test.ts‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
11
/**
2-
* An in-process run is stopped by aborting it here, which needs no Redis. Failure mode: the
3-
* signal deadline passes before the Redis writes start, and the local runs are left going.
2+
* Failure modes:
3+
* - the signal deadline passes before the Redis writes start, and the local runs are left going
4+
* - a ban stops a previously archived workspace whose ownership moved on, and its new owner's
5+
* runs are stopped
46
*/
7+
import { db } from '@sim/db'
8+
import { workspace } from '@sim/db/schema'
9+
import { dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing/mocks/database.mock'
510
import { describe, expect, it, vi } from 'vitest'
611
import {
712
registerManualExecutionAborter,
813
unregisterManualExecutionAborter,
914
} from '@/lib/execution/manual-cancellation'
10-
import { signalExecutionStops } from '@/lib/execution/removal-stop-records'
15+
import { recordExecutionStops, signalExecutionStops } from '@/lib/execution/removal-stop-records'
1116

1217
describe('signalling removed executions', () => {
1318
it('aborts the runs this process holds even after the signal deadline has passed', async () => {
@@ -30,3 +35,19 @@ describe('signalling removed executions', () => {
3035
expect(abort).toHaveBeenCalledOnce()
3136
})
3237
})
38+
39+
describe('recording a banned owner’s stops', () => {
40+
it('records nothing for a workspace the user no longer owns', async () => {
41+
resetDbChainMock()
42+
queueTableRows(workspace, [])
43+
44+
await expect(
45+
db.transaction((tx) =>
46+
recordExecutionStops(tx, { workspaceIds: ['transferred'], ownerId: 'banned-user' })
47+
)
48+
).resolves.toEqual([])
49+
50+
expect(dbChainMockFns.for).toHaveBeenCalledWith('share')
51+
expect(dbChainMockFns.insert).not.toHaveBeenCalled()
52+
})
53+
})

‎apps/sim/lib/execution/removal-stop-records.ts‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@ import { resumeQueue, workflow, workflowExecutionLogs, workspace } from '@sim/db
33
import { createLogger } from '@sim/logger'
44
import { describeError } from '@sim/utils/errors'
55
import { chunkArray } from '@sim/utils/helpers'
6-
import { and, eq, inArray, or, type SQL } from 'drizzle-orm'
6+
import { and, asc, eq, inArray, or, type SQL } from 'drizzle-orm'
77
import { enqueueOutboxEvents, MAX_BULK_ENQUEUE_EVENTS } from '@/lib/core/outbox/service'
88
import { mapWithConcurrency } from '@/lib/core/utils/concurrency'
9-
import type { DbOrTx } from '@/lib/db/types'
9+
import type { DbOrTx, DbTransaction } from '@/lib/db/types'
1010
import { markExecutionCancelled } from '@/lib/execution/cancellation'
1111
import { abortManualExecution } from '@/lib/execution/manual-cancellation'
1212
import { EXECUTION_REMOVAL_STOP_EVENT } from '@/lib/execution/removal-stop-event'
@@ -181,10 +181,28 @@ export async function enqueueExecutionStops(
181181
* Returns the executions, to signal at once with {@link signalExecutionStops} after commit.
182182
*/
183183
export async function recordExecutionStops(
184-
executor: DbOrTx,
185-
scope: ExecutionStopScope
184+
executor: DbTransaction,
185+
scope: ExecutionStopScope & {
186+
/**
187+
* Stop only the workspaces this user still owns. Their rows are held `FOR SHARE` until the
188+
* caller commits, so an ownership transfer racing the stop waits for it instead of having its
189+
* new owner's runs stopped. Only workspace rows are locked, so no lock order is inverted.
190+
*/
191+
ownerId?: string
192+
}
186193
): Promise<ExecutionStopPlan['executions']> {
187-
const plan = await collectExecutionStops(executor, scope)
194+
let { workspaceIds } = scope
195+
if (scope.ownerId && workspaceIds.length > 0) {
196+
const stillOwned = await executor
197+
.select({ id: workspace.id })
198+
.from(workspace)
199+
.where(and(inArray(workspace.id, [...workspaceIds]), eq(workspace.ownerId, scope.ownerId)))
200+
.orderBy(asc(workspace.id))
201+
.for('share')
202+
workspaceIds = stillOwned.map((row) => row.id)
203+
if (workspaceIds.length === 0 && !scope.workflowsCreatedBy) return []
204+
}
205+
const plan = await collectExecutionStops(executor, { ...scope, workspaceIds })
188206
await enqueueExecutionStops(executor, plan)
189207
return plan.executions
190208
}

‎apps/sim/lib/workflows/lifecycle.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,9 @@ export async function disableUserResources(userId: string): Promise<void> {
332332
const stopRuns = async (workspaceIds: string[]): Promise<void> => {
333333
if (workspaceIds.length === 0) return
334334
try {
335-
const executions = await db.transaction((tx) => recordExecutionStops(tx, { workspaceIds }))
335+
const executions = await db.transaction((tx) =>
336+
recordExecutionStops(tx, { workspaceIds, ownerId: userId })
337+
)
336338
await signalExecutionStops(executions)
337339
} catch (error) {
338340
stopFailed = true

0 commit comments

Comments
 (0)