Skip to content

fix(flow): the heartbeat recovers a missed signal instead of rolling forever - #3358

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/heartbeat-recovers-missed-signal
Sep 3, 2026
Merged

fix(flow): the heartbeat recovers a missed signal instead of rolling forever#3358
rubenvdlinde merged 1 commit into
developmentfrom
fix/heartbeat-recovers-missed-signal

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The defect

A UserTask's completion signal was refused on the acceptance rig — [FlowRunSignalService] Refused a signal: the actor is not the awaiting step's assignee, because the assignee group did not exist at signal time. The suspended run's 30-minute heartbeat then fired, re-suspended, and never advanced: resume_at rolled 08:07 → 08:37 → … indefinitely, while the task sat completed and the group had long since been created.

The heartbeat exists precisely to recover a missed signal. It recovered nothing.

The root cause was not the node

UserTaskNode has always re-read its task on re-entry and applied the outcome when terminal. What wedged is upstream:

persistResult() dropped every parked node's resume slot whenever a pass ended anything but suspended. FlowResumeState::storableWhen(suspended:) read NOT-SUSPENDED as "nothing left to continue from" — which conflates it with TERMINAL. A pass legitimately ends queued while a node parked in an earlier pass is still waiting: the in-request advance of a sibling branch (FlowTaskBridge::continueRun()advanceStream()) finalises queued whenever other enabled work remains, and a claim refused on contention does the same.

The parked node then lost the uuid of the task it was waiting on. Its next wake found an empty slot and — correctly, by its own idempotency guard — created a new task. From that moment:

  • the original task's completion could never address the node's slot, and its signal was refused against the new slot's recorded assignee — the refusal observed on the rig;
  • every heartbeat re-read the new, still-open task and re-suspended, rolling resume_at forever;
  • a duplicate task sat in somebody's inbox.

Recovery semantics

  1. Slots survive every live pass end. storableWhen() keeps the per-node slots for any non-terminal status (suspended, queued, running) and drops them only on a terminal run. persistResult() derives that from FlowRun::TERMINAL, in a new keepResumeSlots() that carries the reasoning.
  2. A recovered delivery is audited. When a task-waiting node reads its task terminal with no signal in hand (the completion's wake never arrived), it records heartbeat-recovered on the task's audit through FlowTaskBridge::recordHeartbeatRecovery(), attributed to the task's completedBy — the fact recorded is that person's answer arriving late, not the cron job acting. Best-effort: an audit failure never un-recovers the run. The guarded signal seam already records the refusal; this is the other half of that trail.
  3. No second delivery mechanism. The wake, the node's re-read and the outcome application are exactly the paths that already existed. Per-node slot addressing (fix(flow): a resume answers only the node whose slot it answers, and the kill switch actually stops runs #3325) is untouched, so only the addressed node's slot recovers.

The symmetric cases, decided explicitly

  • A task completed while the run was not yet suspended (the race). signal() refuses a non-suspended run, so that wake is lost. The node then parks on a non-null heartbeat, and the next wake re-reads the task — with the slot now durable, the race costs at most one heartbeat period. No pre-suspension re-check is added: a node cannot read an answer before it has parked on the question.
  • A task whose sequence concluded (TaskSequenceService). A sequence drives every transition through TaskService's verbs, so the task named by the node's slot reaches its terminal state on the same row the heartbeat re-reads. Terminality is a property of that row; no sequence-specific handling is needed.

Tests

tests/Unit/Service/Flow/FlowHeartbeatRecoveryTest.php drives the real engine, dispatcher, node, stream walk, claims and commit path over in-memory mappers, so the pass-to-pass persistence that loses the slot is exercised exactly as the worker exercises it. Only the task bridge is mocked (tasks are rows in another service's table).

  • the in-request advance keeps the sibling's parked slot — proven red on the unfixed storableWhen, and it is the wedge's root cause;
  • the heartbeat recovers a completion whose signal was refused, with attribution and no duplicate task;
  • a still-open task re-parks unchanged (askedAt not restamped);
  • only the addressed node's slot recovers; the waiting sibling keeps its own task.

Plus: recovery-vs-signal audit pairs on UserTaskNodeTest and PortalTaskNodeTest, and storableWhen() live/terminal/empty cases on FlowResumeStateTest.

Full unit suite green: 19,093 tests. PHPCS, Psalm (no errors), PHPStan (no errors) and PHPMD (both rulesets) clean.

Out of scope

Runs already wedged before this fix — slot lost, duplicate task created — cannot be recovered retroactively: the original task's uuid is gone from the run. They end at the abandoned-signal reaper or by manual retry, as today.

🤖 Generated with Claude Code

…forever

A UserTask's completion signal was REFUSED on the acceptance rig (the
assignee group did not exist at signal time), and the suspended run's
30-minute heartbeat then fired, re-suspended, and never advanced:
resume_at rolled 08:07 -> 08:37 -> ... while the task sat `completed` and
the group had long been created. The heartbeat exists precisely to recover
a missed wake; it recovered nothing.

The node was never the problem. UserTaskNode has always re-read its task on
re-entry and applied the outcome when terminal. What wedged is upstream:
persistResult() dropped EVERY parked node's resume slot whenever a pass
ended anything but `suspended`. FlowResumeState::storableWhen(suspended:)
read NOT-SUSPENDED as "nothing left to continue from", which conflates it
with TERMINAL -- and a pass legitimately ends `queued` while a node parked
in an EARLIER pass still waits: the in-request advance of a sibling branch
finalises `queued` whenever other enabled work remains, and a refused claim
does the same.

The parked node then lost the uuid of the task it was waiting on. Its next
wake found an empty slot and -- correctly, by its own idempotency guard --
created a NEW task. From that moment the ORIGINAL task's completion could
never address the node's slot, its signal was refused against the new
slot's recorded assignee (the refusal observed on the rig), every heartbeat
re-read the new open task and re-suspended, and a duplicate task sat in
somebody's inbox.

Slots now survive every pass end the run can still advance from, and drop
only on a terminal one. A recovered delivery is recorded on the task's
audit as `heartbeat-recovered`, attributed to the task's completedBy, so
the trail no longer ends at the refusal.

The symmetric cases need no new mechanism, and the change says so. A task
completed while the run was not yet suspended (the race) loses its signal
-- signal() refuses a non-suspended run -- and the node then parks on a
non-null heartbeat, so the next wake re-reads it; the race costs at most
one heartbeat period. A task concluded by TaskSequenceService reaches its
terminal state on the same row the re-read observes, so it is covered with
no sequence-specific handling. No second delivery mechanism is added: the
wake, the re-read and the outcome application are the paths that already
existed.

Proven red before the fix: FlowHeartbeatRecoveryTest drives the real
engine, dispatcher, node, stream walk, claims and commit path over
in-memory mappers, so the pass-to-pass persistence that loses the slot is
exercised exactly as the worker exercises it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 186460b

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
test-l10n-parity
format
check-schema-l10n
check-l10n-js
composer ✅ 174/174
npm ✅ 543/543
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-03 10:20 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 925c204 into development Sep 3, 2026
45 of 47 checks passed
rubenvdlinde added a commit that referenced this pull request Sep 3, 2026
…3362)

The changed-files coverage ratchet on #3358 was right, and it was pointing
at something real rather than at a percentage. Of the 32 statements that
change added, 30 were the body of
`FlowTaskBridge::recordHeartbeatRecovery()` -- and every test that exercised
the recovery MOCKED FlowTaskBridge, because in those tests the nodes are the
unit. So the method that writes the recovery's audit trail had no execution
coverage at all: the audit entry, its attribution, and the catch that makes
it best-effort were asserted nowhere.

That matters more than the percentage does. The guarded signal seam records
a refusal; this entry is the other half of that trail, and a silent
regression in it would make a recovered answer read as one that vanished.

Three tests through the REAL bridge, in the suite that already builds one:

- the entry is recorded as `heartbeat-recovered`, attributed to the task's
  completedBy, with a reason naming the run whose signal never arrived;
- an audit write that THROWS is swallowed, because the recovery is the node
  applying the outcome and letting the failure out would abort the very walk
  that was un-wedging the run;
- an ending nobody answered (terminated, expired -- `completedBy` is null on
  exactly those) records no actor rather than a guessed one.

And one test for the symmetric case the change documented but left unpinned:
a completion that RACED the suspension. `signal()` refuses a run that is not
suspended, so that wake is lost with nothing to retry it; the test asserts
the refusal, asserts the run parks on a non-null heartbeat, and asserts the
next wake recovers it. That is the whole basis for deciding the race needs
no new mechanism, and it is now falsifiable.

Every one of the four was checked by mutation -- breaking the action name,
the attribution, the catch, or the recovery call itself turns each red.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant