Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/28230-batch-recall-past-first-page.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
'@objectstack/plugin-approvals': patch
'@objectstack/service-automation': patch
---

fix(plugin-approvals): a revise-window recall on a batch item finds its batch run past the first 1000 parked runs

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

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.

- **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.
- **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.
61 changes: 54 additions & 7 deletions packages/plugins/plugin-approvals/src/approval-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,20 @@ export interface ApprovalResumeSurface {
* pause it cannot see is a pause this verb will not act on.
*/
listSuspendedRunsDurable?(): Promise<Array<{ runId: string; flowName: string; nodeId: string; correlation?: string }>>;
/**
* [#28230] The run parked at a `map` node on the item run `itemRunId`, or
* `undefined` when there is none. Read by
* {@link ApprovalService.recall}'s revise-window branch BEFORE it cancels
* the item, so it can wake the batch above it afterwards: the engine
* answers from the item's own `$parentRunId` linkage with keyed reads,
* where {@link listSuspendedRunsDurable} reads one capped page of a
* DB-backed store and can miss the batch.
*
* ⚠️ Declares the method `AutomationEngine` implements publicly
* (`suspendedMapParentOf`). Read-only; rejects when the store cannot be
* read. Optional: without it no batch is woken, as before #28183.
*/
suspendedMapParentOf?(itemRunId: string): Promise<BatchParent | undefined>;
/**
* [#15358] Whether the consumed suspension behind a `failed` run SURVIVES —
* i.e. whether the engine's `restoreConsumedSuspension` would re-arm it.
Expand Down Expand Up @@ -352,6 +366,12 @@ export interface ApprovalResumeSurface {
takeSubflowParentStrand?(childRunId: string): { runId: string; repairable: boolean; error: string } | undefined;
}

/** A run parked at a `map` node on one item, as {@link ApprovalResumeSurface.suspendedMapParentOf} answers it. */
type BatchParent = { runId: string; flowName: string; nodeId: string; correlation?: string };

/** What the revise-window recall read about the batch above its item before cancelling it. */
type BatchLookup = { parent?: BatchParent } | { lookupFailure: unknown };

/** What {@link ApprovalResumeSurface.inspectConsumedSuspension} answers. */
type ConsumedSuspensionVerdict =
Awaited<ReturnType<NonNullable<ApprovalResumeSurface['inspectConsumedSuspension']>>>;
Expand Down Expand Up @@ -5277,6 +5297,9 @@ export class ApprovalService implements IApprovalService {
if (runId) {
resumeError = this.missingRunCapability(runId, requestId, 'the recall', 'cancelRun');
if (!resumeError) {
// [#28230] Read the batch above the item while the item's
// suspension still carries the linkage: `cancelRun` removes it.
const batch = await this.batchAboveItem(runId);
let cancelled: unknown;
try {
cancelled = await this.automation!.cancelRun!(runId, `approval request ${requestId} recalled during revision`);
Expand All @@ -5288,7 +5311,7 @@ export class ApprovalService implements IApprovalService {
}
// [#28183] `cancelRun` never touches a parent, so a batch whose
// `map` item this was would stay parked with nothing to wake it.
if (cancelled === true) ({ resumeError, resumeFailure } = await this.resumeBatchOfRecalledItem(runId, requestId));
if (cancelled === true) ({ resumeError, resumeFailure } = await this.resumeBatchOfRecalledItem(runId, requestId, batch));
}
}
} else if (runId) {
Expand Down Expand Up @@ -5370,6 +5393,29 @@ export class ApprovalService implements IApprovalService {
};
}

/**
* [#28230] The batch run parked on the revise-window item `itemRunId`, read
* before the recall cancels the item — {@link ApprovalResumeSurface.suspendedMapParentOf}
* reads the item's own `$parentRunId`, which `cancelRun` removes with the
* item's suspension. Never throws: a failed read is carried to
* {@link resumeBatchOfRecalledItem}, which tells it once the cancel is done,
* as it always told a failed lookup.
*
* It used to be the entry of `listSuspendedRunsDurable()` parked on
* `map:<itemRunId>`, read after the cancel. That listing reads one capped
* page of a DB-backed store, and a process adds only what it parked itself,
* so a batch another process parked behind more than 1000 runs was not
* found and stayed parked with nothing said.
*/
private async batchAboveItem(itemRunId: string): Promise<BatchLookup> {
if (typeof this.automation?.suspendedMapParentOf !== 'function') return {};
try {
return { parent: await this.automation.suspendedMapParentOf(itemRunId) };
} catch (err) {
return { lookupFailure: err };
}
}

/**
* [#28183] Wake the batch above a run a revise-window recall just cancelled.
*
Expand All @@ -5386,21 +5432,22 @@ export class ApprovalService implements IApprovalService {
* Only a run parked at the built-in `map` node on that item is woken: the
* engine records the marker for no other node, and re-running any other
* node would advance it past the item with nothing recorded. Whatever is
* missing to prove that — no `listSuspendedRunsDurable`, no `getFlow`, no
* missing to prove that — no `suspendedMapParentOf`, no `getFlow`, no
* such parent — leaves things exactly as they were before, which for a
* recall with no batch above it is the whole answer.
* recall with no batch above it is the whole answer. The parent is the one
* {@link batchAboveItem} read before the cancel.
*/
private async resumeBatchOfRecalledItem(
itemRunId: string,
requestId: string,
batch: BatchLookup,
): Promise<{ resumeError?: string; resumeFailure?: ResumeFailureReport }> {
if (typeof this.automation?.listSuspendedRunsDurable !== 'function') return {};
if (typeof this.automation.getFlow !== 'function') return {};
let batchRunId: string | undefined;
try {
const parent = (await this.automation.listSuspendedRunsDurable())
.find(r => r.correlation === `map:${itemRunId}`);
if ('lookupFailure' in batch) throw batch.lookupFailure;
const parent = batch.parent;
if (!parent) return {};
if (typeof this.automation?.getFlow !== 'function') return {};
const flow: any = await this.automation.getFlow(parent.flowName);
const node = Array.isArray(flow?.nodes) ? flow.nodes.find((n: any) => n?.id === parent.nodeId) : undefined;
if (node?.type !== 'map') return {};
Expand Down
Loading
Loading