Skip to content

[Rust] Reject server over-acks - #648

Draft
Pybsama wants to merge 1 commit into
databricks:mainfrom
Pybsama:codex/reject-server-over-acks
Draft

[Rust] Reject server over-acks#648
Pybsama wants to merge 1 commit into
databricks:mainfrom
Pybsama:codex/reject-server-over-acks

Conversation

@Pybsama

@Pybsama Pybsama commented Jul 31, 2026

Copy link
Copy Markdown

What changes are proposed in this pull request?

Fixes #638.

This change makes the Rust gRPC receiver reject malformed cumulative durability acknowledgements before they mutate client state. It:

  • tracks the highest physical offset handed to each active gRPC connection;
  • synchronizes outbound channel handoff and sent-watermark publication, so an observed request blocked on channel capacity is not considered sent;
  • rejects negative and beyond-sent ACK offsets before removing records or completing oneshots/callbacks;
  • treats duplicate and regressive cumulative ACKs as no-ops, preserving a monotonic watermark; and
  • retains an explicit invariant failure if an otherwise valid ACK cannot be applied to the observed queue.

A landing-zone observed count is not a safe upper bound because observe() happens before the bounded outbound channel handoff. A connection-local sent watermark closes that concurrency window and is reset together with the physical offset generator on recovery.

How is this tested?

  • cargo test -p databricks-zerobus-ingest-sdk (162 unit tests plus 24 passing doc tests; 9 doc tests ignored by existing annotations)
  • cargo test -p tests --test rust_tests (93 integration tests)
  • deterministic capacity-one sender test proving an observed request blocked in reserve() does not advance the sent watermark
  • mock gRPC regressions for a single record and a five-record batch, both receiving ACK 1 after only physical offset 0 was sent
  • receiver regressions for negative, duplicate, regressive, valid, and beyond-sent ACKs, including the 1 -> 0 -> 2 sequence
  • cargo clippy -p databricks-zerobus-ingest-sdk --all-targets -- -D warnings
  • make check
  • git diff --check

Track the highest physical offset handed to each gRPC connection and reject malformed cumulative acknowledgements before they can advance durability state. Preserve pending single records and batches on protocol violations.

Fixes databricks#638

Signed-off-by: sama Pyb <peiyibopybsama@gmail.com>
@Pybsama

Pybsama commented Aug 29, 2026

Copy link
Copy Markdown
Author

Revalidated this change against current main (122572a3598d0172dadfe4d90680ef4602a7c141, 56 commits after the public base). The public branch remains unchanged at the original Verified + DCO head 2ef51f9bd864ae8b35c72a556b7daa8e85ac04bd and is currently conflicting.

Current-main evidence:

  • The four production files have the same stable patch-id as the public commit: 0ad60c65b58976e976dcb6674fbd73714b32d547.
  • The two over-ack integration tests fail on unmodified current main (0 passed / 2 failed) and pass after the port (2/2).
  • make check passes (workspace fmt + Clippy with -D warnings).
  • SDK tests pass: 181 unit tests; 24 doc tests pass and 9 remain ignored.
  • The main Rust integration suite passes: 93/93.
  • cargo test --workspace -- --test-threads=1 passes across the workspace.

One current-main OAuth timing test, hung_refresh_falls_back_to_cached_token, fails when the 20-test OAuth binary runs in parallel on both an unmodified main worktree and the patched worktree (19/20), while the same test passes alone on both sides. The serialized workspace run passes, and this change does not touch the OAuth files.

I also tested composition with adjacent PR #792 (a5b6b332499086fa25c2b8734dfaf50fce6d9ae2). The combined tree passes make check, both over-ack tests, the duplicate-ACK timeout test from #792, and the full combined Rust integration suite (94/94). Only a successfully applied forward ACK resets the #792 progress deadline; duplicate/regressive ACKs remain no-ops and invalid ACKs terminate before state mutation.

An independent concurrency review found 0 Critical and 0 Important issues and gave GO. The conflict-resolved tree is staged locally only; I have not pushed a new commit or made a new DCO/signature attestation, so this PR remains Draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Rust] Receiver silently absorbs server over-ack and can wedge the stream

1 participant