[adapters] Register the S3 test's error collector before the reader - #6785
Merged
Conversation
`transport::s3::test::list_object_read_error` could block forever, holding the whole test binary and every job waiting on it. A merge queue run sat for an hour in this test with no output and no failure. `S3InputReader::new_inner` starts listing objects as soon as it is called, so the mocked `NoSuchBucket` error can arrive before the test swaps in its own callback. It then lands in the no-op callback `test_setup` installs and is dropped, and the test's `rx.recv()`, which had no timeout, waits for a message nobody will send. `test_setup` now takes the error callback and installs it before constructing the reader, so no window exists in which errors go to a callback the caller does not want. Bounding the wait as well turns a future regression into a failure after a minute rather than a hang. Bound the adapter job with `timeout-minutes: 30`. The suite finishes in 3 to 10 minutes, so without a bound a hang holds a runner for the 6 hour default. The cancel-if-* sentinels cannot cover this: a job that hangs never reports failure. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
gz
enabled auto-merge
August 1, 2026 02:02
lalithsuresh
approved these changes
Aug 1, 2026
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
2 participants
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.
transport::s3::test::list_object_read_errorcould block forever, holding the whole test binary and every job waiting on it. A merge queue run sat for an hour in this test with no output and no failure: https://github.com/feldera/feldera/actions/runs/30675189907/job/91302225039What happens
s3.rstest_setupon_errorcallbacks3.rs:399new_inners3-input-tokio-wrapperthread right aways3.rs:455worker_taskget_object_keysat once, without waiting for a commandon_errorwith its channel senderrx.recv(), no timeoutThe mock returns
NoSuchBucketon the firstget_object_keys. When step 3 beats step 4, the error reaches the no-op callback from step 1 and is dropped. Nothing ever sends on the channel, so step 5 waits forever.reader.extend()does not gate this: the listing task loops independently of the command channel.gdb against the live runner showed the harness parked in
recv()onCompletedTestwaiting for this one test, which was itself parked inmpsc::Receiver::recv().Fix
test_setuptakes the error callback and installs it before constructing the reader, so no window exists in which errors go to a callback the caller does not want. The wait is bounded as well, which turns a future regression into a failure after a minute rather than a hang.Bounding the adapter job with
timeout-minutes: 30covers the general case. The suite finishes in 3 to 10 minutes, so without a bound a hang holds a runner for the 6 hour default. Thecancel-if-*sentinel scheme cannot cover this, because a job that hangs never reports failure.Validation
Restoring the old ordering (register the collector after
test_setup, plus a sleep to make the race deterministic) reproduces the defect against the bounded wait:With the fix, all 6
transport::s3tests pass.