Skip to content

Commit c616c2c

Browse files
claude[bot]os-salesclaude
authored
fix(service-automation): route the last three suspended-run readers through the shared loader (#14650)
* fix(service-automation): route the three remaining suspended-run readers through the shared loader `cancelRun`, `failAncestors` and `listSuspendedRunsDurable` still preferred the per-process `suspendedRuns` map over the shared `SuspendedRunStore`, which #13617 had already made authoritative for the resume path. Each now takes one answer to "where is this run parked", with the degrading or strict loader chosen per site so the recorded #4632 / #6299 degradation posture is preserved by choice rather than re-derived. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * test(service-automation): import AutomationContext from its declaring module `AutomationContext` is declared in `@objectstack/spec/contracts` and re-imported by `engine.ts` as a type — it is not re-exported from there, so importing it from `./engine.js` added a fourth error (TS2459) to a package whose type-check debt ledger is frozen at 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --------- Co-authored-by: os-sales <sales@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 655b106 commit c616c2c

3 files changed

Lines changed: 552 additions & 53 deletions

File tree

‎.changeset/olive-parrots-attend.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@objectstack/service-automation': patch
3+
---
4+
5+
**The last three readers of suspended-run state read the shared store, not this replica's memory of it.** #13617 made the resume path store-authoritative; `cancelRun`, `failAncestors` and `listSuspendedRunsDurable` still preferred the per-process `suspendedRuns` map, so on a replica holding a stale entry each acted on the node a run was parked at the last time THIS replica touched it. All three now take one answer to "where is this run parked", through the existing `loadSuspendedRun` / `loadSuspendedRunStrict` pair, with the degrading or strict loader chosen per site so each recorded degradation posture is preserved by choice rather than re-derived.
6+
7+
- **`cancelRun` — the strict loader.** Before: a stale replica cancelled from its own snapshot; the row deletion is by id and was right either way, but `forgetSuspendedRun` told the executor of the node in the SNAPSHOT that its pause was over, so the live node's pause stayed armed and a node the run had already left was released a second time. Now the shared row decides which pause is torn down. The strict loader is deliberate: "not found" still returns `false` (already terminal / unknown) exactly as before, and an unreadable store still lands on this seam's own #4632 DURABILITY record at `error` — the degrading loader would have answered `null` under its best-effort `warn` and silently downgraded that verdict. ⚠️ One consequence, stated: while a store is configured this process's map is no longer an answer, so a store outage now reaches that `error` record even for a run this replica is holding, where the old cache-first read cancelled from the local snapshot.
8+
- **`failAncestors` — the degrading loader.** Before: a stale parent was failed at a node it had already left (#13617's own harm shape, one level up). The degrading loader is deliberate: this walk runs inside the catch arm already handling a run's failure, so it must not throw, and "a store failure reads as no ancestor here and stops the walk" is exactly the posture the bare `.catch(() => null)` had. The one thing gained beyond the fix: that silent swallow is now recorded, at the loader's declared best-effort `warn` — no new `error` seam.
9+
- **`listSuspendedRunsDurable` — the merge direction, and the comment.** The durable row now wins an id collision; the comment claiming "In-memory entries win — they are the freshest copy" is corrected, since it is true of exactly one deployment shape. Map entries the durable listing does not carry are still included, deliberately: `store.list()` is a capped, best-effort enumeration (at most 1000 `paused` rows) and the same merge is reached on the degraded path, so "absent from the list" is not the per-id "the store answered and has no row" the strict loader rests on.
10+
11+
No signature, export or return-shape change on any of the three.

‎packages/services/service-automation/src/engine.ts‎

Lines changed: 107 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -5330,54 +5330,73 @@ export class AutomationEngine implements IAutomationService {
53305330
* indistinguishable to the caller, so the run may still be parked and
53315331
* resumable. That path is reported at `error` (#4632/#6299) precisely
53325332
* because nothing above it can tell the difference; see the catch below.
5333+
*
5334+
* [#14332] WHERE THE RUN IS READ FROM: {@link loadSuspendedRunStrict} —
5335+
* the same store-authoritative read `resumeInternal` takes, and the STRICT
5336+
* loader by deliberate choice rather than the degrading
5337+
* {@link loadSuspendedRun}. NOT FOUND (the store answered and holds no row
5338+
* for this id) reads as "already terminal / unknown" and returns `false`,
5339+
* exactly as before; a store that cannot be READ throws out of the loader
5340+
* into the catch below, which keeps this site's own #4632 DURABILITY record
5341+
* at `error`. The degrading loader would have answered `null` under its own
5342+
* best-effort `warn` and silently downgraded that verdict — the posture is
5343+
* preserved here by picking the loader that preserves it.
5344+
*
5345+
* ⚠️ The consequence of a store-authoritative read, stated rather than left
5346+
* to be discovered: while a store is configured this process's map is no
5347+
* longer an answer, so a store outage reaches the `error` record above even
5348+
* for a run THIS replica is holding — where the old cache-first read
5349+
* cancelled from the local snapshot instead. That snapshot is the defect:
5350+
* the row delete is by id and is therefore right either way, but
5351+
* {@link forgetSuspendedRun} notifies the executor of the node recorded on
5352+
* the SNAPSHOT, so a stale replica tore down the pause of a node the run had
5353+
* already left and left the live one's armed.
53335354
*/
53345355
async cancelRun(runId: string, reason?: string): Promise<boolean> {
5335-
let run = this.suspendedRuns.get(runId) ?? null;
5336-
if (!run && this.store) {
5337-
try {
5338-
run = await this.store.load(runId);
5339-
} catch (err) {
5340-
// #6299 — same family, same mechanism as `forgetSuspendedRun`
5341-
// above: the driver's uncontrolled text goes to the structured
5342-
// slot so the record stays one physical line.
5343-
//
5344-
// #4632 verdict: DURABILITY — raised from `warn` to `error`. The
5345-
// failed read is silently turned into "no such suspended run"
5346-
// and this method returns `false`, which its own contract
5347-
// documents as idempotent success (already terminal / unknown),
5348-
// so the cancellation is SKIPPED while the call reads clean. The
5349-
// only in-repo caller measures the cost: plugin-approvals'
5350-
// revise-window recall
5351-
// (`packages/plugins/plugin-approvals/src/approval-service.ts`)
5352-
// never reads the boolean at all — it only catches a THROW, and
5353-
// grades that throw `error` with "the run may be stranded"
5354-
// (#4420). A store-read failure produces precisely that stranded
5355-
// run WITHOUT firing that alarm: the request is marked
5356-
// `recalled`, the record lock is released, `resumeError` stays
5357-
// undefined — and the run stays parked in the store, to be
5358-
// re-armed and resumed by the next restart, inside a flow whose
5359-
// approval has already been withdrawn.
5360-
//
5361-
// This is why #6230's verdict must not be copied here.
5362-
// `loadSuspendedRun` is a DECLARED best-effort reader for
5363-
// incidental callers (a gate lookup, a screen fetch), and
5364-
// `resumeInternal` takes the strict form exactly where the
5365-
// difference matters. `cancelRun` has no strict alternative, and
5366-
// its degradation decides a WRITE.
5367-
//
5368-
// THIRD argument (`error(message, error?, meta?)`), `Error` slot
5369-
// deliberately empty (#5575).
5370-
this.logger.error(
5371-
`[automation] cancelRun('${runId}') could not read the durable suspended-run store, so the ` +
5372-
`cancellation was SKIPPED and reported as idempotent success — this call returns false, which ` +
5373-
`its callers read as "no such suspended run". The run is NOT cancelled: if it is parked in the ` +
5374-
`store it stays parked, and the next restart re-arms and resumes it while the caller has ` +
5375-
`already recorded the cancellation. Fix the store failure in this record's meta, then re-issue ` +
5376-
`cancelRun('${runId}').`,
5377-
undefined,
5378-
describeThrownForLog(err),
5379-
);
5380-
}
5356+
let run: SuspendedRun | null = null;
5357+
try {
5358+
run = await this.loadSuspendedRunStrict(runId);
5359+
} catch (err) {
5360+
// #6299 — same family, same mechanism as `forgetSuspendedRun`
5361+
// above: the driver's uncontrolled text goes to the structured
5362+
// slot so the record stays one physical line.
5363+
//
5364+
// #4632 verdict: DURABILITY — raised from `warn` to `error`. The
5365+
// failed read is silently turned into "no such suspended run"
5366+
// and this method returns `false`, which its own contract
5367+
// documents as idempotent success (already terminal / unknown),
5368+
// so the cancellation is SKIPPED while the call reads clean. The
5369+
// only in-repo caller measures the cost: plugin-approvals'
5370+
// revise-window recall
5371+
// (`packages/plugins/plugin-approvals/src/approval-service.ts`)
5372+
// never reads the boolean at all — it only catches a THROW, and
5373+
// grades that throw `error` with "the run may be stranded"
5374+
// (#4420). A store-read failure produces precisely that stranded
5375+
// run WITHOUT firing that alarm: the request is marked
5376+
// `recalled`, the record lock is released, `resumeError` stays
5377+
// undefined — and the run stays parked in the store, to be
5378+
// re-armed and resumed by the next restart, inside a flow whose
5379+
// approval has already been withdrawn.
5380+
//
5381+
// This is why #6230's verdict must not be copied here.
5382+
// `loadSuspendedRun` is a DECLARED best-effort reader for
5383+
// incidental callers (a gate lookup, a screen fetch), and
5384+
// `resumeInternal` takes the strict form exactly where the
5385+
// difference matters. `cancelRun` has no strict alternative, and
5386+
// its degradation decides a WRITE.
5387+
//
5388+
// THIRD argument (`error(message, error?, meta?)`), `Error` slot
5389+
// deliberately empty (#5575).
5390+
this.logger.error(
5391+
`[automation] cancelRun('${runId}') could not read the durable suspended-run store, so the ` +
5392+
`cancellation was SKIPPED and reported as idempotent success — this call returns false, which ` +
5393+
`its callers read as "no such suspended run". The run is NOT cancelled: if it is parked in the ` +
5394+
`store it stays parked, and the next restart re-arms and resumes it while the caller has ` +
5395+
`already recorded the cancellation. Fix the store failure in this record's meta, then re-issue ` +
5396+
`cancelRun('${runId}').`,
5397+
undefined,
5398+
describeThrownForLog(err),
5399+
);
53815400
}
53825401
if (!run) return false;
53835402
await this.forgetSuspendedRun(run, 'cancelled');
@@ -5747,9 +5766,19 @@ export class AutomationEngine implements IAutomationService {
57475766
let parentId = (context as Record<string, unknown> | undefined)?.$parentRunId;
57485767
let hops = 0;
57495768
while (typeof parentId === 'string' && parentId && hops++ < 32) {
5750-
const parent =
5751-
this.suspendedRuns.get(parentId) ??
5752-
(this.store ? await this.store.load(parentId).catch(() => null) : null);
5769+
// [#14332] The DEGRADING loader, by deliberate choice: this walk runs
5770+
// inside the catch arm that is already handling a run's failure, so it
5771+
// must not throw, and its recorded posture is exactly
5772+
// `loadSuspendedRun`'s — a store failure reads as "no ancestor here"
5773+
// and stops the walk. What changes is only WHICH suspension is read:
5774+
// the shared store's, not this replica's memory of where the parent
5775+
// was last parked. The old `??` chain had #13617's own harm shape —
5776+
// a stale parent failed at a node it had already left, so
5777+
// `forgetSuspendedRun` released the wrong node's pause. The one thing
5778+
// gained beyond that: the bare `.catch(() => null)` swallowed a store
5779+
// failure in total silence, and the loader records it (at `warn`,
5780+
// its declared best-effort level — no new `error` seam here).
5781+
const parent = await this.loadSuspendedRun(parentId);
57535782
if (!parent) return;
57545783
await this.failSuspendedRun(parent, `subflow descendant failed: ${error}`);
57555784
parentId = (parent.context as Record<string, unknown> | undefined)?.$parentRunId;
@@ -5776,9 +5805,14 @@ export class AutomationEngine implements IAutomationService {
57765805

57775806
/**
57785807
* Like {@link listSuspendedRuns} but includes runs held only in the durable
5779-
* {@link SuspendedRunStore} (e.g. suspended before a restart). The in-memory
5780-
* cache takes precedence on id collisions. Falls back to the in-memory list
5781-
* when no store is configured.
5808+
* {@link SuspendedRunStore} (e.g. suspended before a restart). Falls back to
5809+
* the in-memory list when no store is configured.
5810+
*
5811+
* [#14332] The DURABLE row wins an id collision — the store is the shared
5812+
* answer to "where is this run parked" and this process's map is only its
5813+
* own memory of it. A run present in the map but absent from the durable
5814+
* listing is still included, because a capped or failed enumeration is not
5815+
* the per-id "no row" the strict loader rests on; see the merge below.
57825816
*/
57835817
async listSuspendedRunsDurable(): Promise<Array<{ runId: string; flowName: string; nodeId: string; correlation?: string }>> {
57845818
const byId = new Map<string, { runId: string; flowName: string; nodeId: string; correlation?: string }>();
@@ -5831,8 +5865,28 @@ export class AutomationEngine implements IAutomationService {
58315865
);
58325866
}
58335867
}
5834-
// In-memory entries win — they are the freshest copy.
5868+
// [#14332] The DURABLE row wins a collision. The comment this replaces
5869+
// said the opposite — "In-memory entries win — they are the freshest
5870+
// copy" — which is true of exactly one deployment shape, a single
5871+
// process. Put several replicas over one store and this map is a
5872+
// per-replica snapshot of the node a run was parked at THE LAST TIME
5873+
// THIS REPLICA TOUCHED IT, with no invalidation channel to it at all
5874+
// (the mechanism is in {@link loadSuspendedRunStrict}), so preferring it
5875+
// reported a run at a node it had already left.
5876+
//
5877+
// Map entries the durable list does not carry are still appended, and
5878+
// that is NOT the strict loader's rule being softened: a LIST is not a
5879+
// per-id answer. `store.list()` is a capped, best-effort enumeration
5880+
// (`ObjectStoreSuspendedRunStore` reads at most 1000 `paused` rows) and
5881+
// this line is also reached on the DEGRADED path above, where the
5882+
// enumeration failed outright and `byId` is empty. So "absent from the
5883+
// list" is not the evidence "the store answered and has no row" is,
5884+
// which is what {@link loadSuspendedRunStrict} rests on when it lets
5885+
// only {@link cacheOnlySuspensions} answer out of the map. Applying that
5886+
// qualifier here would let a truncated or failed enumeration silently
5887+
// drop live runs from an operability listing.
58355888
for (const r of this.suspendedRuns.values()) {
5889+
if (byId.has(r.runId)) continue;
58365890
byId.set(r.runId, { runId: r.runId, flowName: r.flowName, nodeId: r.nodeId, correlation: r.correlation });
58375891
}
58385892
return [...byId.values()];

0 commit comments

Comments
 (0)