Skip to content

fix(bulk_writer): propagate sync commit failures - #3780

Closed
SiluPanda wants to merge 1 commit into
milvus-io:masterfrom
SiluPanda:fix/bulk-writer-propagate-commit-errors
Closed

fix(bulk_writer): propagate sync commit failures#3780
SiluPanda wants to merge 1 commit into
milvus-io:masterfrom
SiluPanda:fix/bulk-writer-propagate-commit-errors

Conversation

@SiluPanda

Copy link
Copy Markdown

Summary

Fixes #2314.

This change makes synchronous bulk-writer commits propagate failures from the background flush thread instead of only logging them and returning normally.

The current implementation starts _flush() on a thread, joins it for the default synchronous commit(), and then resets the buffer state. If persist() or a remote upload callback fails, the exception stays inside the worker thread, so callers see a successful return and may continue with an empty batch_files result.

The patch records the first flush exception and re-raises it on the caller thread:

  • after waiting for any previous flush to finish, before starting a new one
  • after joining the synchronous flush thread

This keeps the existing asynchronous flush model, but makes the default synchronous LocalBulkWriter.commit() and RemoteBulkWriter.commit() fail loudly and become catchable by user try/except blocks.

This also covers the same root cause reported in #2628 for RemoteBulkWriter.commit().

Testing

  • pytest -q tests/unit/test_local_bulk_writer.py tests/unit/test_remote_bulk_writer.py
  • make lint
  • manual reproduction with patched failures now raises on the caller thread for both local persist failures and remote upload failures

@sre-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SiluPanda
To complete the pull request process, please assign longjiquan after the PR has been reviewed.
You can assign the PR to them by writing /assign @longjiquan in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot

Copy link
Copy Markdown

Welcome @SiluPanda! It looks like this is your first PR to milvus-io/pymilvus 🎉

@mergify

mergify Bot commented Aug 30, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@SiluPanda

Copy link
Copy Markdown
Author

Closing this for now: this repository enforces DCO sign-off, and I cannot provide or maintain that legal attestation for this contribution. The patch remains available on the branch for maintainers or a human contributor to reuse if helpful.

@SiluPanda
SiluPanda force-pushed the fix/bulk-writer-propagate-commit-errors branch from 8758421 to 9cf4df2 Compare August 30, 2026 08:35
@SiluPanda SiluPanda closed this Aug 30, 2026
@mergify mergify Bot added needs-dco and removed dco-passed labels Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: When the bulkwriter fails to generate a file, it returns an empty list instead of throwing an error.

2 participants