fix(knowledge,tables): recover abandoned dispatches, bound the sweep and the workbook preview - #6945
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Abandoned table dispatches are now reclaimed by the stale-execution cron via OOM retries actually happen: Trigger.dev Knowledge/parser: stuck-document retries are oldest-first and capped at 200 per sync. Terminal connector sync logs older than 30 days are pruned without dropping the newest (or newest Reviewed by Cursor Bugbot for commit e7b1301. Configure here. |
Greptile SummaryThis PR adds bounded recovery and retention for abandoned table dispatches and connector logs, limits connector reconciliation and workbook parsing work, fixes OfficeParser module loading, and configures memory escalation for background tasks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/table/dispatcher.ts | Adds heartbeat-based abandoned-dispatch recovery and consistently guarded claim, completion, and pre-window cancellation handling. |
| apps/sim/app/api/cron/cleanup-stale-executions/route.ts | Integrates bounded dispatch recovery and connector sync-log retention into the existing authenticated cleanup cron. |
| packages/db/migrations/0299_bright_nemesis.sql | Adds the nullable heartbeat column used by dispatch recovery without requiring a table rewrite or backfill. |
| apps/sim/lib/knowledge/connectors/sync-engine.ts | Bounds and orders connector reconciliation candidates to prevent unbounded shared-queue fan-out. |
| apps/sim/lib/file-parsers/xlsx-parser.ts | Applies the workbook preview range limit during conversion rather than after potentially excessive allocation. |
| apps/sim/lib/file-parsers/officeparser-module.ts | Normalizes OfficeParser’s CommonJS/default and named-export module shapes for bundled workers. |
| apps/sim/background/table-run-dispatcher.ts | Increases dispatcher memory capacity and adds explicit OOM retry escalation. |
| apps/sim/background/knowledge-processing.ts | Adds explicit OOM retry escalation for document-processing tasks. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Cron[Stale-execution cron] --> Sweep[Find dispatches with stale heartbeat]
Sweep --> Activity{Recent scoped cell activity?}
Activity -->|No| Cancel[Conditionally mark cancelled]
Activity -->|Yes, below absolute ceiling| Spare[Leave dispatch active]
Activity -->|Yes, beyond absolute ceiling| Cancel
Cancel --> Event[Emit bounded cancellation events]
Dispatcher[Dispatcher window loop] --> Heartbeat[Advance cursor/count and heartbeat]
Heartbeat --> Sweep
Reviews (10): Last reviewed commit: "fix(tables): re-read the dispatch before..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b347f91. Configure here.
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 830b82f. Configure here.
Three defects measured in production this afternoon. A dispatcher killed by an OOM left `table_run_dispatches` at `dispatching` forever. Every terminal transition on that table is user- or flow-initiated, so nothing reclaimed the row: four dispatches were stranded in one afternoon, pinning each table's "X running" overlay and blocking re-runs, with no way to clear them from the product. The `table_run_dispatches_watchdog_idx` index has existed for this sweep since the table was created, unused. Liveness comes from a new `heartbeat_at`, stamped by the per-window writes that already advance `cursor` and `processed_count`, so a slow-but-live dispatch is spared however long it runs — the in-process path has no duration ceiling, so ageing from `requested_at` would reclaim live self-hosted work. The sweep reads `COALESCE(heartbeat_at, requested_at)` so rows written before the column stay reclaimable rather than NULL-false forever, and runs as the last arm of the existing stale-execution cron at the same 95-minute window its table-job sibling uses. Rows are cancelled, not completed: the scope never finished. The OOM itself is not a leak. Peak RSS is a flat plateau — 457 MB at 20-45s and 461 MB past 200s, so ten times the duration buys four megabytes — that has crept about two percent per release for a month, from 446 MB in late July to 545 MB, past the 512 MiB `small-1x` ceiling. CPU peaks at 0.19, so the larger preset is bought for RAM alone. `maxAttempts` never covered the kill either: Trigger.dev retries `TASK_PROCESS_OOM_KILLED` only when `retry.outOfMemory.machine` names a preset, and all four runs recorded `attempt_count = 1` while the docstring claimed they resumed from the persisted cursor. The connector stuck-document sweep dispatched without a bound. Its chunk size paced the loop but the candidate query had no limit, so one connector enqueued 2,959 documents in fifteen seconds onto the queue every workspace shares. Nothing was double-billed — those documents were genuinely unindexed — but one connector monopolized the queue, and each dispatch mints a fresh requestId, so the idempotency key differs every pass and none of it deduplicates. Candidates are now taken oldest-first and capped per sync; a deeper backlog is deferred to the next sync rather than dropped.
`officeparser` is CommonJS — `main: officeParser.js`, no `type`, no `exports`
map — so what `await import('officeparser')` yields depends on who built the
code. Node and webpack synthesize named exports from `module.exports`, so
`.parseOfficeAsync` is there. esbuild, which builds the Trigger.dev worker
bundle, puts `module.exports` on `.default` and leaves the named export
undefined, and the package is in neither `build.external` nor
`additionalPackages`, so it is bundled.
Reading the named export directly therefore worked everywhere except the
worker, where calling it threw `TypeError: parseOfficeAsync is not a function`.
All four parsers treat that as "the library failed" and answer with a scrape of
the archive, which returns `degraded: true`, and the document pipeline rejects a
degraded parse outright. The visible result was every `.pptx` and legacy `.doc`
reporting "No text could be extracted from this file — it may be scanned,
image-only, or password-protected", naming a cause that had nothing to do with
the fault. 118 pptx and 14 doc failures landed in a single burst when one
connector's sync first succeeded after ten consecutive crashes.
Resolved in one shared loader rather than per bundler: externalizing the package
has to be repeated in every build config this code runs under and regresses
silently the day one is missed.
The shape handling is split into a pure `resolveParseOfficeAsync` because the
failing shape cannot be reproduced by mocking the specifier — Vitest's
module-namespace proxy throws on a missing export rather than yielding the
`undefined` a real bundle produces, so a test going through `import` can only
assert the shape that already worked. That is also why the existing parser
suites never caught this: each mocks `officeparser` with a fabricated named
export, which presupposes the interop being broken here.
`sheet_to_json` allocates from a worksheet's DECLARED `!ref` range rather than its populated cells, and Excel routinely writes an inflated range from stray formatting. The 1,000-row preview cap was applied to the result, so it bounded the emitted string while the allocation it was meant to bound had already happened. An 880 KB workbook exhausted an 8 GB worker; the same content exhausted 16 GB when this ran inside the connector sync. No machine size fixes that, because the allocation scales with a number the file declares about itself — fleet p99 for this task is 691 MB against 8 GB, so this is a cliff, not pressure. Passing the window into the conversion is what makes the cap real. `defval` goes with it: defaulting every cell in the range made each row dense, so allocation scaled with columns x declared rows rather than with populated cells, and because no row was left empty it silently defeated the `blankrows: false` beside it. Reported totals still come from the declared range, so bounding the conversion does not change what the metadata says the workbook holds. The eleven documents killed this way recorded `attempt_count = 1`: `maxAttempts` does not cover `TASK_PROCESS_OOM_KILLED`, which Trigger.dev retries only when a larger preset is named. Adding that escalation is a safety net rather than the fix, and the same gap the dispatcher had. Also corrects the machine comment, which claimed `large-1x` was 2 vCPU / 2 GB. It is 4 vCPU / 8 GB, and believing the stale figure makes a resize look like the answer when the parser is what is unbounded.
|
@cursor review |
The liveness probe cannot tell whose cells it is looking at when two table-wide dispatches share a group, because `table_row_executions` carries no dispatch column. On a quiet table that is only a delay — the neighbour finishes and the next sweep reclaims — but a busy table with continuous auto-fired work can keep an abandoned dispatch masked indefinitely, which is the stuck overlay this sweep exists to clear. A ceiling bounds it: past a day without a heartbeat, a dispatch is reclaimed whatever its cells are doing. That is safe because a live dispatch stamps its heartbeat between windows regardless of cell activity, so only a single window outliving the ceiling could be reclaimed wrongly, and no window lasts a day on any path — the Trigger.dev run ceiling is ninety minutes. The real fix is a `dispatch_id` on the executions row. Threading it through the patch layer and the upserts underneath it is a change to the hottest write path in tables and belongs in its own review, not on the sixth round of this one.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 65ed5e6. Configure here.
`COALESCE(heartbeat_at, requested_at)` was written twice — once for the stale threshold and again for the absolute ceiling — so the two could drift into disagreeing about what proof of life means. One `lastBeat` fragment, one `notBeatingSince(cutoff)` helper, both cutoffs expressed through it. Also corrects the ceiling's comment: it triggers a day past the stale threshold, not a day past now.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 787c845. Configure here.
…ourth time The two pre-claim exits — table missing, no target groups — still wrote through `markDispatchComplete`. Last round I argued they run before the claim, "where forcing a terminal state is the intent". That was wrong twice over: the table lookup is awaited, so a cancel lands in that window like any other, and a dispatch cancelled mid-lookup has not completed its scope any more than one cancelled mid-window has. Routing them through `completeDispatchIfActive` left `markDispatchComplete` with no callers, so it is gone. That is the part worth having: this is the fourth place the same defect appeared, each time because an unguarded writer was sitting there to be reached. With it deleted, `completeDispatchIfActive` is the only way to complete a dispatch and the class cannot recur.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 946dd8f. Configure here.
Several round trips separate the claim from the enqueue — the window query, the executions prefetch, the tombstone filter — and nothing rechecked the dispatch across them. A Stop-all or the stale sweep landing in that gap had the step stamp cells and run a whole window for a dispatch already recorded as cancelled; the existing recheck sits after the window, which is too late to prevent it. Mirrors that existing check on the other side of the enqueue. It narrows the gap to a single statement rather than closing it — a cancel arriving after this read still races the enqueue, and no check can fix that. The cell-level `cancellationGuard` and the `isExecCancelledAfter` tombstone filter are what catch the remainder.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e7b1301. Configure here.
1 participant
Summary
Five defects, each measured in production rather than inferred.
table_run_dispatchesis user- or flow-initiated, so a dispatcher killed mid-loop left its rowdispatchingforever — pinning the table's "X running" overlay and blocking re-runs, with no way to clear it from the product. Liveness now comes from aheartbeat_atstamped by the per-window writes that already advancecursorandprocessed_count, so a slow-but-live dispatch is spared however long it runs — the in-process path has no duration ceiling, so ageing fromrequested_atwould reclaim live self-hosted work. ReadsCOALESCE(heartbeat_at, requested_at)so rows predating the column stay reclaimable. Runs as the last arm of the existing stale-execution cron. Thetable_run_dispatches_watchdog_idxindex had existed for this sweep since the table was created, unused.small-1xceiling. CPU peaks at 0.19, so the larger preset is bought for RAM alone.maxAttemptsnever covered an OOM. Trigger.dev retriesTASK_PROCESS_OOM_KILLEDonly whenretry.outOfMemory.machinenames a preset. Affected runs recordedattempt_count = 1while a docstring claimed they resumed from the persisted cursor. Fixed on both the dispatcher and the document task.officeparser's named export is undefined under esbuild. It is CommonJS, and the worker bundle putsmodule.exportsondefault, so calling the named export threwTypeError. Every parser reads that as a library failure and answers with adegradedscrape the pipeline rejects — so.pptxand legacy.docreported "No text could be extracted… scanned, image-only, or password-protected", naming a cause unrelated to the fault. Resolved in one shared loader rather than per bundler.sheet_to_jsonallocates from a worksheet's declared!ref, not its populated cells, and the row cap was applied to the result — bounding the emitted string while the allocation it was meant to bound had already happened. Sub-megabyte workbooks exhausted an 8 GB worker, and 16 GB when this ran inside the sync. Passing the window into the conversion is what makes the cap real;defvalgoes with it, since defaulting every cell made rows dense and silently defeated theblankrows: falsebeside it.Type of Change
Testing
type-check,lint, 32/32 audits,check:migrationsbackward-compatible.COALESCEfallback, flipping cancelled to complete, removing the terminal event, removing either limit, removing the sweep ordering, reverting either machine or OOM fallback, restoringdefval, and reverting the parser to named-export-only each turn a named test red.ADD COLUMN— additive, no rewrite, no default, and ignored by the currently deployed code.Checklist