Reject a non-positive max_retries instead of silently uploading nothing

[feiiiiii5]

Summary

upload_points(..., max_retries=0) reports success without uploading anything. Every point in the batch is silently dropped.

Why it happens

max_retries counts attempts, not extra retries — the retry loops in both uploaders start at attempt = 0 and run while attempt < max_retries, with attempt only ever incremented inside the failure branch:

The retry loop

attempt = 0
while attempt < max_retries:
    try:
        openapi_client.points_api.upsert_points(...)
        break
    except ResourceExhaustedResponse as ex:
        ...
    except Exception as e:
        ...
        attempt += 1
return True

With max_retries <= 0 the loop body never executes and the function falls through to return True:

max_retries=0   -> returned True, upsert calls: 0
max_retries=-1  -> returned True, upsert calls: 0
max_retries=1   -> returned True, upsert calls: 1

Nothing warns, nothing raises, and the caller has no way to tell the points were dropped. upload_points and upload_collection both take max_retries: int = 3 and forward it straight through, so a retry budget computed from config — max(0, budget - used) is the natural way to write one — turns "no retries left" into "upload nothing, report success".

grpc_uploader.upload_batch_grpc has the identical loop, so both transports are affected.

What this changes

Both uploaders now reject a non-positive budget up front, matching the parameter-validation style the client already uses for limit and the MMR bounds in local/qdrant_local.py:

The guard

# `max_retries` counts attempts, not extra retries: the loop below runs
# `while attempt < max_retries` starting from zero. A non-positive value
# therefore skips the loop entirely and returns True without uploading
# anything, so the batch is silently dropped. Reject it instead.
if max_retries < 1:
    raise ValueError(f"max_retries value {max_retries} is invalid. Must be 1 or larger.")

One question for maintainers rather than a guess from me: 0 could also be read as "make one attempt, do not retry", which is a defensible reading of the name. Either way it must not mean "send nothing and claim success" — if you would prefer 0 to mean a single attempt, that is a one-line change to the bound and I am happy to make it instead.

Validation

tests/test_uploader_max_retries.py, 8 tests, no live Qdrant required:

Test output

pytest tests/test_uploader_max_retries.py
  without the guard   4 failed, 4 passed
      rest rejects 0        FAILED
      rest rejects -1       FAILED
      grpc rejects 0        FAILED
      grpc rejects -1       FAILED
  with the guard     8 passed

The four that pass either way are deliberate controls: max_retries=1 must still upload exactly once on both transports, the public default must stay 3, and the normal call path with a real PointStruct must be untouched. So the guard cannot be mistaken for having broken single-attempt uploads.

Also checked, so you do not have to: ruff format --line-length=99 --check (the only ruff hook this repo enables — the linter is commented out in .pre-commit-config.yaml) is clean on all three files; ruff check reports the same 8 pre-existing findings before and after; and tests/embed_tests has one pre-existing failure (test_bm25_core, missing qdrant_cli binary) that fails identically on master.

I did not run the integration tests, which need a Qdrant server.

---

All Submissions:

• Contributions should target the dev branch. Did you create your branch from dev? — this targets master, the default branch; dev does not exist on this repository.
• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Have you installed pre-commit with pip3 install pre-commit and set up hooks with pre-commit install?

Changes to Core Features:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully ran tests with your changes locally?

[netlify]

❌ Deploy Preview for poetic-froyo-8baba7 failed.

Name	Link
🔨 Latest commit	b8bb753
🔍 Latest deploy log	https://app.netlify.com/projects/poetic-froyo-8baba7/deploys/6ac2b649174e170008fe7c33

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: f7fdeb40-0d67-4690-8b94-38a92781ae7a

📥 Commits

Reviewing files that changed from the base of the PR and between cf747f4 and b8bb753.

📒 Files selected for processing (3)

• qdrant_client/uploader/grpc_uploader.py
• qdrant_client/uploader/rest_uploader.py
• tests/test_uploader_max_retries.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The REST and gRPC batch upload functions now raise ValueError when max_retries is below one. Tests cover invalid retry counts, one-attempt uploads, the REST default retry count, and an explicit PointStruct vector upload.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to b8bb7

The change prevents a non-positive budget from reporting a successful upload without sending data, while preserving one-attempt uploads. No actionable merge risk is established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: rejecting non-positive max_retries values instead of silently skipping uploads.
Description check	✅ Passed	The description explains the upload issue, the proposed validation, and the tests and validation performed. It is directly related to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}