Skip to content

Commit 86faeab

Browse files
committed
fix(execution): close the review's gaps in removal stops
- 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.
1 parent b65ee91 commit 86faeab

9 files changed

Lines changed: 289 additions & 115 deletions

File tree

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
/**
2+
* The `/admin/remove-user` hooks hand a plan from `before` to `after` across a non-transactional
3+
* delete. Failure modes:
4+
* - the outbox write fails after the delete committed → the in-flight runs must still be signalled
5+
* - `after` runs for a different delete than the one `before` read → nothing is stopped
6+
*/
7+
import { workflowExecutionLogs, workspace } from '@sim/db/schema'
8+
import { dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing/mocks/database.mock'
9+
import { redisConfigMockFns } from '@sim/testing/mocks/redis-config.mock'
10+
import { beforeEach, describe, expect, it, vi } from 'vitest'
11+
import { holdAdminRemovalStops, stopAdminRemovedRuns } from '@/lib/auth/admin-removal-stops'
12+
13+
const mockEval = vi.fn()
14+
15+
function queueRunningExecution(executionId: string) {
16+
queueTableRows(workspace, [])
17+
queueTableRows(workflowExecutionLogs, [
18+
{ executionId, executionDeadlineAt: null, workspaceId: 'workspace-elsewhere' },
19+
])
20+
queueTableRows(workflowExecutionLogs, [])
21+
}
22+
23+
describe('admin user removal stops', () => {
24+
beforeEach(() => {
25+
resetDbChainMock()
26+
mockEval.mockReset().mockResolvedValue(1)
27+
redisConfigMockFns.mockGetRedisClient.mockReturnValue({ eval: mockEval })
28+
})
29+
30+
it('signals the removed user’s runs even when their stops cannot be recorded', async () => {
31+
const user = { id: 'user-1' }
32+
queueRunningExecution('execution-1')
33+
await holdAdminRemovalStops(user)
34+
dbChainMockFns.insert.mockImplementationOnce(() => {
35+
throw new Error('outbox unavailable')
36+
})
37+
38+
await expect(stopAdminRemovedRuns(user)).resolves.toBeUndefined()
39+
40+
expect(mockEval).toHaveBeenCalledWith(
41+
expect.any(String),
42+
1,
43+
'execution:cancel:execution-1',
44+
expect.anything(),
45+
expect.anything(),
46+
expect.anything(),
47+
expect.anything()
48+
)
49+
})
50+
51+
it('stops nothing for a delete whose runs it did not read', async () => {
52+
queueRunningExecution('execution-2')
53+
await holdAdminRemovalStops({ id: 'user-2' })
54+
55+
await stopAdminRemovedRuns({ id: 'user-2' })
56+
57+
expect(mockEval).not.toHaveBeenCalled()
58+
})
59+
})
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { db } from '@sim/db'
2+
import { createLogger } from '@sim/logger'
3+
import { getErrorMessage } from '@sim/utils/errors'
4+
import {
5+
collectAccountRemovalStops,
6+
type ExecutionStopPlan,
7+
enqueueExecutionStops,
8+
signalExecutionStops,
9+
} from '@/lib/execution/removal-stop-records'
10+
11+
const logger = createLogger('AdminRemovalStops')
12+
13+
/**
14+
* Plans read by {@link holdAdminRemovalStops} and acted on by {@link stopAdminRemovedRuns},
15+
* keyed by the user entity Better Auth passes to both hooks of one delete. A delete that fails
16+
* never reaches `after`, so its plan is dropped with the entity.
17+
*/
18+
const pendingStops = new WeakMap<object, ExecutionStopPlan>()
19+
20+
/**
21+
* The user delete's `before` hook: `/admin/remove-user` deletes the user row outside any
22+
* transaction, and the cascade erases the rows naming its runs, so they are read now. A failed
23+
* read lets the removal proceed as before rather than blocking an admin.
24+
*/
25+
export async function holdAdminRemovalStops(user: { id: string }): Promise<void> {
26+
try {
27+
pendingStops.set(user, await collectAccountRemovalStops(db, user.id))
28+
} catch (error) {
29+
logger.error('Failed to read the runs a user removal must stop', {
30+
userId: user.id,
31+
error: getErrorMessage(error),
32+
})
33+
}
34+
}
35+
36+
/**
37+
* The user delete's `after` hook, once the delete has committed: signals the in-flight runs at
38+
* once, whether or not their stops could be recorded, and records the stops for the outbox
39+
* worker's queue sweep and retries. Never throws.
40+
*/
41+
export async function stopAdminRemovedRuns(user: { id: string }): Promise<void> {
42+
const plan = pendingStops.get(user)
43+
pendingStops.delete(user)
44+
if (!plan) return
45+
try {
46+
await enqueueExecutionStops(db, plan)
47+
} catch (error) {
48+
logger.error('Failed to record the stops of a removed user’s runs', {
49+
userId: user.id,
50+
error: getErrorMessage(error),
51+
})
52+
} finally {
53+
await signalExecutionStops(plan.executions)
54+
}
55+
}

‎apps/sim/lib/auth/auth.ts‎

Lines changed: 4 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import { readAttributionProperties } from '@/lib/analytics/attribution'
3838
import { FREEBUFF_CLICK_ID_COOKIE } from '@/lib/analytics/freebuff'
3939
import { reportFreebuffConversion } from '@/lib/analytics/freebuff.server'
4040
import { getAccessControlConfig, isEmailBlockedByAccessControl } from '@/lib/auth/access-control'
41+
import { holdAdminRemovalStops, stopAdminRemovedRuns } from '@/lib/auth/admin-removal-stops'
4142
import { createAnonymousSession, ensureAnonymousUserExists } from '@/lib/auth/anonymous'
4243
import { buildConnectorProviders } from '@/lib/auth/connectors/providers'
4344
import {
@@ -146,7 +147,6 @@ import {
146147
consumeOAuthCredentialDraftBinding,
147148
processCredentialDraft,
148149
} from '@/lib/credentials/draft-processor'
149-
import type { ExecutionStopPlan } from '@/lib/execution/removal-stop-records'
150150
import { sendEmail } from '@/lib/messaging/email/mailer'
151151
import { getFromEmailAddress, getPersonalEmailFrom } from '@/lib/messaging/email/utils'
152152
import { quickValidateEmail } from '@/lib/messaging/email/validation'
@@ -182,14 +182,6 @@ import { SSO_TRUSTED_PROVIDERS } from '@/ee/sso/constants'
182182

183183
const logger = createLogger('Auth')
184184

185-
/**
186-
* Workflow runs `/admin/remove-user` must stop, read by the user delete's `before` hook while the
187-
* rows naming them exist and enqueued by its `after` hook once the delete has committed. Keyed by
188-
* the user entity Better Auth passes to both hooks of the same delete; a delete that fails never
189-
* reaches `after`, so its entry is dropped with the entity.
190-
*/
191-
const adminRemovalExecutionStops = new WeakMap<object, ExecutionStopPlan>()
192-
193185
/**
194186
* A Stripe price that maps to no catalog plan leaves the stored plan in place. That is expected
195187
* for enterprise (priced per customer) and for a legacy plan still on the price it was sold at;
@@ -481,41 +473,12 @@ export const auth = betterAuth({
481473
* usage rows cascade away without `deleteUserAccount`. Better Auth runs `after` once the
482474
* delete commits, which is when the stored reporting prefixes that counted them must be
483475
* discarded, and when the runs the cascade orphaned are stopped: in-flight ones at once,
484-
* queued ones by the outbox worker, since this module cannot load that delivery. The
485-
* delete is not transactional, so those runs are read just before it; a failed read lets
486-
* the removal proceed as before rather than blocking an admin.
476+
* queued ones by the outbox worker, since this module cannot load that delivery.
487477
*/
488478
delete: {
489-
before: async (user) => {
490-
try {
491-
const { collectAccountRemovalStops } = await import(
492-
'@/lib/execution/removal-stop-records'
493-
)
494-
adminRemovalExecutionStops.set(user, await collectAccountRemovalStops(db, user.id))
495-
} catch (error) {
496-
logger.error('[databaseHooks.user.delete.before] Failed to read runs to stop', {
497-
userId: user.id,
498-
error: getErrorMessage(error),
499-
})
500-
}
501-
},
479+
before: holdAdminRemovalStops,
502480
after: async (user) => {
503-
const executionStops = adminRemovalExecutionStops.get(user)
504-
adminRemovalExecutionStops.delete(user)
505-
if (executionStops) {
506-
try {
507-
const { enqueueExecutionStops, signalExecutionStops } = await import(
508-
'@/lib/execution/removal-stop-records'
509-
)
510-
await enqueueExecutionStops(db, executionStops)
511-
await signalExecutionStops(executionStops.executions)
512-
} catch (error) {
513-
logger.error('[databaseHooks.user.delete.after] Failed to stop removed runs', {
514-
userId: user.id,
515-
error: getErrorMessage(error),
516-
})
517-
}
518-
}
481+
await stopAdminRemovedRuns(user)
519482
await invalidateReportingUsagePrefixes()
520483
},
521484
},

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

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,7 @@ describe('DatabaseJobQueue workspace cancellation', () => {
515515
})
516516

517517
it('cancels only workflow job types, so the workspace’s other jobs keep running', async () => {
518-
await new DatabaseJobQueue().cancelByWorkspace('workspace-1')
518+
await new DatabaseJobQueue().cancelByWorkspace('workspace-1', { maxRuns: 1000 })
519519

520520
const typeFilter = flattenMockConditions(dbChainMockFns.where.mock.calls[0][0]).find(
521521
(node) => node.type === 'inArray' && node.column === asyncJobs.type
@@ -528,4 +528,40 @@ describe('DatabaseJobQueue workspace cancellation', () => {
528528
'workflow-group-cell',
529529
])
530530
})
531+
532+
const ids = (count: number, offset = 0) =>
533+
Array.from({ length: count }, (_, index) => ({ id: `job-${offset + index}` }))
534+
535+
it('keeps cancelling in batches until a short batch shows none are left', async () => {
536+
dbChainMockFns.returning
537+
.mockResolvedValueOnce(ids(100))
538+
.mockResolvedValueOnce(ids(100, 100))
539+
.mockResolvedValueOnce(ids(30, 200))
540+
541+
await expect(
542+
new DatabaseJobQueue().cancelByWorkspace('workspace-1', { maxRuns: 1000 })
543+
).resolves.toEqual({ cancelledJobs: 230, complete: true })
544+
})
545+
546+
it('stops at its run cap and reports the sweep incomplete', async () => {
547+
dbChainMockFns.returning.mockResolvedValueOnce(ids(100)).mockResolvedValueOnce(ids(50, 100))
548+
549+
await expect(
550+
new DatabaseJobQueue().cancelByWorkspace('workspace-1', { maxRuns: 150 })
551+
).resolves.toEqual({ cancelledJobs: 150, complete: false })
552+
expect(dbChainMockFns.returning).toHaveBeenCalledTimes(2)
553+
})
554+
555+
it('cancels nothing once its caller’s time is up', async () => {
556+
const controller = new AbortController()
557+
controller.abort()
558+
559+
await expect(
560+
new DatabaseJobQueue().cancelByWorkspace('workspace-1', {
561+
maxRuns: 1000,
562+
signal: controller.signal,
563+
})
564+
).resolves.toEqual({ cancelledJobs: 0, complete: false })
565+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
566+
})
531567
})

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

Lines changed: 43 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,13 @@ import {
1919
type JobType,
2020
validateMaxDurationSeconds,
2121
WORKSPACE_EXECUTION_JOB_TYPES,
22+
type WorkspaceJobCancellationOptions,
2223
type WorkspaceJobCancellationResult,
2324
} from '@/lib/core/async-jobs/types'
2425
import { recordExecutionCancellationBackendResult } from '@/lib/core/execution-limits/metrics'
2526

2627
const logger = createLogger('DatabaseJobQueue')
28+
const WORKSPACE_CANCELLATION_BATCH_SIZE = 100
2729

2830
type AsyncJobRow = typeof asyncJobs.$inferSelect
2931
type Runner = NonNullable<EnqueueOptions['runner']>
@@ -523,39 +525,52 @@ export class DatabaseJobQueue implements JobQueueBackend {
523525
* Cancels the workspace's pending and processing rows, so no process can claim them, and
524526
* aborts the runners this process holds for them. A runner another process holds stops through
525527
* its execution's cancellation signal instead. Batches run through `batchEnqueueAndWait` keep
526-
* no row and are left to that signal and their dispatch's own cancellation.
528+
* no row and are left to that signal and their dispatch's own cancellation. Works in batches
529+
* up to `maxRuns`, checking `signal` between them, like the Trigger.dev sweep.
527530
*/
528-
async cancelByWorkspace(workspaceId: string): Promise<WorkspaceJobCancellationResult> {
529-
const now = new Date()
530-
const cancelled = await db
531-
.update(asyncJobs)
532-
.set({
533-
status: JOB_STATUS.CANCELLED,
534-
completedAt: now,
535-
error: 'Cancelled',
536-
updatedAt: now,
537-
})
538-
.where(
539-
and(
540-
inArray(asyncJobs.status, [JOB_STATUS.PENDING, JOB_STATUS.PROCESSING]),
541-
inArray(asyncJobs.type, [...WORKSPACE_EXECUTION_JOB_TYPES]),
542-
sql`${asyncJobs.metadata}->>'workspaceId' = ${workspaceId}`
531+
async cancelByWorkspace(
532+
workspaceId: string,
533+
options: WorkspaceJobCancellationOptions
534+
): Promise<WorkspaceJobCancellationResult> {
535+
const active = and(
536+
inArray(asyncJobs.status, [JOB_STATUS.PENDING, JOB_STATUS.PROCESSING]),
537+
inArray(asyncJobs.type, [...WORKSPACE_EXECUTION_JOB_TYPES]),
538+
sql`${asyncJobs.metadata}->>'workspaceId' = ${workspaceId}`
539+
)
540+
let cancelledJobs = 0
541+
let complete = false
542+
while (cancelledJobs < options.maxRuns && !options.signal?.aborted) {
543+
const batchSize = Math.min(WORKSPACE_CANCELLATION_BATCH_SIZE, options.maxRuns - cancelledJobs)
544+
const now = new Date()
545+
const cancelled = await db
546+
.update(asyncJobs)
547+
.set({ status: JOB_STATUS.CANCELLED, completedAt: now, error: 'Cancelled', updatedAt: now })
548+
.where(
549+
and(
550+
active,
551+
inArray(
552+
asyncJobs.id,
553+
db.select({ id: asyncJobs.id }).from(asyncJobs).where(active).limit(batchSize)
554+
)
555+
)
543556
)
544-
)
545-
.returning({ id: asyncJobs.id })
557+
.returning({ id: asyncJobs.id })
546558

547-
for (const { id } of cancelled) {
548-
const controller = inlineAbortControllers.get(id)
549-
if (!controller) continue
550-
controller.abort(new DOMException('user', 'AbortError'))
551-
inlineAbortControllers.delete(id)
559+
for (const { id } of cancelled) {
560+
const controller = inlineAbortControllers.get(id)
561+
if (!controller) continue
562+
controller.abort(new DOMException('user', 'AbortError'))
563+
inlineAbortControllers.delete(id)
564+
}
565+
cancelledJobs += cancelled.length
566+
if (cancelled.length < batchSize) {
567+
complete = true
568+
break
569+
}
552570
}
553571

554-
logger.info('Cancelled database jobs for workspace', {
555-
workspaceId,
556-
cancelledJobs: cancelled.length,
557-
})
558-
return { cancelledJobs: cancelled.length, complete: true }
572+
logger.info('Cancelled database jobs for workspace', { workspaceId, cancelledJobs, complete })
573+
return { cancelledJobs, complete }
559574
}
560575

561576
cancelByKey(cancelKey: string): boolean {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ function scopeCondition(executor: DbOrTx, scope: ExecutionStopScope): SQL | unde
7373
* removal is not transactional, immediately before it): a cascade erases these rows. An active
7474
* resume runs under its own execution id, so the claimed resume of a paused run is included.
7575
*/
76-
async function collectExecutionStops(
76+
export async function collectExecutionStops(
7777
executor: DbOrTx,
7878
scope: ExecutionStopScope
7979
): Promise<ExecutionStopPlan> {

0 commit comments

Comments
 (0)