test(sv2): make the aggregated fallback test fail instead of hang (#849) - #855
Open
defenwycke wants to merge 2 commits into
Open
test(sv2): make the aggregated fallback test fail instead of hang (#849)#855defenwycke wants to merge 2 commits into
defenwycke wants to merge 2 commits into
Conversation
WIP — the method and task registry only; nothing calls it yet. A sniffer's proxy task runs until the PROCESS exits, and it retries `TcpStream::connect` to its upstream once a second for ever. Every `tests/*.rs` is one binary running many tests, so a sniffer left behind by a finished test keeps looping against an upstream that has gone, and they accumulate across a file. That is a candidate cause for #849 item 3 (`translator_aggregated_integration`: four tests pass with `--exact`, hang when run together). `PoolSv2`, `TranslatorSv2` and the JD roles all have a `shutdown()`; the sniffers did not, which is why `shutdown_all!` never covered them. ⚠ Not yet verified against the hang — the hypothesis is untested. Claude-Session: https://claude.ai/code/session_01XjzQeoCkuKx3vb4amkzAdT
`aggregated_translator_triggers_fallback_on_close_channel_message` does not fail when it goes wrong — it hangs, for ever, and takes the whole binary with it. Measured with `--exact` on an idle box: 5 runs out of 5 killed at a 150s cap, no panic, no log line from the test process after the first 10 seconds. This corrects the record in `ci.yml` and #849, which both say the four aggregated tests "pass INDIVIDUALLY" and hang only when run together. They do not. This one hangs alone, and no predecessor is involved: t1-then-t3 hung, t3-alone hung, and a passing run happens roughly 1 time in 3 either way. `Sniffer::wait_for_message_type` already carries a 60s deadline meant to turn exactly this into a failure, and #450 moved its queue read onto the blocking pool so the read could not stall the executor. That is necessary and not sufficient. `#[tokio::test]` is a current-thread runtime and the queue read is only one of the things on it that takes a blocking lock — the sniffer's own forwarding task (`add_message` -> `safe_lock`), the translator and the pool all take one inline. tokio's timer lives on that same single thread, so once any of them blocks it, `timeout` and `sleep` stop advancing and the deadline can never arrive. The guard is unreachable precisely when it is needed. Giving this test four worker threads makes the deadline reachable again: 0 hangs in 4 runs, two of which failed on the 60s deadline with its own message, two of which passed. ⚠ The surviving failure is REAL and is not fixed here: after `CloseChannel` the translator often never opens the fallback upstream, logging `Failed to send fallback status from ChannelManager`. It reproduces roughly 2 runs in 3 and was invisible for as long as the target hung rather than failed. Filed separately. This target still must not gate until that is fixed and proven green on CI. Also finishes the sniffer shutdown from the previous commit. `shutdown()` is now async so `shutdown_all!` can take it — that macro expands to `tokio::join!` over each handle's `shutdown()`, so a sync method could not be used by the one thing meant to call it — and every sniffer in this file is now shut down with its pool and translator rather than left running to the end of the process.
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
1 participant
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.
Closes nothing on its own — #849 stays open, and this adds #854.
aggregated_translator_triggers_fallback_on_close_channel_messagedoes not fail when it goeswrong. It hangs, for ever, and takes the whole target with it: 5 runs of 5 killed at a 150s cap,
no panic, no log line from the test process after the first 10 seconds.
The record was wrong
ci.ymland #849 both say the four aggregated tests "pass INDIVIDUALLY" and hang only when runtogether, i.e. an isolation failure. They do not. This one hangs alone with
--exact, and repeatsput it at roughly one pass in three regardless of what ran before it. The comment in
ci.ymliscorrected here.
Why the existing guard did not fire
Sniffer::wait_for_message_typecarries a 60s deadline for exactly this case, and #450 moved itsqueue read onto the blocking pool so the read could not stall the executor. That is necessary and
not sufficient.
#[tokio::test]is a current-thread runtime, and the queue read is only one of thethings on it that takes a blocking lock — the sniffer's own forwarding task (
add_message→safe_lock), the translator and the pool all take one inline. tokio's timer lives on that samesingle thread, so once any of them blocks it
timeoutandsleepstop advancing and the deadlinecan never arrive. The guard is unreachable precisely when it is needed.
Four worker threads make it reachable again: 0 hangs in 4 runs — two failed on the 60s deadline
with its own message, two passed.
What this does not do
⚠ It does not make the target gate-able, and it does not fix the underlying bug. After
CloseChannelthe translator often never opens the fallback upstream (Failed to send fallback status from ChannelManager), about 2 runs in 3 — filed as #854, same message as #812 on theadjacent path, still present on the v1.11.38 tree. This PR only converts a 420s timeout with an
empty log into a 60s failure with a message, so that bug is visible at all.
translator_aggregated_integrationstays out of the blockingsv2-integrationset until #854lands and the target is proven green on CI, not locally.
Also
Finishes the sniffer shutdown.
Sniffer::shutdown()is nowasync, becauseshutdown_all!expands to
tokio::join!over each handle'sshutdown()and could not have taken a sync method —the one thing meant to call it. Every sniffer in this file is now shut down alongside its pool and
translator instead of running to process exit.
Gates
cargo fmt --all -- --checkandcargo clippy -p integration_tests_sv2 --tests -- -D warningsboth clean. Test evidence is per-run counts above, all on an idle box with each run in its own
process group.