diff --git a/tests/Unit/Service/Flow/FlowHeartbeatRecoveryTest.php b/tests/Unit/Service/Flow/FlowHeartbeatRecoveryTest.php index 17ed44d8a..fc0f80b6b 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 fda18e879..a94003b97 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