A focused delta-audit of the code added since 1.0.0-alpha7 (the concurrency and recovery work from #3608046: Fix 1.0.0-alpha7 audit findings: completion/timeout/migration concurrency, join-stall recovery, and re-offer notification and the Webform handler unification from #3608073: Unify the webform start and resume handlers into a single start-or-resume handler) found a set of concurrency, data-integrity and correctness defects, plus a few pre-existing Webform issues in the same area. This issue fixes all of them in one merge request.

Security fundamentals were verified sound and are not affected: capability tokens are unforgeable (HMAC), there is no cross-instance or cross-tenant IDOR, and the Cancel URL is not an open-redirect vector. The findings below are reliability and correctness, not authentication or authorization bypasses.

Concurrency and recovery (WorkflowEngine, TimeoutSweep, WorkflowVersionManager)

The common root cause is a "load entity, set a field, save()" sequence inside an open transaction that trusts a plain re-read to observe a concurrent commit, which MySQL REPEATABLE READ defeats (the snapshot is fixed at the transaction's first read, and acquiring a lock afterward does not refresh it). The fix uses the conditional raw UPDATE pattern the code already uses correctly for token claims.

  • Dead-letter window: the dead-lettered token's ERROR status and its incident are committed by two separate writes, so a reconcile sweep running in between sees zero live tokens and zero open incidents and completes a failed instance as COMPLETED. Fix: commit the ERROR status and the incident (or the FAILED status) in one transaction, and treat a STATUS_ERROR token as blocking completion.
  • TimeoutSweep resolves nodes from the live workflow config, not the instance's pinned definition, so a pinned instance's timeout behavior can change under it, and a due token whose node was removed or renamed is skipped without clearing its deadline, permanently occupying the head of the deadline-sorted batch and starving every later timeout. Fix: resolve from the pinned definition, and clear (or log-and-clear) an unresolvable token's deadline so it cannot block the batch.
  • Instance migration re-reads token status with a plain SELECT inside its transaction and can therefore write a stale row back over a token a concurrent signal() consumed, resurrecting it beside its successors. Fix: rewrite node_id and definition_version with conditional UPDATEs guarded on the expected status.
  • armJoinTimeouts, when reached inside an advancing transaction (a self-cancelling cohort branch), re-reads a waiting token under the join lock but on the transaction's stale snapshot, and can write a deadline back onto a token a concurrent join fire already consumed, later double-firing the join. Fix: arm the deadline with a conditional UPDATE guarded on status and a null deadline.
  • checkCompletion flips the instance to COMPLETED with an unconditional save, so a reconcile sweep can overwrite a concurrent cancel or fail. Fix: flip with a conditional UPDATE (WHERE status running) and run the completion side-effects only when it changed a row.
  • skipIncident and failFromIncident resolve the incident before the compensating transition commits, reopening the same reconcile window. Fix: resolve the incident inside the transaction that re-activates the token or flips the status.
  • Stall recovery (recoverStalledInstance) is only reachable from cancel(); the documented cron caller does not exist, so a join that loses its deadline can hang forever. Fix: add the sweep leg that finds a running instance whose live tokens are all waiting with no deadline and recovers it.
  • The reconcile batch has no ordering and does not exclude incident-halted instances, which match it on every run; once enough accumulate they starve genuinely stuck instances. Fix: exclude open-incident instances in SQL and order the scan.
  • A join fire can leave a surplus waiting sibling carrying the armed deadline, which the sweep later fires as a bogus second cohort. Fix: clear the surplus sibling's deadline on fire.
  • Lifecycle and audit events are dispatched inside open transactions on the recovery paths, so a rolled-back recovery can still have emitted an external notification. Fix: dispatch after commit where feasible, or document the transactional-listener contract.

Webform interaction (handler, interaction plugin, element)

  • On resume the handler resolves the parked token by node (lowest id) rather than the exact token the branch-scoped handle named, so with two tokens parked on one node (parallel or converging branches) one party's submission resumes and stores data on another branch. This matches the pre-alpha8 behavior; it is fixed here. Fix: resume the exact named token when it is still parked, falling back to node resolution only for a genuine loop re-entry.
  • A collect step reopens the instance's latest submission of the webform, not the current branch's, so parallel branches collecting on one webform can hand one party another's submission. Fix: scope the reopen to the branch.
  • The binding element stores the live capability token in submission data (a bearer value at rest, reachable through exports while the target step is still parked). Fix: redact the stored value once the submission is bound.
  • Minor: a workflow-starting submission that starts inside preSave can orphan a RUNNING instance if the submission save then fails; an expired binding token on a start-capable form is silently swallowed; re-editing the starting submission re-seeds instance variables. Each is tightened.

Tests

Each fix adds or extends coverage for the specific interleaving or topology it addresses, and the full WorkflowEngine, orchestra_interaction_webform and orchestra_webform_example suites pass.

Issue fork orchestra-3608117

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mably created an issue. See original summary.

mably’s picture

Status: Active » Needs review
mably’s picture

MR !270 opened against 1.x. Orchestra kernel suite passes locally (core engine: reconcile, join-stall recovery, dead-letter, join, timeout, migration, incident; plus the interaction_webform handler, 10/10). Functional and FunctionalJavascript run in CI (they need the container's webserver and ChromeDriver).

  • mably committed 816b61ab on 1.x
    fix: #3608117 Fix 1.0.0-alpha8 delta-audit findings: concurrency...
mably’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.