[Python] Fix wait_for_offset regression; release 1.6.1 - #728
Merged
Conversation
The PyO3 0.20 -> 0.29 upgrade (#652) dropped the implicit `None` default for `Option<T>` arguments, so `timeout_sec` silently became required on `ZerobusStream.wait_for_offset`. The SDK's own wrapper calls it as `wait_for_offset(offset)`, so it raised `TypeError` on every call through the public API on 1.5.0 and 1.6.0 (issue #726). Restore the optional defaults with `#[pyo3(signature = ...)]` on the four affected sync bindings: `wait_for_offset`, `RecordAcknowledgment.wait_for_ack`, `ZerobusSdk.create_stream`, and `create_stream_with_headers_provider`. The async surface already carried the attribute and was unaffected. Add `tests/test_signatures.py` locking down the optional defaults so this class of regression is caught without network or credentials — no example exercised the sync record-stream `wait_for_offset` path, which is why it shipped unnoticed. Release v1.6.1: bump version, move NEXT_CHANGELOG into CHANGELOG. Co-authored-by: Isaac Signed-off-by: elenagaljak-db <elena.galjak@databricks.com>
Co-authored-by: Isaac Signed-off-by: elenagaljak-db <elena.galjak@databricks.com>
elenagaljak-db
force-pushed
the
effort/zerobus-sdk
branch
from
August 12, 2026 14:04
80bb92c to
7b9e3c5
Compare
| impl RecordAcknowledgment { | ||
| /// Wait for the acknowledgment and return the offset ID. | ||
| /// This method can only be called once. | ||
| #[pyo3(signature = (_timeout_sec = None))] |
Collaborator
There was a problem hiding this comment.
Here we expose _timeout_sec, but the stub and example document timeout_sec. Thus ack.wait_for_ack(timeout_sec=30) still raises TypeError. The new test incorrectly locks in _timeout_sec at python/tests/test_signatures.py:36. Rename the Rust parameter and test the documented keyword.
Collaborator
There was a problem hiding this comment.
As discussed offline, this would be a breaking change.
teodordelibasic-db
approved these changes
Aug 12, 2026
| impl RecordAcknowledgment { | ||
| /// Wait for the acknowledgment and return the offset ID. | ||
| /// This method can only be called once. | ||
| #[pyo3(signature = (_timeout_sec = None))] |
Collaborator
There was a problem hiding this comment.
As discussed offline, this would be a breaking change.
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.
Summary
Fixes #726 —
ZerobusStream.wait_for_offset(offset)raisedTypeError: missing 1 required positional argument: 'timeout_sec'on every call through the public sync API on 1.5.0 and 1.6.0.Root cause
The PyO3
0.20 → 0.29upgrade (#652) dropped the rule where anOption<T>argument implicitly defaults toNone. Without an explicit#[pyo3(signature = ...)],timeout_secsilently became a required argument in the generated Python signature — with no change to any source line:(offset, timeout_sec=None)(offset, timeout_sec)The SDK's own wrapper calls
self._inner.wait_for_offset(offset), so it could never satisfy the second signature.Fix
Restore the optional defaults with
#[pyo3(signature = ...)]on the four affected sync bindings:ZerobusStream.wait_for_offset(the crash)RecordAcknowledgment.wait_for_ackZerobusSdk.create_streamZerobusSdk.create_stream_with_headers_providerThe async surface already carried the attribute and was unaffected. The Rust bodies are untouched (
wait_for_offsetalready discardstimeout_sec), so this is a pure signature restoration back to 1.4.0 behavior — additive, non-breaking.Why it shipped unnoticed
No example exercised the broken combination (sync record stream + explicit
wait_for_offset). The JSON/proto examples useflush(); the Arrow examples callwait_for_offseton the Arrow stream, whose binding never had atimeout_secargument.tests/test_signatures.pycloses that coverage gap — 8 tests asserting the optional defaults survive, needing neither network nor credentials. Verified they fail on the unfixed binding and pass with the fix.Release v1.6.1
Patch on top of the current released line (1.6.0). Bumped the version in
rust/Cargo.toml,Cargo.lock, andzerobus/__init__.py; movedNEXT_CHANGELOG.mdintoCHANGELOG.mdand reset the template.Test plan
python -m unittest tests.test_signatures tests.test_smoke→ 30 passing.TypeError; the 3 async tests stay green.This pull request and its description were written by Isaac.