test(flow): cover the heartbeat recovery paths that no test executed - #3362
Merged
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3358, which the changed-files coverage ratchet flagged: "coverage of the code this change KEEPS or ADDS dropped by 2.15% — this change adds 32 statements to those files."
The ratchet was right, and it was pointing at something real rather than at a percentage.
What was actually uncovered
Of the 32 statements #3358 added, 30 were the body of
FlowTaskBridge::recordHeartbeatRecovery()— and every test that exercised the recovery mockedFlowTaskBridge, because in those tests the nodes are the unit under test. So the method that writes the recovery's audit trail had no execution coverage at all: the audit entry, its attribution, and thecatchthat makes it best-effort were asserted nowhere.That matters more than the percentage. The guarded signal seam records a refusal; this entry is the other half of that trail. A silent regression in it would make a recovered answer read as one that vanished — which is precisely the diagnostic gap that made the original wedge hard to see.
What this adds
Three tests through the real bridge, in
FlowTaskBridgeTest, which already constructs one:heartbeat-recovered, attributed to the task'scompletedBy, with a reason naming the run whose signal never arrived;completedByis null) records no actor rather than a guessed one.And one test for the symmetric case #3358 documented but left unpinned — a completion that raced the suspension.
signal()refuses a run that is notsuspended, 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 entire basis for deciding the race needs no new mechanism, and it is now falsifiable rather than merely claimed.Evidence these tests can fail
Each was checked by mutation, since a test that cannot fail is worse than no test:
heartbeat-recovered'cron'catch (Throwable)narrowed so the audit error escapesUserTaskNodelib/is untouched by this PR — tests only.Local verification: full unit suite 19,097 green; PHPCS, Psalm (no errors), PHPStan (no errors) and PHPMD (both rulesets, per subdir) clean; hydra-gates v1.11.0
--scope-to-diffagainstorigin/development.🤖 Generated with Claude Code