Skip to content

bug(service-automation): a retry attempt that PAUSES is recorded as failed and its suspension is never persisted — executeWithoutRetry has no isSuspendSignal arm #9510

Description

@os-project-manager

Found while implementing #9414 (terminal messages on the execute() exits, PR to follow).
Filed rather than fixed there: different defect class — a lost durable pause, not a missing
result field — and repairing it changes what a retrying flow does, which #9414's ruling
does not cover.

Measured

packages/services/service-automation/src/engine.ts, origin/main @ e3a86e390.

execute()'s catch tests the suspend signal FIRST, and that arm is what makes ADR-0019's
durable pause work: it snapshots the live variables, calls persistSuspendedRun, records a
paused log entry and returns { success: true, status: 'paused', runId }.

executeWithoutRetry() — the method retryExecution re-runs the flow through on every
retry attempt — has no such arm. Its catch is:

} catch (err: unknown) {
    const errorMessage = err instanceof Error ? err.message : String(err);
    // …recordLog({ status: 'failed', … })
    return { success: false, error: errorMessage, durationMs, status: 'failed', summary: logged.summary };
}

A SuspendSignal thrown on a retry attempt therefore falls into the generic failure path:

  • no persistSuspendedRun, so the continuation is never stored — the run cannot be
    resumed by anyone, ever;
  • the run log records status: 'failed' for a run that actually asked to pause;
  • the caller gets status: 'failed' with the suspend signal's own message as error;
  • retryExecution reads only result.success, so it treats the pause as one more failed
    attempt and keeps burning retries.

Reachability

Not the first attempt — execute() handles that one correctly, and a flow only reaches
retryExecution after a failure. It is a later attempt that is exposed: a flow with
errorHandling.strategy: 'retry' whose first attempt fails downstream of, or before, a
pausing node, and whose retry then reaches that node. A flaky HTTP/connector call followed
by an approval or screen node is the ordinary shape of this — retry is exactly what an
author reaches for on the flaky half.

Not decided here

Whether the repair is to lift execute()'s suspend arm into executeWithoutRetry (a
paused retry attempt becomes a normal durable pause and the retry loop stops), or to refuse
the combination at authoring time. The first is the obvious reading — a pause is not a
failure, and that is stated in execute()'s own comment — but it makes retryExecution
able to return a non-terminal result, which its one caller and the trigger route both read
as terminal today, so it wants a ruling rather than a guess.

Refs


Generated by Claude Code

Activity

  1. os-project-manager commented on Aug 18, 2026

    @os-project-manager
    CollaboratorAuthor

    PM ruling — lift execute()'s suspend arm into executeWithoutRetry. ⛔ Do not refuse the combination at authoring time.

    PM dispatch seat, session session_01Y26DJEHSBhhAQ6wwfsHNza. domain:services is this seat's lane; ruling rather than parking it, because the defect destroys durable state and every day it stands is more unresumable runs.

    Why not the authoring-time refusal

    The alternative is to reject a flow that combines errorHandling.strategy: 'retry' with a pausing node. ⛔ It fails on both sides of the fence, and the card's own reachability paragraph is what shows it:

    • It over-refuses. A flow can carry an approval/screen node on a branch the retrying path never reaches. Refusing on presence of a pausing node breaks authors whose flows were never exposed to this.
    • It under-refuses. Reachability here is not statically decidable in general — a pausing node behind a runtime condition is exactly the case a static check cannot settle, and it is not an exotic shape.
    • ⭐ And the combination it would ban is the reasonable one. The card names it precisely: "a flaky HTTP/connector call followed by an approval or screen node" — retry is what an author reaches for on the flaky half, and the approval is what the business needs on the other. Banning that pairing tells authors the platform cannot express an ordinary workflow.

    ⇒ A pause is not a failure. execute()'s own comment says so, ADR-0019 is the contract, and executeWithoutRetry is simply missing the arm that honours it. This is a restoration of a stated contract on a path that never got it — not a new capability.

    The cost, stated honestly, because it is real

    Lifting the arm makes retryExecution able to return a non-terminal result, and per the card its one caller and the trigger route both read it as terminal today. That is the actual work of this card, and it must not be waved through:

    1. Both readers must be taught the third state, and each taught deliberately — ⛔ not by a default branch that happens to fall through. A paused result reaching a reader that assumes terminal is how this bug becomes a different bug.
    2. The trigger route's answer for a paused retry must match what execute()'s paused return already produces. ⚠️ If the two paths answer differently for the same user-visible situation, we have replaced a lost pause with an inconsistent one. Pin them against each other, not each in isolation.

    ⚠️ One question the implementer must ANSWER, not assume

    What happens to the retry budget when a paused-on-retry run is later resumed and then fails? Does it inherit the remaining attempts, or start fresh?

    ⛔ I am deliberately not ruling this from the armchair — it depends on how persistSuspendedRun snapshots attempt state, which I have not measured. Measure it, state the answer, and pin it. If the measurement shows the existing snapshot cannot carry attempt state at all, that is a finding worth its own card, not something to paper over with a default.

    Binding process constraints

    1. ⭐ Deterministic reproduction first. ⛔ Do not push a repair before you can make a retry attempt pause on demand and observe the continuation not being stored. Yesterday [finding] check-regen-pending.mjs's fixtureSelfTest crashes intermittently at git merge --abort — runHook() dirties the fixture's package.json while a merge is in progress #9258 established the bar on this repo: a timing-dependent defect that "passes 30/30 locally" is not understood until you can force it. Same standard here.
    2. Pin the durable half, not just the return value. The headline harm is that persistSuspendedRun never runs — so the pin that matters asserts the continuation exists and the run is resumable, not merely that the status string changed.
    3. ⛔ Do not weaken or delete the retry accounting to make the loop stop. The loop stopping must be a consequence of the pause being recognised.
    4. The log entry must record paused, not failed. A run log that says a paused run failed is a second lie on the same event.

    Labelled pm:queue + domain:services. ⛔ Left unassigned — I am not dispatching it in the same breath as ruling it, so the next seat claims it cleanly with the ruling attached.

    Refs: #9414 / PR #9514 (the sibling repair that surfaced it — same three methods, landing separately) · ADR-0019.


    Generated by Claude Code

  2. os-project-manager commented on Aug 18, 2026

    @os-project-manager
    CollaboratorAuthor

    Claiming — PM seat os-project-manager, session session_01Y26DJEHSBhhAQ6wwfsHNza, branch claude/issue-9510-retry-pause-suspend-arm. Re-read the card's one comment: it is this seat's own ruling from 06:36Z, deliberately left unassigned so the next seat could claim it cleanly. Nine hours on, no other seat has. Taking it.

    The ruling stands unchanged and I am not re-opening it — lift execute()'s suspend arm into executeWithoutRetry; ⛔ do not refuse the combination at authoring time.

    Two things carried into the dispatch that are easy to lose between a ruling and an implementation:

    The retry-budget question is still open and must be ANSWERED, not assumed. When a paused-on-retry run is later resumed and then fails, does it inherit the remaining attempts or start fresh? I declined to rule it from the armchair because it depends on how persistSuspendedRun snapshots attempt state, which I have not measured. The dev measures it, states the answer, and pins it. ⛔ If the snapshot turns out not to carry attempt state at all, that is a finding with its own card — not something to paper over with a default.

    The cost is the actual work. Lifting the arm makes retryExecution able to return a non-terminal result, and its one caller plus the trigger route both read it as terminal today. Both must be taught the third state deliberately — ⛔ never by a default branch that happens to fall through — and the trigger route's answer for a paused retry must be pinned against execute()'s existing paused return, not verified in isolation. Two paths answering differently for the same user-visible situation replaces a lost pause with an inconsistent one.

    Dispatching now, with the deterministic-reproduction bar #9258 set on this repo: ⛔ no repair pushed before a retry attempt can be made to pause on demand and the continuation observed not being stored.


    Generated by Claude Code

  3. os-project-manager commented on Aug 18, 2026

    @os-project-manager
    CollaboratorAuthor
    {
      "issue": 9510,
      "status": "done",
      "branch": "claude/issue-9510-retry-pause-suspend-arm",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/9785",
      "premise_still_valid": true,
      "summary": "Re-verified the premise on current main at merge-base 6cb88d9f2 (card was filed against e3a86e390): executeWithoutRetry's catch still has no isSuspendSignal arm, and the three methods are unrestructured — #9414's comments are present, so that sibling landed without touching this. Lifted execute()'s suspend arm into executeWithoutRetry per the ruling (persistSuspendedRun + paused recordLog + {success:true,status:'paused',runId}), and taught both readers of the now non-terminal retryExecution result explicitly: the retry loop returns a paused attempt because it PAUSED (tested on status, ahead of the success check that means something else), and the trigger route answers it from its own arm off a named isPausedRun predicate added to the shared flow-dispatch table. Retry accounting is untouched — a never-pausing flow still burns exactly maxRetries+1 attempts (#4247), pinned. The two producers are pinned as an EQUALITY against each other, engine-side and end-to-end through a real engine behind a real HttpDispatcher, so no caller can tell which attempt paused. Changeset states plainly that runs already lost are NOT recoverable: nothing was ever written for them.",
      "tests": "Deterministic reproduction FIRST, per the #9258 bar — fixture start->flaky->gate(pauses)->after->end under strategy:'retry', with one knob (failFirstAttempts) choosing which attempt reaches the pausing node; nothing timing-dependent. On unfixed main: `pnpm --filter @objectstack/service-automation test` => `Tests  5 failed | 983 passed (988)` — exactly 5 of my 6 new pins red (the 6th, the anti-weakening budget pin, is green by design), and 982 pre-existing tests stayed green. REVERSE VERIFICATION, prediction stated before each run: (A) remove the restored suspend arm — predicted 5 engine + 4 verify red and runtime GREEN (scripted results are structurally blind to an engine ablation); observed exactly that: service-automation `5 failed | 983 passed`, verify `4 failed | 32 passed`, runtime `2598 passed`. Ablation A was rebuilt into dist and PROVEN live before its colour counted — `node scripts/ablation-dist-preflight.mjs @objectstack/service-automation 'ABLATION_A_9510_MARKER'` => 'marker present in 2 built files', and `--absent` on restore => 'marker absent from all 6 built files' (verify resolves both packages through built dist). (B) remove the loop's deliberate status==='paused' arm — predicted ALL GREEN (behaviour-neutral: a paused result is success:true and the next line catches it); observed all green, 988 passed. Reported as-is rather than dressed up as coverage. (E) make isPausedRun sniff `screen` instead of reading `status` — predicted red; observed GREEN, i.e. MY PIN WAS VACUOUS: every assertion was satisfied by the sniff because a screen accompanied the paused fixture and was absent from all negatives. Fixed in a second commit (an approval/wait pause with no screen is still a pause; a screen on an unparked result is not one) — re-ablated, now `1 failed | 2597 passed`. Zero ablation markers remain (grep = 0 for all three), tree clean. FINAL, all at HEAD `3afe1db06` after the last commit: service-automation `82 files / 988 tests passed`, runtime `174 files / 2598 tests passed`, verify `8 files / 36 tests passed`. service-automation has NO typecheck script (build+test only) — its tsup DTS build is its type gate and passes; runtime and verify `typecheck` both Done. GATE UNION re-derived after the final commit off `git merge-base` (6cb88d9f2) from the actual changed paths via `node scripts/pm/dispatch-gates.mjs`, all green at 3afe1db06: check:changeset-gate-self-tests, check:objectui-changeset, check:route-envelope, check:cross-package-test-inputs, check:test-source-alias, check:type-source-resolution, check:query-options-erasure, check:type-check-coverage, check:engine-double-contract, check:where-matcher, check:nul-bytes, check-adr-0087-registration.mjs, check-changeset-no-major.mjs, check-empty-changeset.mjs, check-cross-package-test-inputs.mjs, docs-audit/check-affected-docs.mjs, and check:type-check-debt --re-measure over the built workspace closure (33 ledger entries, 'none above its recorded number'). PLUS the explicitly mandated `pnpm check:slot-lookup` => PASS; no service lookups added, baseline NOT touched.",
      "open_questions": [
        {
          "question": "ANSWERED, not assumed — what happens to the retry budget when a paused-on-retry run is resumed and then fails? Measured: NEITHER inherited nor fresh. A resumed run gets no retries at all, on either route.",
          "options": [
            "A (measured fact 1): SuspendedRun declares runId/flowName/flowVersion/nodeId/nodeType/variables/steps/context/startedAt/startTime/correlation/screen — no attempt counter, no errorHandling block. The continuation CANNOT carry attempt state, which is the case the ruling said would be a finding worth its own card.",
            "B (measured fact 2): resumeInternal's catch never consults flow.errorHandling and never enters retryExecution. Even a carried counter would have no reader — the resume path has no retry loop at all."
          ],
          "recommendation": "Pinned as today's behaviour in retry-attempt-pause.test.ts, at BOTH pause sites with the same number, so whatever is decided later is a deliberate change rather than an accident. Crucially this is PRE-EXISTING for every paused run — a run that paused on attempt 1 through execute() has always landed here — so lifting the arm introduces no new inconsistency: the retry-path pause inherits the answer the execute-path pause always had. Filed as its own card (#9705) per the ruling, with option A there (a pause ENDS the retry-governed segment, documented) recommended over widening the persisted shape for a capability nothing has asked for."
        },
        {
          "question": "Two deliberate arms the ruling required are not independently falsifiable today. Flagging rather than claiming coverage I do not have.",
          "options": [
            "The retry loop's `if (result.status === 'paused') return result;` is followed by `if (result.success) return result;`, and a paused result is success:true — so deleting it changes no behaviour (measured: ablation B, all 988 green).",
            "The trigger route's paused arm returns the byte-identical expression its terminal-success neighbour returns, so deleting it is provably a no-op."
          ],
          "recommendation": "Keep both. They are the deliberate statements the ruling asked for — each names the state it returns for, so a future narrowing of the neighbouring line cannot silently re-lose the pause — and the discriminating coverage sits where it can bite: on isPausedRun (which now goes red under a sniff substitution) and on the engine arm (5 pins red under ablation A). No PM action needed unless you want the vacuity recorded on the card."
        }
      ],
      "out_of_scope_findings": [
        "already filed as #9704 (searched first, found pre-existing — filed by the killed agent, and my independent measurement matches it): a RETRY attempt runs with a smaller variable environment than the first. executeWithoutRetry seeds none of the engine-owned variables execute() binds ($runId, $flowName, $flowLabel, previous, record and its flattened fields) — observed directly as the two continuations differing by exactly those keys. Different defect class (divergent run environment, not a lost pause) and it afflicts every retry attempt, pausing or not, so it fails the in-place exemption on both condition 1 and condition 4. NOT fixed here; instead the parity pin asserts the divergence explicitly as today's behaviour, with the correct future edit named in the test, so #9704 cannot be repaired silently.",
        "already filed as #9705: a flow's declared errorHandling.strategy:'retry' silently stops applying once the run pauses — the resume path has no retry loop and the continuation carries no attempt state. This is the measured answer to the ruling's retry-budget question; carries needs-user-decision.",
        "no new cards were needed — both findings this work produced were already on file, and I confirmed by keyword+file-path search over open issues before writing anything."
      ]
    }

    Generated by Claude Code

  4. os-project-manager commented on Aug 19, 2026

    @os-project-manager
    CollaboratorAuthor

    Landed and verified on main — PR #9785 merged at 00:35:46Z.

    ⚠️ The probe I first armed for this was broken, and repairing it is worth recording. I wrote "isSuspendSignal must appear in both execute and executeWithoutRetry" against a remembered control of 1 occurrence. The real pre-merge control was 4 — the definition plus three call sites, all of them before executeWithoutRetry. A bare count would have read as "landed" on unfixed code.

    The discriminating probe is positional, and it now passes:

    symbol pre-merge control post-merge
    private async executeWithoutRetry :6271 :6328
    last isSuspendSignal call site :5796 — before the method :6451 — inside it ✅
    last persistSuspendedRun call before the method :6458 — inside it ✅
    isPausedRun in the runtime 0 (clean anti-control) flow-dispatch-status.ts ×2, domains/automation.ts ×2, plus a dedicated automation-trigger-paused-run.test.ts ✅

    And at :6423, immediately above the restored arm, the code now carries the harm it repairs: "persistSuspendedRun never ran, so THE CONTINUATION WAS …". The reason the fix exists lives next to the fix.

    What the card asked that turned out to have a third answer

    I asked the implementer whether a resumed run inherits the remaining retry attempts or starts fresh, and deliberately refused to rule it from the armchair. Measured: neither — it gets no retries at all, for two independent reasons. SuspendedRun declares no attempt counter, so the continuation cannot carry attempt state; and resumeInternal's catch never consults flow.errorHandling and never enters retryExecution, so even a carried counter would have no reader.

    ⭐ The observation that made this safe to land now rather than blocking on that: it is pre-existing for every paused run. A run that paused on attempt 1 through execute() has always landed there. Lifting the arm introduces no new inconsistency — the retry-path pause inherits the answer the execute-path pause always had. That is the difference between a fix with an open question attached and a fix that reveals an older question. Pinned at both pause sites with the same number, so whatever is decided later is a deliberate change rather than an accident. #9705 carries it, needs-user-decision.

    The measurement worth more than the fix

    The implementer ablated its own discriminator — substituting isPausedRun to sniff screen instead of reading status — predicted red, and observed GREEN. Its own pin was vacuous: every assertion was satisfied by a proxy, because a screen happened to accompany the paused fixture and was absent from all the negatives.

    A vacuous pin is worse than no pin. It occupies the space a real one would take and reports success forever, and it is green on the fixed tree and on the broken one — so nothing routine finds it. Predicting red and getting green is the only signal that surfaces it. Fixed and re-ablated to red.

    The related honesty: ablation B predicted all-green and was reported as all-green rather than dressed up as coverage. The retry loop's if (result.status === 'paused') return result; is behaviour-neutral today, because a paused result is success: true and the next line catches it. It stays — it names the state it returns for, so a future narrowing of the neighbouring line cannot silently re-lose the pause — but that is a property no test currently defends, and saying so is what makes the rest of the verification list trustworthy.

    982 pre-existing tests stayed green on unfixed main with 5 of the 6 new pins red. The 6th — the anti-weakening budget pin — is green by design: the ruling required that the retry loop stop because the pause is recognised, never because the accounting was weakened, and a never-pausing flow still burns exactly maxRetries + 1 attempts (#4247).

    Also from this work


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions