Skip to content

Commit 7ddde08

Browse files
committed
refactor(tables): name the dispatch liveness predicate and bound its fan-out
Extracts the cell-activity check into `hasRecentCellActivity`, so the stale predicate reads as its two conditions — nothing beating, nothing executing — rather than a twenty-line SQL blob nested inside an `and()`. No behaviour change; this is the code three review rounds found defects in, and being able to read it is what makes those defects findable. Bounds the terminal-event fan-out with `mapWithConcurrency`, matching how the scheduler already fans out. The sibling cancel paths emit over one table's dispatches; this sweep can carry a whole tick's worth across many tables, and each event is its own write. Also repairs the test that covers it. `collectChunks` walks into the `tableRowExecutions` table object the fragment interpolates, so every column name appears in the chunks whether the predicate references it or not — the group, row, table and timestamp assertions all passed with their predicates deleted. Matching the literal SQL instead makes them fail, which mutating each clause now confirms.
1 parent 0a985c7 commit 7ddde08

2 files changed

Lines changed: 80 additions & 49 deletions

File tree

apps/sim/lib/table/dispatcher.ts

Lines changed: 63 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
sql,
1818
} from 'drizzle-orm'
1919
import { getJobQueue } from '@/lib/core/async-jobs/config'
20+
import { mapWithConcurrency } from '@/lib/core/utils/concurrency'
2021
import { writeWorkflowGroupState } from '@/lib/table/cell-write'
2122
import { USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants'
2223
import { isExecCancelledAfter } from '@/lib/table/deps'
@@ -43,6 +44,9 @@ const logger = createLogger('TableRunDispatcher')
4344

4445
const ACTIVE_DISPATCH_STATUSES = ['pending', 'dispatching'] as const
4546

47+
/** Concurrent terminal-event writes when the stale sweep reclaims a batch. */
48+
const STALE_DISPATCH_EVENT_CONCURRENCY = 10
49+
4650
export type DispatchStatus = 'pending' | 'dispatching' | 'complete' | 'cancelled'
4751
export type DispatchMode = 'all' | 'incomplete' | 'new'
4852

@@ -812,6 +816,44 @@ export async function completeDispatchIfActive(dispatchId: string): Promise<bool
812816
return transitioned.length > 0
813817
}
814818

819+
/**
820+
* Whether any cell inside a dispatch's own scope has reported since `since`.
821+
*
822+
* Scoped to the dispatch's groups, and to its rows when it names any, because
823+
* `table_row_executions` carries no dispatch column. Left table-wide, a live
824+
* dispatch's cells read as evidence that an abandoned dispatch beside it was
825+
* still working — and auto-fired and row-scoped runs do NOT cancel overlapping
826+
* dispatches (`cancelPriorRuns` requires `isManualRun`, and the per-row path is
827+
* a no-op for dispatch cancellation), so sharing a group is ordinary rather than
828+
* exceptional.
829+
*
830+
* Two table-wide dispatches over the same groups can still vouch for each other,
831+
* since nothing in the row execution says whose work it is. Closing that needs a
832+
* `dispatch_id` on `table_row_executions`, threaded through every cell-write
833+
* site. Until then the residue is a delay, not a permanent mask: the live
834+
* dispatch's cells stop reporting when it finishes.
835+
*
836+
* Rides the partial `(table_id, status)` index, which covers exactly these three
837+
* statuses.
838+
*/
839+
function hasRecentCellActivity(since: Date): SQL {
840+
return sql`EXISTS (
841+
SELECT 1 FROM ${tableRowExecutions}
842+
WHERE ${tableRowExecutions.tableId} = ${tableRunDispatches.tableId}
843+
AND ${tableRowExecutions.groupId} IN (
844+
SELECT jsonb_array_elements_text(${tableRunDispatches.scope} -> 'groupIds')
845+
)
846+
AND (
847+
jsonb_typeof(${tableRunDispatches.scope} -> 'rowIds') <> 'array'
848+
OR ${tableRowExecutions.rowId} IN (
849+
SELECT jsonb_array_elements_text(${tableRunDispatches.scope} -> 'rowIds')
850+
)
851+
)
852+
AND ${tableRowExecutions.status} IN ('queued', 'running', 'pending')
853+
AND ${tableRowExecutions.updatedAt} >= ${sql.param(since, tableRowExecutions.updatedAt)}
854+
)`
855+
}
856+
815857
/**
816858
* Cancels dispatches whose holder died without reaching a terminal state.
817859
*
@@ -881,21 +923,7 @@ export async function cancelStaleDispatches(
881923
and(
882924
inArray(tableRunDispatches.status, [...ACTIVE_DISPATCH_STATUSES]),
883925
sql`COALESCE(${tableRunDispatches.heartbeatAt}, ${tableRunDispatches.requestedAt}) < ${sql.param(staleBefore, tableRunDispatches.heartbeatAt)}`,
884-
sql`NOT EXISTS (
885-
SELECT 1 FROM ${tableRowExecutions}
886-
WHERE ${tableRowExecutions.tableId} = ${tableRunDispatches.tableId}
887-
AND ${tableRowExecutions.groupId} IN (
888-
SELECT jsonb_array_elements_text(${tableRunDispatches.scope} -> 'groupIds')
889-
)
890-
AND (
891-
jsonb_typeof(${tableRunDispatches.scope} -> 'rowIds') <> 'array'
892-
OR ${tableRowExecutions.rowId} IN (
893-
SELECT jsonb_array_elements_text(${tableRunDispatches.scope} -> 'rowIds')
894-
)
895-
)
896-
AND ${tableRowExecutions.status} IN ('queued', 'running', 'pending')
897-
AND ${tableRowExecutions.updatedAt} >= ${sql.param(staleBefore, tableRowExecutions.updatedAt)}
898-
)`
926+
sql`NOT ${hasRecentCellActivity(staleBefore)}`
899927
)
900928

901929
// Claimed as explicit ids first, then updated by id, so the bound is evaluated
@@ -939,22 +967,26 @@ export async function cancelStaleDispatches(
939967
requestedAt: row.requestedAt,
940968
}))
941969

942-
// Same terminal event every other cancel path emits — without it the row goes
943-
// terminal in the database while the client overlay stays stuck, which is the
944-
// symptom this function exists to clear.
945-
await Promise.all(
946-
dispatches.map((d) =>
947-
appendTableEvent({
948-
kind: 'dispatch',
949-
tableId: d.tableId,
950-
dispatchId: d.id,
951-
status: 'cancelled',
952-
scope: d.scope,
953-
cursor: d.cursor,
954-
mode: d.mode,
955-
isManualRun: d.isManualRun,
956-
})
957-
)
970+
/**
971+
* Same terminal event every other cancel path emits — without it the row goes
972+
* terminal in the database while the client overlay stays stuck, which is the
973+
* symptom this function exists to clear.
974+
*
975+
* Bounded rather than a bare `Promise.all`: the sibling cancel paths fan out
976+
* over one table's dispatches, while this sweep can carry a whole tick's worth
977+
* across many tables, and each event is its own write.
978+
*/
979+
await mapWithConcurrency(dispatches, STALE_DISPATCH_EVENT_CONCURRENCY, (d) =>
980+
appendTableEvent({
981+
kind: 'dispatch',
982+
tableId: d.tableId,
983+
dispatchId: d.id,
984+
status: 'cancelled',
985+
scope: d.scope,
986+
cursor: d.cursor,
987+
mode: d.mode,
988+
isManualRun: d.isManualRun,
989+
})
958990
)
959991

960992
return dispatches

apps/sim/lib/table/stale-dispatch-recovery.test.ts

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -103,33 +103,32 @@ describe('cancelStaleDispatches', () => {
103103
it('spares a dispatch whose cells are still reporting', async () => {
104104
await cancelStaleDispatches(STALE_BEFORE, 200)
105105

106-
const chunks = collectChunks(dbChainMockFns.where.mock.calls[0][0])
106+
/**
107+
* Matched on the literal SQL, not on column references: the fragment
108+
* interpolates the `tableRowExecutions` table object, so every one of its
109+
* column names appears in the chunks whether the predicate uses it or not —
110+
* asserting on those passes even with the predicate deleted.
111+
*/
112+
const joined = collectChunks(dbChainMockFns.where.mock.calls[0][0]).join(' ')
113+
107114
/**
108115
* The dispatch's own heartbeat is stamped between windows, not during them,
109116
* and the loop is checkpointed for the whole window — so a long window
110117
* leaves it untouched while the dispatch is plainly alive. Its cells carry
111118
* the signal the checkpointed parent cannot, and both have to be stale
112119
* before the row is reclaimed.
113120
*/
114-
expect(chunks.some((chunk) => chunk.includes('NOT EXISTS'))).toBe(true)
115-
expect(chunks).toContain('tableRowExecutions.updatedAt')
116-
expect(chunks).toContain('tableRowExecutions.tableId')
117-
/**
118-
* Narrowed to the dispatch's own groups: row executions carry no dispatch
119-
* column, so a table-wide probe lets a LIVE dispatch's cells vouch for an
120-
* ABANDONED one beside it, and the abandoned row is never reclaimed.
121-
*/
122-
expect(chunks).toContain('tableRowExecutions.groupId')
123-
expect(chunks.some((chunk) => chunk.includes('jsonb_array_elements_text'))).toBe(true)
121+
expect(joined).toContain('NOT ')
122+
expect(joined).toContain('EXISTS (')
123+
124124
/**
125-
* Rows too, when the dispatch names any. Auto-fired and row-scoped runs do
126-
* NOT cancel overlapping dispatches — `cancelPriorRuns` requires
127-
* `isManualRun`, and the per-row path is a no-op for dispatch cancellation —
128-
* so a live dispatch sharing a group is ordinary, and only the row filter
129-
* keeps its cells from vouching for an abandoned neighbour.
125+
* Scoped to the dispatch's own groups AND its rows. Auto-fired and
126+
* row-scoped runs do NOT cancel overlapping dispatches, so a live dispatch
127+
* sharing a table is ordinary — without both filters its cells vouch for an
128+
* abandoned neighbour and the abandoned row is never reclaimed.
130129
*/
131-
expect(chunks).toContain('tableRowExecutions.rowId')
132-
expect(chunks.some((chunk) => chunk.includes("'rowIds'"))).toBe(true)
130+
expect(joined).toContain("-> 'groupIds'")
131+
expect(joined).toContain("-> 'rowIds'")
133132
})
134133

135134
it('emits the terminal event so a stuck client overlay clears', async () => {

0 commit comments

Comments
 (0)