Skip to content

Commit 898e439

Browse files
fix(plugin-approvals): a revise-window recall on a batch item finds its batch run past the first 1000 parked runs (#28268)
<!-- claude-projects-attribution: {"github_login":"os-elon-musk"} --> _Requested by **Elon** · [project thread](https://claude.ai/code/project/chan_01WedDzV26nniwxv7jC88B9D?thread=cmsg_01WedDzV26nniwxv7jC88B9DQALeiHN1aR44XyBgGozrec)_ domain:server#4 · os-elon-musk · session_016WV4d1FQJt7HxgWZZ5cYz9 Fixes #28230 Clause-②: no (a recall on a store with more than 1000 parked runs now wakes its batch, as it already does below that; no schema, accept set, error code or result field changes) Metadata-protocol: none ## 维护者速读 - 改了什么:批量签批里,修改窗口内撤回某一项时,撤回改为在 `cancelRun` 之前按该项 run 自身的 `$parentRunId` / `$parentMapNode` 键读出它上面的批次 run(新增引擎成员 `AutomationEngine.suspendedMapParentOf`),不再扫 `listSuspendedRunsDurable()` 的 1000 行首页。 - 为什么改:实测 main(f40b777d):两个副本共用一个 `ObjectStoreSuspendedRunStore`,批次之前已挂起 1000 条别的 run,在另一副本上撤回:该项被取消,批次 run 一直停在这一项上,第 2 项不开,`resumeError` 为空,什么都不说;998 条时一切正常。 - 风险与回滚:只换了“找批次”这一步;找到之后的恢复、只认内置 `map` 节点的检查、失败的告知方式都没变。新成员只读,两次按 id 读,存储读不出时抛错并照旧写进 `resumeError`。回滚即 revert 本 PR。 - 先看:`packages/services/service-automation/src/engine.ts` 的 `suspendedMapParentOf`,与 `approval-service.ts` 里 `recall` 修改窗口分支和新私有方法 `batchAboveItem`。 - 两分钟验证:`pnpm --filter @objectstack/plugin-approvals exec vitest run src/batch-item-recall-resumes-batch.test.ts`(预期 9 passed,需先 build `@objectstack/service-automation`)与 `pnpm --filter @objectstack/service-automation exec vitest run src/suspended-map-parent-of.test.ts`(预期 5 passed)。 - 需要你做的:无,席位在 CI 绿后按落地授权落地。 ## What changes - **`packages/services/service-automation/src/engine.ts`: one new public read, `suspendedMapParentOf(itemRunId)`.** It loads the item run's suspension by id (the strict, store-authoritative loader `hasSuspendedRun` uses), reads the `$parentRunId` / `$parentMapNode` the `map` node launched it with, loads the parent's suspension by that id, and answers `{ runId, flowName, nodeId, correlation }` only while the parent is parked on `map:<itemRunId>` at the node the item names. Otherwise `undefined`. Two keyed reads, never a listing. It rejects when the store cannot be read, as `hasSuspendedRun` does. No other engine hunk; `run-resume.ts` and the store are untouched. - **`packages/plugins/plugin-approvals/src/approval-service.ts`.** `ApprovalResumeSurface` declares the member (optional). In `recall`'s revise-window branch the new private `batchAboveItem(runId)` reads it BEFORE `cancelRun`, because the cancel removes the item's suspension and the link with it. It never throws; a failed read is carried to `resumeBatchOfRecalledItem`, which now takes the parent it read instead of scanning `listSuspendedRunsDurable()`, and tells a failed lookup in `resumeError` after the cancel, as before. The built-in `map` check through `getFlow` and everything after it are unchanged. - **Tests.** `suspended-map-parent-of.test.ts` (new, service-automation) pins the engine member; `batch-item-recall-resumes-batch.test.ts` (plugin-approvals) gains a `#28230` block over two replicas and one `ObjectStoreSuspendedRunStore`. Its existing wiring moved into a `wireReplica` helper; #28228's five cells are unchanged and green. No new engine double: the block reuses the file's already-pinned one, so `scripts/engine-double-contract.pinned.json` is untouched. - **`.changeset/28230-batch-recall-past-first-page.md`.** `@objectstack/plugin-approvals` and `@objectstack/service-automation`, patch. ## Why ### Before (measured at `origin/main` `f40b777d`) Replicas `a` and `b` share one `ObjectStoreSuspendedRunStore` and one table set. N `paused` rows of another flow are stored first. The batch (`map` over two `task_signoff` items) is launched on `a`, item 1 is sent back on `a`, and the submitter recalls item 1 on `b`. | | N = 1000, before | N = 998, before | N = 1000, after | |:---|:---|:---|:---| | batch on `b.listSuspendedRunsDurable()` | no | yes | no (no longer read) | | item 1's run | `cancelled` | `cancelled` | `cancelled` | | item 2's request | none: the batch stays parked on item 1 | `pending` | `pending` | | recall answer | `resumed: false`, no `resumeError` | `resumed: false`, no `resumeError` | `resumed: false`, no `resumeError` | | after item 2 is approved | (never reached) | marker in slot 1, `count` 1 | marker in slot 1, `count` 1, batch `completed` | The listing reads one 1000-row page of `paused` rows in driver order, and a process adds only what it parked itself. So the batch is missed exactly when another process (a replica, or this one before a restart) parked it behind more than 1000 runs. ### Why this path Triage 6531219559 / 6532374421 ruled 「a correlation-keyed lookup instead of a full listing」. The item run already carries its batch: `map-node.ts` launches every item with `$parentRunId` and `$parentMapNode`. Reading that before the cancel needs no store change, no schema change and no new code. The parked-on-this-item check guards one real shape: a `subflow` inside a `map` item inherits the item's `$parentMapNode` through the spread context, so its `$parentRunId` names the item run, which is parked at a `subflow` node, not the map (pinned). ## Tests `suspended-map-parent-of.test.ts` (5 cells): a `map` item answers its batch, also from a second engine over the same store with nothing in memory; CONTROL a `subflow` child, a `subflow` nested in a `map` item, an item whose batch has moved on, a top-level run and an unknown id answer `undefined`. `batch-item-recall-resumes-batch.test.ts`, `#28230` block (4 cells): 1. the batch past the first page resumes, item 1's slot holds #28176's `cancelled` marker, `count` 1, the batch completes (red on `main`: no request opens for item 2); 2. CONTROL: a batch within the first page behaves as before; 3. CONTROL: a recall with no batch above it, past the first page, behaves as before, and the batch beside it stays parked on its item; 4. CONTROL: a parent the engine answers (a home-grown fan-out that writes the same linkage) but parked at a node other than the built-in `map` is left alone. Ablations through `scripts/ablation-replace.mjs`, each restored to the HEAD blob with `git diff HEAD` empty: | Ablation | Red | |:---|:---| | A1: `batchAboveItem` answers no parent | #28183 cell 1, #28230 cells 1 and 2 | | A2: the batch is read after `cancelRun` | #28183 cell 1, #28230 cells 1 and 2 | | A3: drop the built-in `map` node check | #28230 cell 4 | | A4: engine member drops the parked-on-this-item check | engine cells 2, 3, 4 | (A5, dropping the `typeof parentRunId` check, stayed green: a missing id loads no suspension, so it answers `undefined` either way. It is the type narrowing the next read needs, not a guard.) ## Verification Head `776907f9`, after a whole-tree `turbo run build`, `pnpm install --frozen-lockfile` and fetching the teardown self-test's pinned commit. - `pnpm --filter @objectstack/service-automation run test`: 234 files, 2952 passed. `pnpm --filter @objectstack/plugin-approvals run test`: 106 files, 1361 passed. Both packages' `typecheck`: exit 0. - `dispatch-gates --commands`: 64 families, all exit 0 (`check:dual-build-cjs-loads` and `check:i18n` exited 3 before the whole-tree build and 0 after it). `--ran`: 64 derived, 64 run, 0 NOT-MEASURED, 0 UNRUN. `check:engine-double-contract` and `check:objectql-double-limit` are green with no ledger edit. - `pnpm lint`: exit 0. `check-commit-card-trailers --range origin/main..HEAD`: exit 0. ## Out of scope - None filed. Probed while pinning: a `subflow` inside a `map` item inherits the item's `$parentMapNode`, but on `main` the nested chain still completes into the item run and the batch moves on, so it is not a defect. The engine member's parked-on-this-item check keeps it from answering that shape. - `listSuspendedRunsDurable` still reads one 1000-row page for its other caller, the restored-continuation proof in `approval-service.ts`, which refuses on an absent entry (a retry, not a silent strand). Not this card's. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016WV4d1FQJt7HxgWZZ5cYz9 Co-authored-by: Claude <noreply@anthropic.com>
1 parent ef777b8 commit 898e439

5 files changed

Lines changed: 452 additions & 31 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
'@objectstack/service-automation': patch
4+
---
5+
6+
fix(plugin-approvals): a revise-window recall on a batch item finds its batch run past the first 1000 parked runs
7+
8+
Clause-②: no (a recall on a store with more than 1000 parked runs now wakes its batch, as it already does below that; no schema, accept set, error code or result field changes)
9+
Metadata-protocol: none
10+
11+
The recall found the batch above the item it cancels in `listSuspendedRunsDurable()`. On a database-backed store that listing reads one page of 1000 `paused` rows, and a process adds only the runs it parked itself. So when another replica (or this process before a restart) parked the batch and more than 1000 runs were parked ahead of it, the recall did not find the batch. The batch stayed parked on the cancelled item, and the recall said nothing.
12+
13+
- **Now.** Before it cancels the item, the recall asks the engine for the batch through the item's own link to it. The new `AutomationEngine.suspendedMapParentOf(itemRunId)` reads the item's suspension, then its parent's, by id. It answers the parent only while the parent is parked on `map:<itemRunId>` at the `map` node that launched the item. The read comes before `cancelRun` because the cancel removes the item's suspension, and the link with it.
14+
- **Unchanged.** Below 1000 parked runs the recall wakes the batch as before. A recall with no batch above it, and a run parked on the item at a node other than the built-in `map`, are left alone as before. A failed lookup is reported in `resumeError` after the cancel, as before.

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,20 @@ export interface ApprovalResumeSurface {
288288
* pause it cannot see is a pause this verb will not act on.
289289
*/
290290
listSuspendedRunsDurable?(): Promise<Array<{ runId: string; flowName: string; nodeId: string; correlation?: string }>>;
291+
/**
292+
* [#28230] The run parked at a `map` node on the item run `itemRunId`, or
293+
* `undefined` when there is none. Read by
294+
* {@link ApprovalService.recall}'s revise-window branch BEFORE it cancels
295+
* the item, so it can wake the batch above it afterwards: the engine
296+
* answers from the item's own `$parentRunId` linkage with keyed reads,
297+
* where {@link listSuspendedRunsDurable} reads one capped page of a
298+
* DB-backed store and can miss the batch.
299+
*
300+
* ⚠️ Declares the method `AutomationEngine` implements publicly
301+
* (`suspendedMapParentOf`). Read-only; rejects when the store cannot be
302+
* read. Optional: without it no batch is woken, as before #28183.
303+
*/
304+
suspendedMapParentOf?(itemRunId: string): Promise<BatchParent | undefined>;
291305
/**
292306
* [#15358] Whether the consumed suspension behind a `failed` run SURVIVES —
293307
* i.e. whether the engine's `restoreConsumedSuspension` would re-arm it.
@@ -352,6 +366,12 @@ export interface ApprovalResumeSurface {
352366
takeSubflowParentStrand?(childRunId: string): { runId: string; repairable: boolean; error: string } | undefined;
353367
}
354368

369+
/** A run parked at a `map` node on one item, as {@link ApprovalResumeSurface.suspendedMapParentOf} answers it. */
370+
type BatchParent = { runId: string; flowName: string; nodeId: string; correlation?: string };
371+
372+
/** What the revise-window recall read about the batch above its item before cancelling it. */
373+
type BatchLookup = { parent?: BatchParent } | { lookupFailure: unknown };
374+
355375
/** What {@link ApprovalResumeSurface.inspectConsumedSuspension} answers. */
356376
type ConsumedSuspensionVerdict =
357377
Awaited<ReturnType<NonNullable<ApprovalResumeSurface['inspectConsumedSuspension']>>>;
@@ -5277,6 +5297,9 @@ export class ApprovalService implements IApprovalService {
52775297
if (runId) {
52785298
resumeError = this.missingRunCapability(runId, requestId, 'the recall', 'cancelRun');
52795299
if (!resumeError) {
5300+
// [#28230] Read the batch above the item while the item's
5301+
// suspension still carries the linkage: `cancelRun` removes it.
5302+
const batch = await this.batchAboveItem(runId);
52805303
let cancelled: unknown;
52815304
try {
52825305
cancelled = await this.automation!.cancelRun!(runId, `approval request ${requestId} recalled during revision`);
@@ -5288,7 +5311,7 @@ export class ApprovalService implements IApprovalService {
52885311
}
52895312
// [#28183] `cancelRun` never touches a parent, so a batch whose
52905313
// `map` item this was would stay parked with nothing to wake it.
5291-
if (cancelled === true) ({ resumeError, resumeFailure } = await this.resumeBatchOfRecalledItem(runId, requestId));
5314+
if (cancelled === true) ({ resumeError, resumeFailure } = await this.resumeBatchOfRecalledItem(runId, requestId, batch));
52925315
}
52935316
}
52945317
} else if (runId) {
@@ -5370,6 +5393,29 @@ export class ApprovalService implements IApprovalService {
53705393
};
53715394
}
53725395

5396+
/**
5397+
* [#28230] The batch run parked on the revise-window item `itemRunId`, read
5398+
* before the recall cancels the item — {@link ApprovalResumeSurface.suspendedMapParentOf}
5399+
* reads the item's own `$parentRunId`, which `cancelRun` removes with the
5400+
* item's suspension. Never throws: a failed read is carried to
5401+
* {@link resumeBatchOfRecalledItem}, which tells it once the cancel is done,
5402+
* as it always told a failed lookup.
5403+
*
5404+
* It used to be the entry of `listSuspendedRunsDurable()` parked on
5405+
* `map:<itemRunId>`, read after the cancel. That listing reads one capped
5406+
* page of a DB-backed store, and a process adds only what it parked itself,
5407+
* so a batch another process parked behind more than 1000 runs was not
5408+
* found and stayed parked with nothing said.
5409+
*/
5410+
private async batchAboveItem(itemRunId: string): Promise<BatchLookup> {
5411+
if (typeof this.automation?.suspendedMapParentOf !== 'function') return {};
5412+
try {
5413+
return { parent: await this.automation.suspendedMapParentOf(itemRunId) };
5414+
} catch (err) {
5415+
return { lookupFailure: err };
5416+
}
5417+
}
5418+
53735419
/**
53745420
* [#28183] Wake the batch above a run a revise-window recall just cancelled.
53755421
*
@@ -5386,21 +5432,22 @@ export class ApprovalService implements IApprovalService {
53865432
* Only a run parked at the built-in `map` node on that item is woken: the
53875433
* engine records the marker for no other node, and re-running any other
53885434
* node would advance it past the item with nothing recorded. Whatever is
5389-
* missing to prove that — no `listSuspendedRunsDurable`, no `getFlow`, no
5435+
* missing to prove that — no `suspendedMapParentOf`, no `getFlow`, no
53905436
* such parent — leaves things exactly as they were before, which for a
5391-
* recall with no batch above it is the whole answer.
5437+
* recall with no batch above it is the whole answer. The parent is the one
5438+
* {@link batchAboveItem} read before the cancel.
53925439
*/
53935440
private async resumeBatchOfRecalledItem(
53945441
itemRunId: string,
53955442
requestId: string,
5443+
batch: BatchLookup,
53965444
): Promise<{ resumeError?: string; resumeFailure?: ResumeFailureReport }> {
5397-
if (typeof this.automation?.listSuspendedRunsDurable !== 'function') return {};
5398-
if (typeof this.automation.getFlow !== 'function') return {};
53995445
let batchRunId: string | undefined;
54005446
try {
5401-
const parent = (await this.automation.listSuspendedRunsDurable())
5402-
.find(r => r.correlation === `map:${itemRunId}`);
5447+
if ('lookupFailure' in batch) throw batch.lookupFailure;
5448+
const parent = batch.parent;
54035449
if (!parent) return {};
5450+
if (typeof this.automation?.getFlow !== 'function') return {};
54045451
const flow: any = await this.automation.getFlow(parent.flowName);
54055452
const node = Array.isArray(flow?.nodes) ? flow.nodes.find((n: any) => n?.id === parent.nodeId) : undefined;
54065453
if (node?.type !== 'map') return {};

0 commit comments

Comments
 (0)