Conversation
Every batch colour copies the results it collected during the run into the wrapper that `client.batch.failed_objects`, `collection.batch.failed_objects` and `batch.results` read, and every one of them does it only after the wait for the background workers has returned: `_BatchBase._shutdown()` for `dynamic()`, `fixed_size()` and `rate_limit()`, `_BatchBaseSync._wait()` and `_BatchBaseAsync._wait()` for `stream()`, which is the only mode the async client exposes. A Ctrl-C during that wait - the usual way a bulk import is stopped in a notebook, reaching an awaiting async cell as a `CancelledError` - unwinds before the copy, so every error collected so far is dropped and the accessors come back empty. The batch has already told the user to go and look at them: "Failed to send N objects in a batch of M. Please inspect client.batch.failed_objects or collection.batch.failed_objects for the failed objects." Move the copy into a `finally:` in all three places, so the results are published on the interrupted path as well as the happy one. Two details that only become visible once the errors are reachable: - The copy handed over the live containers, and on the interrupted path the background workers are still running, so the "final" error list kept growing while the user looked at it: `len()` in one notebook cell and a frame built from the same property in the next disagreed. The shared `_publish_results()` helper now publishes snapshots. `BatchObjectReturn.__add__` and `BatchReferenceReturn.__add__` mutate the left-hand side, so `results` has to be rebuilt rather than reassigned; the errors themselves are shared, not copied. - `_BatchBase._shutdown()` only asked its daemon threads to stop after `flush()` returned, so an interrupt left them uploading the batch the user had just aborted and a `BgBatchScheduler` thread survived every aborted attempt. The `finally:` now sets the shutdown event first. Objects still queued at that moment are no longer sent; a request already in flight still completes. Fixes weaviate#1300
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
|
To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge. |
This branch has not been deployed
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.
Closes #1300.
Ctrl-C during a batch shutdown throws away the error list, which is the one thing a notebook user needs after interrupting an import.
_BatchBaseSync.flush()joins the background workers and then copies what they collected onto the wrapper thatbatch.failed_objects,batch.failed_referencesandbatch.resultsread from. AKeyboardInterruptlands in the join, so the copy never runs. The objects that failed before the interrupt are sitting in the internal wrapper, unreachable, and the user sees an empty list — the same shape they would see after a clean import with no failures._BatchBaseAsync._wait()has the identical structure and the identical hole, reached by aCancelledErrorinstead.Change
The copy moves into a
finally, in both paths, so what was gathered is published whatever ended the wait.It also became a shared
_publish_results()helper rather than two copies of four assignments, because there is a second bug in the assignment version that only shows up on the interrupted path. The old code aliases the live containers. On a clean exit that is harmless — the workers are done. On an interrupted exit the workers are still running, so the user is handed an error list that keeps growing while they page through it._publish_results()snapshots instead.BatchObjectReturn.__add__andBatchReferenceReturn.__add__both mutate their left-hand side in place, so the two returns have to be rebuilt rather than reassigned; the error objects themselves are shared, not deep-copied, since nothing ever mutates one.The sync path takes the existing results lock around the publish, which the old code did not.
Tests
Six new tests in
test/collection/test_batch.py, three interrupted and three clean, across the sync shutdown, the stream wait and the async stream wait. The interrupted ones raise a realSIGINTrather than a mockedKeyboardInterrupt— the bug is about where the interrupt lands, so injecting it at a chosen line would be testing the test. A fixture installs the default SIGINT handler first, since a test runner may have replaced it.Delete-the-fix: reverting
weaviate/collections/batch/tomainwhile keeping the new tests:including
test_stream_wait_async_publishes_results_on_a_clean_exitfailing withassert 2 == 1, which is the aliasing half rather than thefinallyhalf — the published list had grown by the time it was read.