Issue #3608146: Consolidate the recovery layer behind one guarded transition primitive

Frank Mablyrequested to merge
issue/orchestra-3608146:3608146-recovery-refactor into 1.x

Pays down the structural debt behind the alpha8 delta-audit (#3608117): the audits kept finding the same class of concurrency bug in different places because state changes were made ad hoc at each call site. This is a pure, behavior-preserving refactor. No runtime behavior changes; the full kernel suite stays green.

What changed

1. One guarded state-transition primitive

transition(ContentEntityInterface $entity, string $expected, string $new, array $extra = []): bool is now the single guarded status flip. It is a guarded UPDATE ... SET status = :new WHERE id = :id AND status = :expected, which current-reads the row and so wins only the single expected-to-new transition, even inside a caller's open transaction. On success it syncs the in-memory copy and invalidates the entity caches.

  • claimTerminalStatus() is now a thin wrapper over it that always stamps changed (retention needs it).
  • handleAdvanceFailure()'s dead-letter ERROR claim routes through it.

A raw UPDATE does not fire entity save hooks, so it is deliberately reserved for contended flips (completion, cancellation, failure, dead-lettering). Single-actor operator transitions that must cascade through a save hook (an incident resolution, where orchestra_inbox cancels the work item on orchestra_token_update) stay an entity save() on purpose. That distinction is now documented, not accidental.

2. The isolation invariant, codified

The primitive's docblock states it once, authoritatively: under MySQL REPEATABLE READ a transaction reads from its snapshot, so a plain entity re-read (loadUnchanged(), or load-then-setStatus-then-save()) cannot see a status another transaction committed after the snapshot began, even under a lock. Never trust a re-read to observe a concurrent commit; guard the write instead. The loose "fresh status read" comment in checkCompletion() was corrected to point at this.

3. The two recovery sweeps consolidated

reconcileStuckInstances() and recoverStalledInstances() each re-derived "find running instances of a stuck shape, excluding incident-halted ones, in a bounded ordered batch." They now share:

  • stuckInstanceQuery($limit): the base finder (status = running, NOT EXISTS open incident, ordered by id, range-limited);
  • sweepInstances($query, $handler, $log): execute + load + per-instance isolated try/catch;
  • tokenInstanceColumn(): the repeated table-mapping boilerplate.

Each sweep now adds only its own shape subquery.

Minor intentional delta

Because the primitive is uniform, the token dead-letter path now also invalidates the token's cache tags after its ERROR flip (the old inline code reset the entity cache but did not invalidate tags). This is over-invalidation at worst (a spurious cache miss if the surrounding transaction later rolls back), never staleness, and invalidating a token's tags on a status change is the more correct behavior anyway.

Scope note

The exactly-once advance/join machinery (token park/consume, join arm, sibling consume) was verified sound by the delta-audit and is explicitly out of scope here: those are single-writer writes within an established claim, not contended re-reads, so they are labelled as such rather than rewrapped.

Testing

  • Full orchestra kernel suite: green.
  • phpcs (Drupal + DrupalPractice, warnings on): clean.
  • Adversarial behavior-equivalence audit: performed.

Refs #3608146. Follow-up to #3608117.

Merge request reports