Skip to content

Commit 4abf873

Browse files
committed
fix(mothership): harden abandoned-run takeover
- Add the `mothership-abandoned-run-takeover-disabled` AppConfig kill switch, checked once per scan; off by default, so takeovers run unless it is on. - Stop a controller whose chat lock lapsed while every renewal threw, so a controller partitioned from Redis cannot keep driving a run taken over. - Never revoke another call awaiting its re-run from a sibling's wait or workbench access; the sibling's workbench access still stops that call's dead attempt's commands. - Treat a workspace access loss found inside recovery like an authorization denial, by error type; other recovery failures stay recoverable. - Cover organization Assistant takeover for a member and a removed member.
1 parent 18f650f commit 4abf873

10 files changed

Lines changed: 303 additions & 29 deletions

File tree

‎apps/sim/lib/core/config/feature-flags.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,13 @@ const FEATURE_FLAGS = {
120120
'targeting. Off-AppConfig falls back to KNOWLEDGE_MEMBER_ACCESS.',
121121
fallback: 'KNOWLEDGE_MEMBER_ACCESS',
122122
},
123+
'mothership-abandoned-run-takeover-disabled': {
124+
description:
125+
'Kill switch for Sim taking over Chat runs whose controller died (a deploy or a crash) and ' +
126+
're-running their interrupted tool. Global on/off only; on stops every takeover scan. ' +
127+
'Off by default, so takeovers run unless this is turned on.',
128+
fallback: null,
129+
},
123130
} satisfies Record<string, FeatureFlagDefinition>
124131

125132
/**

‎apps/sim/lib/desktop/executor/bound-turn.integration.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,7 +511,7 @@ describe.runIf(Boolean(redisUrl))("a turn bound to a desktop's background execut
511511

512512
await lapse(toolCallId, 'lease')
513513
/** A Sim tool waiting on the same run sweeps its expired executions; this one is not its. */
514-
await revokeExpiredSimToolExecutions({ runId: run.runId, userId })
514+
await revokeExpiredSimToolExecutions({ runId: run.runId, userId, toolCallId: 'waiting-call' })
515515
expect((await storedCall(toolCallId)).status).toBe('running')
516516
await answer
517517
const late = await desktop.complete(toolCallId, executionToken, { output: 'built' })

‎apps/sim/lib/mothership/async-runs/repository.ts‎

Lines changed: 51 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import {
2424
like,
2525
lt,
2626
ne,
27+
not,
2728
notInArray,
2829
or,
2930
type SQL,
@@ -757,6 +758,18 @@ const interruptedSimExecution = and(
757758
lt(copilotAsyncToolCalls.executionAttempts, SIM_TOOL_EXECUTION_MAX_ATTEMPTS)
758759
)
759760

761+
/**
762+
* An interrupted execution a takeover may still run again: its run still admits tools. Another
763+
* call's wait or workbench access must not revoke it as interrupted before that re-run.
764+
*/
765+
const rerunnableSimExecution = and(
766+
interruptedSimExecution,
767+
sql`EXISTS (SELECT 1 FROM ${copilotRuns} r WHERE r.id = ${copilotAsyncToolCalls.runId} AND r.tool_admission_closed_at IS NULL AND r.status NOT IN (${sql.join(
768+
TERMINAL_RUN_STATUSES.map((status) => sql`${status}`),
769+
sql`, `
770+
)}))`
771+
)
772+
760773
/**
761774
* Sim claims a call it is about to execute under an execution lease; a terminal result never
762775
* releases it. The claim takes a call's first execution, or re-runs one whose owner died
@@ -994,12 +1007,23 @@ export async function hasLiveSimToolExecution(runId: string): Promise<boolean> {
9941007
return !!live
9951008
}
9961009

997-
/** Revocation ends local execution authority; recorded remote commands remain independently unsettled. */
998-
export async function revokeExpiredSimToolExecutions(input: { runId: string; userId: string }) {
1010+
/**
1011+
* Revocation ends local execution authority; recorded remote commands remain independently
1012+
* unsettled. The waiting call's own lapsed execution is revoked; another call's is spared while
1013+
* a takeover may still run it again.
1014+
*/
1015+
export async function revokeExpiredSimToolExecutions(input: {
1016+
runId: string
1017+
userId: string
1018+
toolCallId: string
1019+
}) {
9991020
return db.transaction((tx) =>
10001021
revokeExpiredExecutions(
10011022
tx,
1002-
sql`EXISTS (SELECT 1 FROM ${copilotRuns} r WHERE r.id = ${copilotAsyncToolCalls.runId} AND r.id = ${input.runId}::uuid AND r.user_id = ${input.userId})`
1023+
and(
1024+
sql`EXISTS (SELECT 1 FROM ${copilotRuns} r WHERE r.id = ${copilotAsyncToolCalls.runId} AND r.id = ${input.runId}::uuid AND r.user_id = ${input.userId})`,
1025+
or(eq(copilotAsyncToolCalls.toolCallId, input.toolCallId), not(rerunnableSimExecution))
1026+
)
10031027
)
10041028
)
10051029
}
@@ -1358,7 +1382,11 @@ export async function prepareWorkbenchAccess(
13581382
)
13591383
await revokeExpiredExecutions(
13601384
tx,
1361-
sql`EXISTS (SELECT 1 FROM ${copilotRuns} r WHERE r.id = ${copilotAsyncToolCalls.runId} AND r.chat_id = ${owner.chatId} AND r.user_id = ${input.userId})`
1385+
and(
1386+
sql`EXISTS (SELECT 1 FROM ${copilotRuns} r WHERE r.id = ${copilotAsyncToolCalls.runId} AND r.chat_id = ${owner.chatId} AND r.user_id = ${input.userId})`,
1387+
/** This run was just verified to admit tools; every older run's admission just closed. */
1388+
not(and(eq(copilotAsyncToolCalls.runId, input.runId), interruptedSimExecution))
1389+
)
13621390
)
13631391
const previous = await tx
13641392
.select({
@@ -1393,6 +1421,20 @@ export async function prepareWorkbenchAccess(
13931421
* A re-run's earlier attempts may have left commands running on the remote workbench, which
13941422
* outlive the Sim process that started them; they are stopped like any earlier call's.
13951423
*/
1424+
/** A sibling awaiting its re-run has no live handler, but its dead attempt's commands may still run. */
1425+
const rerunnableSiblings = await tx
1426+
.select({
1427+
toolCallId: copilotAsyncToolCalls.toolCallId,
1428+
processes: copilotAsyncToolCalls.sandboxProcesses,
1429+
})
1430+
.from(copilotAsyncToolCalls)
1431+
.where(
1432+
and(
1433+
eq(copilotAsyncToolCalls.runId, input.runId),
1434+
ne(copilotAsyncToolCalls.toolCallId, input.toolCallId),
1435+
interruptedSimExecution
1436+
)
1437+
)
13961438
const earlierAttempts = {
13971439
toolCallId: input.toolCallId,
13981440
processes: Object.fromEntries(
@@ -1401,7 +1443,7 @@ export async function prepareWorkbenchAccess(
14011443
)
14021444
),
14031445
}
1404-
const processes = [...previous, earlierAttempts].flatMap((row) =>
1446+
const processes = [...previous, ...rerunnableSiblings, earlierAttempts].flatMap((row) =>
14051447
Object.entries(row.processes).flatMap(([id, process]) =>
14061448
process.settled
14071449
? []
@@ -1594,9 +1636,10 @@ export async function claimWorkflowToolExecution(
15941636
/** Only the server re-runs a workflow whose Sim executor died; no browser resumes it. */
15951637
executor === 'sim' ? interruptedSimExecution : undefined,
15961638
/**
1597-
* A server claim whose claimant died before starting the execution. Starting is
1598-
* fenced by `claimToolExecution`, so a claimant still alive and this one cannot
1599-
* both run it: the second to start observes the first's result.
1639+
* A server claim that never started, such as one whose claimant died between
1640+
* claiming and starting. Nothing here checks the claimant is gone: what keeps
1641+
* the call to one execution is `claimToolExecution`, the only start, so if the
1642+
* earlier claimant is alive whichever starts second observes the first's result.
16001643
*/
16011644
executor === 'sim'
16021645
? and(

‎apps/sim/lib/mothership/persistence/tool-confirm/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ export async function waitForToolConfirmation(
156156
const checkDurableConfirmation = async (source: 'subscribe' | 'pubsub' | 'poll') => {
157157
if (options.executionScope) {
158158
try {
159-
await revokeExpiredSimToolExecutions(options.executionScope)
159+
await revokeExpiredSimToolExecutions({ ...options.executionScope, toolCallId })
160160
} catch (error) {
161161
logger.warn('Could not verify retained tool execution ownership', {
162162
toolCallId,

‎apps/sim/lib/mothership/persistence/tool-confirm/tool-confirm.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,10 @@ describe('copilot orchestrator persistence', () => {
308308
})
309309
await vi.advanceTimersByTimeAsync(5_000)
310310
expect(await waiting).toMatchObject({ status: 'error', message: 'Outcome unknown' })
311-
expect(revokeExpiredSimToolExecutions).toHaveBeenCalledWith(scope)
311+
expect(revokeExpiredSimToolExecutions).toHaveBeenCalledWith({
312+
...scope,
313+
toolCallId: 'tool-1',
314+
})
312315
} finally {
313316
vi.useRealTimers()
314317
}

0 commit comments

Comments
 (0)