fix(approvals): a dead approval run no longer leaves the record RECORD_LOCKED (#3456) - #3703
Merged
Merged
Conversation
…D_LOCKED (#3456) The record lock keys on a pending `sys_approval_request` and could not tell the run that OWNS that request from an unrelated user editing the record. A flow that touched its own target record while its own approval was still pending — a manual `resume` with no decision, or a node writing the record between opening the approval and the decision — died on its own `RECORD_LOCKED`, leaving the record locked behind the dead run. Prevention: the automation engine stamps `flowRunId` onto the run context at setup, alongside `runAs`, and it travels with every data node's ObjectQL context into `ctx.session`; the lock hook exempts a write whose `flowRunId` matches the pending request's `flow_run_id`. Keyed on run identity rather than elevation on purpose — a `runAs:'user'` run stays RLS-scoped while it writes. The field is pure provenance: server-constructed like `isSystem`, never client-supplied, read by no security middleware, and it permits exactly one write — to the record its own run already holds a pending request against. Recovery: a sweep on the existing approvals clock finalizes a pending request whose owning run is terminal (`completed`/`failed`/`cancelled`/`timed_out`) as `recalled`, releasing the lock, audited under the reserved actor `system:dead-run`. This covers the case no in-band handler can — a run killed by a process crash. Fail-safe by construction: it acts only on an explicit terminal status from a closed set, so `paused` (a live approval), `running`, an unknown status, an unreadable run and a deployment with no automation engine are all read as alive. Also fixes `AutomationEngine.getRun`, which returned the FIRST log entry for a run id. A run that pauses then finishes records two entries under one id, so every suspend-then-finish run — every approval, screen and wait flow — reported itself as `paused` forever, on the Runs surface and to this sweep alike. Residual, deliberate: a `runAs:'user'` run with no trigger user (a schedule) passes no ObjectQL context at all, so it carries no `flowRunId` and is still subject to the lock. Manufacturing a context to carry the run id would flip that run from its documented unscoped fail-open (#1888) to baseline-member RLS — a separate, larger change. The sweep recovers that shape. Tests: plugin-approvals 229, service-automation 365, objectql 1092, lint 390 — all green. Each new guard was mutation-verified: reverting the exemption, the closed terminal set, or the getRun fix turns the matching tests red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
size/l
documentation
Contributor
📓 Docs Drift CheckThis PR changes 4 package(s): 110 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…the lock's two new behaviours The `flowRunId` field added to `ExecutionContextSchema` left the GENERATED `content/docs/references/kernel/execution-context.mdx` stale, failing the spec `check:docs` gate. Regenerated via the prescribed path (`gen:schema && gen:docs`) — a one-row addition to the field table. Also updates the hand-written approvals guide, which described the record lock as absolute and predated any automatic recovery: - the lock now exempts the run that opened the request, and the exemption is keyed on run identity rather than elevation, so a `runAs:'user'` run stays RLS-scoped while it writes; - a dead run's lock is released by the sweep, with the note that it acts only on a positively-confirmed terminal run — a paused run, an unknown run, or an unreachable engine all count as alive. `check:docs` and `check:doc-authoring` both green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv
os-zhuang
marked this pull request as ready for review
July 27, 2026 15:31
os-zhuang
added a commit
that referenced
this pull request
Jul 27, 2026
…on (#3456 follow-up) (#3719) Follow-up to #3703. Tests plus one comment reference; no behaviour change. `releaseDeadRunRequests` recalls a pending request whose owning run is terminal, on the premise that such a pair can only be an orphan. That premise held only because every in-band transition moves the request out of `pending` before handing the run back — a convention spread across four public methods and seven resume/cancelRun call sites, enforced by nothing. Reverse it anywhere and the sweep would cancel a live approval. Pins the invariant itself rather than any one method's call order: at the instant the run is handed back, no request owned by that run may still be `pending`. Seven scenarios cover all seven sites — decide approve/reject, recall, send-back, send-back past the revision budget (ADR-0044 auto-reject), recall inside the revise window (cancelRun), and resubmit. Shown load-bearing by a reorder-only mutation of `recall` (end state identical): exactly one test fails, the new one, with the other 235 green — which is also the proof no existing test covered the ordering. Also points the `resolveRunDataContext` residual comment at #3712, which tracks the schedule-triggered run that still cannot carry `flowRunId`.
os-zhuang
added a commit
that referenced
this pull request
Jul 28, 2026
…ord (#3712) (#3749) Closes #3712. Residual of #3456, left open by #3703. A run with no principal now passes provenance alone: `resolveRunDataContext` returns `{ flowRunId }` — no userId, positions, permissions, or `isSystem`. Every principal gate keys on one of those fields, so the context authorizes identically to no context at all; the run keeps its documented #1888 unscoped posture and only becomes attributable. Provenance moved out of the hook session into `ctx.provenance`, so an identity-less writer is never forced to present an empty session — which would have turned "no caller" into "an anonymous caller" and narrowed the #1888 fail-open for attachments alone. Also relaxes `BaseEngineOptionsSchema.context` to a partial envelope (`ExecutionContextInput`): parse-time defaults made positions/permissions/ isSystem required on a caller-supplied option, asserting that every data-engine context carries a principal.
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3456 — the #3424 follow-up, expected-behavior item 3. Implements option 3 (both) from the issue: exempt the owning run's writes and release the lock when a run dies anyway.
The gap
The record lock keys on a pending
sys_approval_requestand cannot tell the run that owns that request from an unrelated user editing the record. So a flow that touched its own target record while its own approval was still pending — a manualresumewith no decision, or a node writing the record between opening the approval and the decision — died on its ownRECORD_LOCKED, and the record stayed locked behind the dead run. Recovery existed (#3424 lets an adminrecall/reject), but nothing made it self-healing.Only one of the two options would have left a hole, so both are here: option 1 stops the run from dying, option 2 cleans up the runs that die for reasons option 1 can't prevent (a process crash being the clearest).
① Prevention — the owning run may write its own record
The engine stamps
flowRunIdonto the run context at setup, alongsiderunAs— same lifetime, same single construction point (resolveRunContext). It ridesresolveRunDataContextinto every data node's ObjectQL context, throughbuildSessionintoctx.session, and the lock hook exempts a write whoseflowRunIdmatches the pending request'sflow_run_id. Because it lives on the run context, it is persisted with a suspended run and survives pause/resume, including a cold resume after a restart.Keyed on run identity, not elevation — deliberately. Widening the write to
isSystemwould have been the easy fix and the wrong one: arunAs:'user'run must stay RLS-scoped while it writes.flowRunIdis pure provenance — server-constructed likeisSystem(this envelope is built from the authenticated session, never from request input), read by no security middleware, and it authorizes exactly one thing: a write to the single record its own run already holds a pending request against. A strayflowRunIdagainst a request with noflow_run_idis not a skeleton key; there is a test for that.② Recovery — release records held by runs that died anyway
A pending request whose owning run has reached a terminal state can never be decided, so it is finalised as
recalled— which releases the lock — and audited under the reserved actorsystem:dead-runwith the run id and its status in the comment, so it is never mistaken for a submitter's withdrawal.recalledis reused because it is already the platform's terminal state for a live request that ended without a decision; the history view lists it, and no newApprovalStatusvalue has to enter the spec.It runs on the existing approvals sweep clock (
Promise.allSettledalongside the SLA scan, so neither leg can short-circuit the other), which also gives it the boot catch-up the crash case needs.Fail-safe by construction. It acts only on an explicit terminal status from a closed set.
paused— the normal state of every live approval — plusrunning, an unrecognised status, an unknown run, agetRunthat throws, and a deployment with no automation engine are all read as "still alive". The failure mode is "a dead run's lock survives until an admin recalls it" (today's behaviour), never "a live approval is destroyed".completedsits in the terminal set with the failure states because the approval node only writes a request row on the path where it also suspends, and every in-band transition finalises the request before resuming the run — so a completed run with a pending request means someone resumed it out of band.A prerequisite bug this surfaced
AutomationEngine.getRunreturned the first log entry for a run id. A run that pauses and later finishes records two entries under one id (paused, then the terminal one), so every suspend-then-finish run — every approval, screen and wait flow — reported itself aspausedforever. That is user-visible on the Runs observability surface (runtime/src/domains/automation.ts), not just an obstacle to this sweep: without the fix the sweep could never have seen a dead approval run, because those are exactly the runs that pause first. Fixed to take the latest entry.Verification
plugin-approvals229,service-automation365,objectql1092,lint390 — all green, and all four changed packages build clean through DTS.Every new guard was mutation-verified — each fix reverted in turn, confirming the tests go red for the right reason:
allows the OWNING run to write its own target recordpaused, unrecognised,runninggetRunlatest-entry fixNew coverage includes the end-to-end proof — a user edit is refused while the dead run holds the lock and succeeds after the sweep — and an integration test asserting
flowRunIdreaches the ObjectQL context and equals the engine's own run id, for bothrunAsmodes, plus a check that the stamp never leaks back onto a caller-reused context object.On the full-workspace
pnpm test: touchingpackages/specmakes turbo rebuild it concurrently with dependents' tests, and those tests transiently fail withCannot find package '@objectstack/spec/integration'whiledistis being rewritten. It is a build/test race, not a regression — the failing package differed run to run, and every affected package passes standalone once spec'sdistis stable.Residual, deliberately not changed
A
runAs:'user'run with no trigger user (a schedule) passes no ObjectQL context at all, so it carries noflowRunIdand is still subject to the lock. Manufacturing a context just to carry the run id would flip that run from its documented unscoped fail-open (#1888) to baseline-member RLS — a separate and larger behaviour change. The sweep is what recovers that shape. Recorded in a code comment atresolveRunDataContext, in the changeset, and here.Generated by Claude Code