From 565d7afa9d0a3786d073fcd45faec61dcc5c578e Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 3 Sep 2026 12:45:16 +0200 Subject: [PATCH] test(flow): cover the heartbeat recovery paths that no test executed 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) --- .../Flow/FlowHeartbeatRecoveryTest.php | 45 +++++++++ .../Unit/Service/Flow/FlowTaskBridgeTest.php | 99 +++++++++++++++++++ 2 files changed, 144 insertions(+) diff --git a/tests/Unit/Service/Flow/FlowHeartbeatRecoveryTest.php b/tests/Unit/Service/Flow/FlowHeartbeatRecoveryTest.php index 17ed44d8a7..fc0f80b6b8 100644 --- a/tests/Unit/Service/Flow/FlowHeartbeatRecoveryTest.php +++ b/tests/Unit/Service/Flow/FlowHeartbeatRecoveryTest.php @@ -511,4 +511,49 @@ public function testOnlyTheNodeWhoseTaskEndedRecovers(): void { $this->assertArrayNotHasKey('askA', $kept, 'a node that answered has nothing left to remember'); $this->assertSame($taskB, (string)($kept['askB']['taskUuid'] ?? ''), 'the waiting sibling keeps its own task'); }//end testOnlyTheNodeWhoseTaskEndedRecovers() + + /** + * 🔴 THE SYMMETRIC CASE, PINNED: a task completed while the run was NOT yet + * suspended. `signal()` refuses any run that is not `suspended`, so that + * completion's wake is simply LOST — there is no queue for it, and nothing + * retries it. The design decided this needs no new mechanism, and this test + * is what makes that decision falsifiable: the lost wake must cost latency + * only, because the node parks on a NON-NULL heartbeat and the next wake + * re-reads the task. + * + * Were the heartbeat ever allowed to be null here, this run would be + * unreachable forever — `findDue()` never returns a run with a null + * `resume_at` — which is exactly the trap `UserTaskNode` documents. + * + * @return void + * + * @spec openspec/changes/flow-heartbeat-recovery/specs/flow-heartbeat-recovery/spec.md#requirement-a-heartbeat-wake-re-reads-the-awaited-task-and-applies-a-terminal-outcome + */ + public function testACompletionThatRacedTheSuspensionIsRecoveredByTheHeartbeat(): void { + $run = $this->suspendedOnBothTasks(); + $slots = ($run->getContext()['resumeState'] ?? []); + $taskA = (string)$slots['askA']['taskUuid']; + + // THE RACE: the task completes while the run is still mid-walk. The + // completion listener calls signal(), which refuses a non-suspended + // run — so the wake is lost and nothing queues a retry of it. + $this->complete(uuid: $taskA, completedBy: 'bob'); + $run->setStatus(FlowRun::STATUS_RUNNING); + $this->assertNull( + $this->service->signal($run, []), + 'a run that is not suspended refuses the signal, so the completion wake is lost' + ); + + // The walk finishes and the run parks — on a heartbeat that is NEVER + // null, which is the only reason the lost wake is recoverable at all. + $run->setStatus(FlowRun::STATUS_SUSPENDED); + $this->assertNotNull($run->getResumeAt(), 'a task-waiting run must park on a clock, never on a signal alone'); + + // The next heartbeat re-reads the task and applies the outcome. + $run = $this->service->execute($run, $this->flow(), new HeartbeatSubject()); + + $this->assertSame([$taskA], $this->recovered, 'the raced completion is recovered, and audited as a recovery'); + $this->assertSame(['askA', 'askB'], $this->created, 'recovery never creates a task'); + }//end testACompletionThatRacedTheSuspensionIsRecoveredByTheHeartbeat() + }//end class diff --git a/tests/Unit/Service/Flow/FlowTaskBridgeTest.php b/tests/Unit/Service/Flow/FlowTaskBridgeTest.php index fda18e879b..a94003b97d 100644 --- a/tests/Unit/Service/Flow/FlowTaskBridgeTest.php +++ b/tests/Unit/Service/Flow/FlowTaskBridgeTest.php @@ -332,4 +332,103 @@ public function testTheBagSeparatesADecisionFromAnEnding(): void { $this->assertFalse($bag['rejected']); $this->assertSame(Task::STATE_TERMINATED, $bag['outcome'], 'an ending with no outcome reports its state'); }//end testTheBagSeparatesADecisionFromAnEnding() + + // ---- Heartbeat recovery ------------------------------------------------------- + + /** + * 🔴 THE OTHER HALF OF THE REFUSAL TRAIL. The guarded signal seam records + * that a completion was refused; without this entry the trail ends there + * and a recovered answer reads as one that vanished. Attributed to the + * task's COMPLETER, because the fact being recorded is that person's + * answer arriving late by poll — not the cron job acting. + * + * Driven through the REAL bridge. Every other test of this behaviour mocks + * FlowTaskBridge (the nodes are the unit there), so this method's body had + * no execution coverage at all until this test. + * + * @return void + * + * @spec openspec/changes/flow-heartbeat-recovery/specs/flow-heartbeat-recovery/spec.md#requirement-a-heartbeat-recovered-delivery-is-recorded-on-the-tasks-audit + */ + public function testAHeartbeatRecoveryIsAuditedToTheTasksCompleter(): void { + $task = $this->terminalTask(); + $task->setCompletedBy('bob'); + + $seen = []; + $this->tasks->expects($this->once()) + ->method('record') + ->willReturnCallback( + function (string $uuid, string $action, ?string $actor, string $reason) use (&$seen, $task): Task { + $seen = ['uuid' => $uuid, 'action' => $action, 'actor' => $actor, 'reason' => $reason]; + + return $task; + } + ); + + $this->bridge->recordHeartbeatRecovery(task: $task); + + $this->assertSame('t-1', $seen['uuid']); + $this->assertSame('heartbeat-recovered', $seen['action']); + $this->assertSame('bob', $seen['actor'], 'the recovery is the completer\'s answer arriving, not the worker\'s'); + $this->assertStringContainsString('run-1', $seen['reason'], 'the reason names the run whose signal never arrived'); + }//end testAHeartbeatRecoveryIsAuditedToTheTasksCompleter() + + /** + * 🔴 BEST-EFFORT, AND THAT IS THE POINT. The recovery itself is the node + * applying the outcome; this entry only describes it. An audit write that + * fails must therefore NOT propagate — letting it out would abort the walk + * that was recovering the run and put the run straight back into the wedge + * this whole change exists to remove. + * + * @return void + * + * @spec openspec/changes/flow-heartbeat-recovery/specs/flow-heartbeat-recovery/spec.md#requirement-a-heartbeat-recovered-delivery-is-recorded-on-the-tasks-audit + */ + public function testAFailedRecoveryAuditIsSwallowedSoTheRecoveredRunStands(): void { + $task = $this->terminalTask(); + $task->setCompletedBy('bob'); + + $this->tasks->expects($this->once()) + ->method('record') + ->willThrowException(new RuntimeException('the audit table is unavailable')); + + $this->bridge->recordHeartbeatRecovery(task: $task); + + // Reached only because nothing propagated: the recovery outlives its + // own audit failure. + $this->addToAssertionCount(1); + }//end testAFailedRecoveryAuditIsSwallowedSoTheRecoveredRunStands() + + /** + * A task that ended WITHOUT a completer — terminated or expired rather than + * answered — still records its recovery, with no actor rather than an + * invented one. `completedBy` is null on exactly those endings, and an + * audit that guessed a name there would be worse than one that admits it + * has none. + * + * @return void + * + * @spec openspec/changes/flow-heartbeat-recovery/specs/flow-heartbeat-recovery/spec.md#requirement-a-heartbeat-recovered-delivery-is-recorded-on-the-tasks-audit + */ + public function testARecoveredEndingWithNoCompleterRecordsNoActor(): void { + $task = $this->terminalTask(); + $task->setState(Task::STATE_TERMINATED); + $task->setCompletedBy(null); + + $actor = 'unset'; + $this->tasks->expects($this->once()) + ->method('record') + ->willReturnCallback( + function (string $uuid, string $action, ?string $seenActor, string $reason) use (&$actor, $task): Task { + $actor = $seenActor; + + return $task; + } + ); + + $this->bridge->recordHeartbeatRecovery(task: $task); + + $this->assertNull($actor, 'an ending nobody answered is recorded with no actor, never a guessed one'); + }//end testARecoveredEndingWithNoCompleterRecordsNoActor() + }//end class