The pre-alpha12 code audit found several correctness issues clustered in the engine timeout, park and resume paths. Each preserves core integrity in the common case but can double-fire a side effect or roll back committed progress under concurrency or misconfiguration. They are grouped here because they share the timeout and park machinery.

E1 (major): the timeout claim can double-fire or fire early on a re-armed deadline. In TimeoutSweeper::fireInner() and fireTimerInner(), the PARKED timeout claim matches the exact deadline value but does not also require the deadline to be at or before now. A non-resuming action (notify, reassign, unclaim) that re-arms the deadline to a future value lets an overlapping sweep, or an operator "fire now", claim and run the action a second time before it is due: a premature reassign, unclaim, or subprocess escalation. The WAITING branch already guards correctly.

G1 (major): park-path notification dispatch runs unguarded inside the engine transaction. TokenParkedEvent, and the work-item assignment event chained from it, are dispatched inside advanceNode(), whose transaction rolls back and rethrows on any exception. The notification dispatchers do audience resolution and link issuing with no try/catch, so a notify-config error (for example a users-by-variable audience against a malformed variable) rolls back the committed park and dead-letters the token. Only the terminal mail send is currently guarded; the timeout path shows the intended pattern.

I1 (minor): resumeWithResult() returns TRUE even on a lost race. It decides its return value from a non-atomic pre-check, while signal() silently no-ops when a concurrent caller already advanced the token, so a true double-submit double-fires the webform onResumed() extension point. The engine still advances exactly once.

E2 (minor): a persistently throwing timeout action re-fires every cron run. The timeout-action path lacks the bounded-retry-to-incident dead-letter treatment that every other failure path has, so a deterministically failing action hot-loops indefinitely rather than escalating to an incident.

E3 (minor, design call): a run completing at a terminal node that is not an end node keeps the last parked node authored status. setInstanceStatus() fires on park and on reaching an end node, and is cleared on cancel and fail, but not on completion at a plain terminal node, so the instance can complete still carrying its last milestone. Decide whether to resolve the status from any completing terminal node or clear it on completion.

G2 and G3 (minor): status and initiator are absent from the signed audit trail. setInstanceStatus() writes via a bare update with no auditable event, and the started audit context omits the instance initiator, so neither the authored-status milestones nor who started a run are recorded in the tamper-evident trail.

Fixing all of the above in one merge request, with discriminating kernel tests (including the missing status-cleared-on-cancel case) and French translations.

Issue fork orchestra-3611505

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 committed 3edb63f3 on 1.x
    fix: #3611505 Engine exactly-once, notification side-effect isolation,...
mably’s picture

Status: Active » 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.