Skip to content

[Rust] Multiplexed stream ingest followups#426

Open
danilonajkov-db wants to merge 6 commits into
mainfrom
issue-423-mux-followups
Open

[Rust] Multiplexed stream ingest followups#426
danilonajkov-db wants to merge 6 commits into
mainfrom
issue-423-mux-followups

Conversation

@danilonajkov-db

@danilonajkov-db danilonajkov-db commented Jun 25, 2026

Copy link
Copy Markdown
Member

What changes are proposed in this pull request?

This fixes the MultiplexedStream poison/admission race and makes capacity waits bounded.

The new ingest flow is:

  1. Reserve real landing-zone capacity on the selected sub-stream.
  2. Enter the sub-stream's private ingest-ordering section.
  3. Acquire the mux admission read lock and re-check mux state.
  4. Re-check sub-stream state.
  5. Allocate the offset and enqueue using the reserved capacity.

The poison path acquires the mux admission write lock before flushing sub-streams, so no record can be admitted between the final closed check and the poison flush.

ZerobusStream retains ownership of its internal ordering lock through enqueue_reserved_admitted; the mux supplies only the admission check. Capacity reservations are opaque and cannot expose the underlying semaphore implementation.

Capacity waits are bounded by the existing flush_timeout_ms option. If acknowledgments never free capacity, ingest_record / ingest_records return ZerobusError::ConnectionTimeout instead of waiting indefinitely. A capacity timeout does not poison an otherwise healthy mux.

How is this tested?

  • Regression test for concurrent ingests waiting for capacity while a sub-stream failure poisons the mux.
  • Regression test for a permanently full sub-stream returning ConnectionTimeout after the configured timeout.
  • Full multiplexed stream integration suite: 23 tests passed.
  • Rust SDK unit suite: 141 tests passed.
  • Doc tests passed.
  • cargo fmt --all --check
  • cargo clippy -p databricks-zerobus-ingest-sdk --all-targets -- -D warnings

@danilonajkov-db danilonajkov-db changed the title [Rust] Fix mux ingest race [Rust] Add reserveable capacity in landing-zone Jun 25, 2026
@danilonajkov-db danilonajkov-db linked an issue Jun 25, 2026 that may be closed by this pull request
@danilonajkov-db danilonajkov-db self-assigned this Jun 25, 2026
@danilonajkov-db danilonajkov-db changed the title [Rust] Add reserveable capacity in landing-zone [Rust][Mux follow-ups] Add reserveable capacity in landing-zone and ack callback Jun 29, 2026
@danilonajkov-db danilonajkov-db changed the title [Rust][Mux follow-ups] Add reserveable capacity in landing-zone and ack callback [Rust][Multiplexing follow-ups] Add reserveable capacity in landing-zone and ack callback Jun 29, 2026
Comment thread rust/NEXT_CHANGELOG.md Outdated
Comment thread rust/sdk/src/landing_zone.rs Outdated
Comment thread rust/sdk/src/multiplexed_stream.rs
Comment thread rust/sdk/src/multiplexed_stream.rs Outdated
Comment thread rust/sdk/src/callbacks.rs Outdated
Comment thread rust/sdk/src/lib.rs Outdated
@danilonajkov-db

danilonajkov-db commented Jun 30, 2026

Copy link
Copy Markdown
Member Author

@elenagaljak-db thanks for the review, i've decided to move the ack callback to another PR to not mix contexts here. I've pasted your comments regarding the ack callback on that PR and resolved the ones here

@danilonajkov-db danilonajkov-db changed the title [Rust][Multiplexing follow-ups] Add reserveable capacity in landing-zone and ack callback [Rust][Multiplexing follow-ups] Add reserveable capacity in landing-zone Jun 30, 2026
@danilonajkov-db danilonajkov-db marked this pull request as draft July 3, 2026 10:14
@danilonajkov-db danilonajkov-db changed the title [Rust][Multiplexing follow-ups] Add reserveable capacity in landing-zone [Rust] Fix multiplexed stream ingest admission Jul 13, 2026
@danilonajkov-db danilonajkov-db changed the title [Rust] Fix multiplexed stream ingest admission [Rust] Multiplexed stream ingest followups Jul 13, 2026
@danilonajkov-db danilonajkov-db force-pushed the issue-423-mux-followups branch 2 times, most recently from 4ead0b6 to e3f3f1e Compare July 13, 2026 12:47
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
@danilonajkov-db danilonajkov-db force-pushed the issue-423-mux-followups branch from e3f3f1e to dae8ddf Compare July 13, 2026 12:48
@danilonajkov-db danilonajkov-db marked this pull request as ready for review July 13, 2026 13:54
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
Comment thread rust/sdk/src/multiplexed_stream.rs Outdated
"MultiplexedStream poisoned due to sub-stream failure"
);

let _admission = self.admission.write().await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we discussed this before.

Since we hold this lock across join_all(...flush()), an ingest already parked at admission.read().await in enqueue_reserved_admitted cannot acquire the read lock until the whole flush finishes, so it stays blocked for that window before it sees is_closed and errors, and because it also holds sync_mutex, other ingests on that sub-stream queue behind it.

The write lock maybe only needs to be held long enough to drain readers that are mid-enqueue, not for the whole flush. Could we take and immediately drop it as a barrier after setting is_closed, then run the flush and signal_shutdown() unlocked? Any reader that acquires the read lock afterward fails check_closed() anyway, so nothing new is admitted.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense to not block ingests if stream is mid close/failure and return error. Fixed

Comment thread rust/sdk/src/stream/grpc/ingest.rs Outdated

#[cfg(feature = "testing")]
pub(crate) fn capacity_wait_timeout(&self) -> std::time::Duration {
std::time::Duration::from_millis(self.options.flush_timeout_ms)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would make sense to have a separate knob for this timeout other than the flush one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a seperate timeout variable, but didn't make it configureable for the clients. My thinking is that in the future we want the mux stream automatically try the next stream if the first one is full, so it will become an implementation detail the customer doesn't need to worry about. WDYT?

Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
@danilonajkov-db

Copy link
Copy Markdown
Member Author

ill wait for #523 to be merged so I can fix the tests

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] MultiplexedStream follow-ups

3 participants