fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot - #14334

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state
Sep 2, 2026
Merged

fix(automation): resume a paused run from the shared store, not from a stale per-replica snapshot#14334
os-sales merged 4 commits into
mainfrom
claude/issue-13617-approval-resume-stale-run-state

Conversation

@claude

@claudeclaudeBot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes#13617

What was wrong

AutomationEngine keeps paused runs in a per-process Map, and
loadSuspendedRunStrict read that map first, consulting the durable
sys_automation_run row only on a miss. That is a correct read for exactly one
deployment shape: a single process. Put three replicas behind a load balancer
over one postgres and the map becomes a per-replica snapshot of the node a run
was parked at the last time that replica touched it — and nothing invalidates
it, because there is no invalidation channel to it at all.

Traced against the report's three-level approval flow:

  1. replica A parks the run at lv1 and keeps it in memory;
  2. the lv1 decision round-robins to replica B, which advances the run to lv2
    in the store and in B's memory — A's memory still says lv1;
  3. the lv2 decision lands back on A, which read its own memory, resumed from
    lv1, and traversed to lv2a second time.

That is the reported symptom exactly: the same level re-opened as a fresh
pending request tens of milliseconds after the previous one completed, so one
approver approves every level twice and a three-level flow yields five
sys_approval_request rows. Land the same one-beat-stale read on the final
level and the run rolls back to the previous level and never terminates, which
is the second shape the reporter added in their follow-up comment. Both shapes
are one mechanism at two points in the flow. A single replica shows zero
duplicates because there is one map and it is never behind.

Two callers that must not be wrong funnel through that one reader:
resumeInternal (which node does this resume continue from) and
hasSuspendedRun (the approvals pre-flight that decides whether to record a
decision at all), so both were reading the same stale answer.

The fix

The resume path is now store-authoritative. With a SuspendedRunStore
configured, the store answers where a run is parked; the in-memory map is
consulted only for a run whose durable save failed.

That last clause is deliberate and is why a new cacheOnlySuspensions set
exists. persistSuspendedRun documents a degradation and logs it at error: a
failed durable save costs cross-restart durability, not in-process
resumability. Making the store authoritative without tracking which rows it
never received would have silently converted that into an unresumable run. The
store's "no row" is authoritative for every run it ever accepted — including the
runs this process advanced past, which is the whole defect — and says nothing
about a row it was never handed.

forgetSuspendedRun clears the qualifier alongside the cache entry; it is the
single choke point every consumption passes through, so the set is bounded by
the map it qualifies.

The resume ordering is untouched. The suspension is still consumed before
traverseNext, and forgetSuspendedRun is unchanged — this changes only
which suspension is read, never when it is consumed. #13937 owns that fork
and it is not pre-empted here.

The triage questions, answered

1. Is the missing attachClusterPubSub() this card's root cause, or another
gap in the same environment?
Another gap — measured, not argued.
attachClusterPubSub exists only in packages/metadata (MetadataManager) and
packages/objectql (the write-epoch mirror); a grep for cluster or pub/sub
wiring across packages/services/service-automation/src and
packages/plugins/plugin-approvals/src returns nothing but one unrelated prose
comment. There is no invalidation channel to the run-state map that the missing
bridge could have disabled. Attaching that bridge moves none of the tests in
this PR; reading the shared store moves all of them.

2. Is "resume from shared storage" the fix or a workaround? The fix, and it
is the card's own stated expectation. No lock and no serialization of approve
was added — that would have been masking, and per the triage constraint it would
have been reported rather than chosen.

3. Cluster surface not re-done. Nothing outside
packages/services/service-automation/src is touched. The reporter's negative
controls (record-change dispatch fires once, the scheduled-job fence elects one
leader, one notification per event) are consistent with a defect local to this
one reader, and that is where the change stays. packages/services/service-cluster
was read as an exemplar only — no write.

Sibling #13686 (interval-job leader election) refuted as the cause. An
approval resume arrives on the decision-write path — ApprovalService calls
automation.resume(runId, ...) directly once it has recorded the decision — not
from a job tick. No scheduling is involved, so leader election cannot be the
mechanism. #13686 landing did not and could not fix this.

Verification

Reverse verification, from the committed state, with the mutation confirmed on
disk (blob hash changed; injected line counted) and the restore confirmed by
byte identity against the HEAD blob plus an empty git diff HEAD:

Restoring the old cache-first order at the top of loadSuspendedRunStrict takes
the new pin file to 4 red / 4 green, measured:

casered without the fix
shape 1, middle level[ 'lv1', 'lv2', 'lv2' ] where ['lv1','lv2','lv3'] is correct
shape 2, final levelexpected 'paused' to be undefined — the run never terminates
finished runthe stale replica resumed a finished run and reported success
unreadable storeno STORE_UNAVAILABLE at all — the cache hit meant the broken store was never read

The four that stay green under that mutation are the ones that must: the
single-replica control (the report's own control), the healthy cold-replica
control, the no-store control, and the failed-durable-save degradation. A fix
that moved the defect rather than removing it would have taken one of those
with it.

Refusals are asserted by code (RUN_NOT_FOUND, STORE_UNAVAILABLE) plus the
absence of a run status, never by a bare throw.

Ran, all at 74c5880b unless noted:

  • pnpm --filter @objectstack/service-automation test — 97 files, 1157 tests, green.
  • pnpm --filter @objectstack/plugin-approvals test — 35 files, 652 tests, green.
    These resolve service-automation through its built dist, so the fix is
    verified through the published entry point, not only in source. (Both packages
    needed their dependency closure built first; the initial resolve failures were
    a stale worktree, not this change.)
  • tsc --noEmit on service-automation: exactly 3 errors, byte-identical to
    the frozen ledger entry (TS2341 x3 in nested-region-parity.test.ts at
    95/151/180). This change adds none. --listFiles confirms engine.ts, the new
    pin, and wait-node.test.ts are all really in the program.
  • The full derived gate family, harvested runnably with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (35 commands = 27 by path and kind, plus 8 the changeset brings): 34 green, 1
    NOT MEASURED
    . The one is scripts/check-test-completeness.mjs, which exits 3
    = PREREQUISITE NOT MET because it grades a saved turbo run test log that only
    CI produces; its own output says this branch is not a finding. Exit codes were
    captured before any pipe.
  • pnpm lint (repo-wide eslint . --no-inline-config) — green, 71s. Not
    narrowed; the whole scan ran.
  • The ratchet families were re-run on this exact head after the final commit:
    check:type-check-debt, check:type-check-coverage,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:objectql-double-limit — all green.

Also in this diff, and why

Three prose corrections that this change makes necessary rather than optional.
builtin/wait-node.ts and its test documented, at length, that
STORE_UNAVAILABLE was reachable only from the re-arm callback because a run
parked in this process was answered from memory for the life of its suspension.
That is no longer true, and the ablation measured it turning red. The notes now
say so. The arming-path specimen itself is unchanged on purpose: it runs on an
engine with no store, so what it pins is still the handler's branch.
suspended-screen-durability.test.ts had one comment asserting a same-process
read needs no store read; corrected, assertions untouched.

Deliberately not changed

cancelRun and failAncestors also read the map before the store, and
listSuspendedRunsDurable merges with in-memory entries winning. They are the
same class but not the same fix: the first two carry bespoke degradation
contracts with their own recorded verdicts, so correcting them is a judgment
call rather than a mechanical one, and the third has no in-repo consumer. Filed
as #14332 rather than folded in.

A residual this PR does not close: two decisions for the same run arriving
on two replicas simultaneously can still both read the same fresh row and both
advance, because the resuming guard is per-process. That is a concurrency
race, not the reported one-beat staleness (the report's decisions are human
approvals, sequential), and closing it needs a compare-and-set on the run's
advance — an optimistic-version column on sys_automation_run, i.e. a schema
change and a widened SuspendedRunStore contract. Filed as #14333, not built.


🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…store
WIP — implementation only; regression tests follow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…-authoritative resume
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx(via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/builtin/wait-node.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739packageMentionDocs.

Which tree this was computed on

This run read content/docs from 945b6971f1e510ceba3a4ecbbe520f5580d63229 — the merge of head 74c5880bedb017c3f7a144579c71e4fe79d4544a into base f645d6f8879f5e868b8c0aac978dc0db92552739, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 945b6971f1e510ceba3a4ecbbe520f5580d63229 && git checkout 945b6971f1e510ceba3a4ecbbe520f5580d63229
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f645d6f8879f5e868b8c0aac978dc0db92552739 74c5880bedb017c3f7a144579c71e4fe79d4544a && git checkout -B drift-repro f645d6f8879f5e868b8c0aac978dc0db92552739 && git merge --no-ff 74c5880bedb017c3f7a144579c71e4fe79d4544a
node scripts/docs-audit/affected-docs.mjs --json f645d6f8879f5e868b8c0aac978dc0db92552739

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f645d6f8879f5e868b8c0aac978dc0db92552739 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-salesClaude

Copy link
Copy Markdown
Collaborator

Landing provenance (domain:services seat, session session_01AUF1NoViznQK32gqpK8wS8): flipped ready and enqueued via auto-merge on head 74c5880b after the landing checks — PM ACCEPT on the card (13617#issuecomment-5503382212: scope, exports, resume-ordering fence, triage answers and ablation verified against the tree); all 33 check runs on this head completed success or path-filter skipped (Lint & Repo Gates green 02:40:32Z, Test Core rollup green 02:37:31Z); Clause-② no (zero new exports, no contract file) so no contract-review carrier applies. Path surface: packages/services/service-automation/** + one changeset — no governed surface, so the merge queue is the landing path. Follow-ups #14332 and #14333 stay open for triage.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sales@claude