[Service Bus] Send server-timeout on management operations - #48563
[Service Bus] Send server-timeout on management operations#48563Pranjal Patel (pranz1996) wants to merge 4 commits into
Conversation
Management operations (peek, deferred receive, lock renewal, session state, session listing, schedule/cancel scheduled) now send the `com.microsoft:server-timeout` application property, asking the service to bound the operation on its side. The value is the caller's remaining time less a one second buffer, so the service answers before the client gives up, or 60 seconds when the caller supplied no timeout. Previously no bound was sent at all, so a stalled service could hold a management call until the AMQP link itself failed. The property is set in the base handler, after the retry loop computes the remaining time, so it reflects the time actually left on the attempt. Both transports pass application properties through unchanged, so no transport change is needed. REQUEST_RESPONSE_TIMEOUT already held the correct vendor-prefixed key and was previously unused. Matches the .NET, Java and Go SDKs (.NET ServiceBusRetryOptions.TryTimeout, Java MessageUtils.adjustServerTimeout, Go defaultServerTimeout). Refs: AB#38822503
|
Azure Pipelines: Successfully started running 1 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds service-side timeout bounds to Service Bus management operations.
Changes:
- Calculates buffered server timeouts with a 60-second default.
- Adds the timeout property to synchronous and asynchronous requests.
- Adds unit coverage and release notes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
_base_handler.py |
Adds timeout to sync requests. |
aio/_base_handler_async.py |
Adds timeout to async requests. |
_common/constants.py |
Defines timeout constants. |
_common/utils.py |
Calculates timeout milliseconds. |
test_server_timeout.py |
Tests calculation and request propagation. |
CHANGELOG.md |
Documents the behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Address review feedback on the management server-timeout change. The public management APIs bound `timeout` only at zero, so a caller could pass a value whose millisecond form exceeds a uint32 (about 49.7 days). The AMQP encoder would then raise while packing the message rather than performing the operation. Clamp before scaling as well as after, so a very large or infinite float cannot overflow the int conversion either. Also document message settlement over the management link as affected. It reaches the service through the common handler when a message cannot be settled on the receive link, for example after the lock has expired or for a deferred message, and passes no timeout, so it takes the 60 second default. Refs: AB#38822503
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
sdk/servicebus/azure-servicebus/azure/servicebus/_common/utils.py:110
- The upper clamp is applied before subtracting the buffer, so any timeout above the AMQP range is encoded as
MAX_SERVER_TIMEOUT_MS - 1000, notMAX_SERVER_TIMEOUT_MSas the documented “remaining time minus buffer, then clamp” behavior requires. Compute the buffered milliseconds first and then clamp; the cap test should also assert equality to the maximum so this boundary is covered.
capped = min(timeout, MAX_SERVER_TIMEOUT_MS / 1000)
remaining_ms = int(capped * 1000) - SERVER_TIMEOUT_BUFFER_MS
return min(max(remaining_ms, 0), MAX_SERVER_TIMEOUT_MS)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
sdk/servicebus/azure-servicebus/azure/servicebus/_common/utils.py:109
- The cap is applied before subtracting the buffer, so it starts truncating one second too early. For example,
timeout=4_294_968should produce4_294_967_000ms (remaining time minus one second, still below the uint maximum), but this returns4_294_966_295. Cap the millisecond value atMAX + bufferbefore subtracting so the documented formula holds across the upper boundary.
capped = min(timeout, MAX_SERVER_TIMEOUT_MS / 1000)
remaining_ms = int(capped * 1000) - SERVER_TIMEOUT_BUFFER_MS
Johnathan W (j7nw4r)
left a comment
Johnathan W (j7nw4r)
left a comment
There was a problem hiding this comment.
The design is right. _do_retryable_operation recomputes the remaining time before each attempt, so _mgmt_request_response is the correct place to set this. The key matches Java, and the millisecond table matches Go.
Two asks. Nothing tests the response half. Every test asserts what the client sends, but the changelog promises a retryable OperationTimeoutError, and that rests on the service returning com.microsoft:timeout in errorCondition. Pin it without a live namespace: push a fake non-200 response with that condition through mgmt_handlers.default and assert the type.
Second, the uamqp path is untested and is the one place the value type changes. Under pyamqp it is a plain dict; under uamqp a uamqp.types.AMQPuInt. AMQP_UINT_VALUE has never gone into application_properties before. One test that stubs the uamqp type closes it.
Worth a line in the description too: a settle over the management link that times out at 60 seconds now retries, and the retry can raise MessageLockLostError where the call used to block.
| parserVersion: 0.3.31 | ||
| pythonVersion: 3.13.15 |
There was a problem hiding this comment.
apiMdSha256 is unchanged, so no API moved. Only the parser and interpreter versions shift, and the new pythonVersion is your local one. Please drop this commit.
There was a problem hiding this comment.
I kept parserVersion change -> without that API.md consistency check fails
|
|
||
| - Added `ServiceBusReceivedMessage.from_bytes()` classmethod to construct a `ServiceBusReceivedMessage` from raw AMQP payload bytes without requiring the deprecated `uamqp` library. ([#43979](https://github.com/Azure/azure-sdk-for-python/issues/43979)) | ||
| - Added `ServiceBusClient.list_queue_sessions()` and `ServiceBusClient.list_subscription_sessions()` (sync and async) to list session IDs for entities with active messages, with optional filtering by session-state update timestamp. The methods return an `ItemPaged[str]` (`AsyncItemPaged[str]` on the async client) so callers can iterate every session transparently or page with `by_page()`. Implements the `com.microsoft:get-message-sessions` management operation. ([#46575](https://github.com/Azure/azure-sdk-for-python/pull/46575)) | ||
| - Management operations (peek, deferred receive, message settlement over the management link, lock renewal, session state, session listing, schedule/cancel) now send `com.microsoft:server-timeout`: the caller's remaining time less one second, or 60 seconds when none was given. Previously no bound was sent, so a stalled service held the call until the AMQP link failed; it now raises a retryable `OperationTimeoutError`, so a persistently stalled service surfaces after roughly four minutes at default retry settings. Matches the .NET, Java and Go SDKs. |
There was a problem hiding this comment.
This changes existing operations, so it is not a new feature. Move it to ### Other Changes.
There was a problem hiding this comment.
Thanks! Updated that.
| return DEFAULT_SERVER_TIMEOUT_MS | ||
| capped = min(timeout, MAX_SERVER_TIMEOUT_MS / 1000) | ||
| remaining_ms = int(capped * 1000) - SERVER_TIMEOUT_BUFFER_MS | ||
| return min(max(remaining_ms, 0), MAX_SERVER_TIMEOUT_MS) |
There was a problem hiding this comment.
The outer min never binds, since capped is already bounded. return max(int(capped * 1000) - SERVER_TIMEOUT_BUFFER_MS, 0) says the same thing.
There was a problem hiding this comment.
Fixed it properly. Please review that part again.
Address review feedback. Nothing tested the response half: every assertion covered what the client sends, while the changelog promises a retryable OperationTimeoutError that rests on the service returning com.microsoft:timeout in errorCondition. Push a fake non-200 response with that condition through mgmt_handlers.default and assert the type, for both transports, with no live namespace. uamqp is the one place the value type changes, since pyamqp uses a plain dict where uamqp uses an AMQPuInt that has not previously gone into application_properties. One test stubs the transport type to prove the property is built from the transport's own AMQP_UINT_VALUE, and one encodes a real message when uamqp is installed. Also drop the redundant outer min in get_server_timeout_ms, which never binds because the buffer is subtracted after the cap, and move the changelog entry to Other Changes since it changes existing operations rather than adding a feature. api.metadata.yml carries the parserVersion bump only. That field is gated by the consistency check and the package still had 0.3.30 while the check generates 0.3.31. apiMdSha256 and pythonVersion are unchanged. Refs: AB#38822503
f2bb172 to
12a85bc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
sdk/servicebus/azure-servicebus/azure/servicebus/_common/utils.py:109
- The upper clamp is applied before subtracting the one-second buffer, so large/infinite timeouts can never produce
MAX_SERVER_TIMEOUT_MS; they top out atMAX_SERVER_TIMEOUT_MS - 1000. This does not implement “remaining time minus the buffer, clamped to the uint range” and leaves the top second of the valid uint range unreachable. Subtract first, then clamp the millisecond result.
capped = min(timeout, MAX_SERVER_TIMEOUT_MS / 1000)
return max(int(capped * 1000) - SERVER_TIMEOUT_BUFFER_MS, 0)
[Pilot] PR Pipeline Failure AnalysisA CI pipeline failed on this pull request. Here is an automated analysis of what went wrong and how to get the build green. What failedThe
This is a test failure. The test Recommended next steps
Raw pipeline analysis (azsdk ci analyze)
|
…sport The test skipped on azure.servicebus._transport._uamqp_transport, but that module imports cleanly without uamqp: UamqpTransport is defined inside the try block that imports it, so the module is present while the class is not. importorskip therefore succeeded and the attribute access raised AttributeError on every CI leg, which do not install uamqp. Skip on uamqp itself and import the class afterwards. Verified both ways: with uamqp installed the test runs, and with uamqp unavailable it skips. Refs: AB#38822503
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
sdk/servicebus/azure-servicebus/azure/servicebus/_common/utils.py:109
- The upper clamp is applied before subtracting the buffer, so an over-limit caller timeout never produces the documented AMQP uint maximum. For example, 5,000,000 seconds yields 4,294,966,295 rather than clamping the buffered value to 4,294,967,295. Clamp after accounting for the buffer (and strengthen the test to assert equality at the cap).
capped = min(timeout, MAX_SERVER_TIMEOUT_MS / 1000)
return max(int(capped * 1000) - SERVER_TIMEOUT_BUFFER_MS, 0)
sdk/servicebus/azure-servicebus/CHANGELOG.md:25
- This overstates the guarantee:
server-timeoutis only a broker-side request. When the caller omitstimeout, both client transports still have no local expiry, so a broker that goes silent or ignores the property can still block indefinitely. Qualify the four-minute outcome as contingent on the service returningcom.microsoft:timeout, and retain guidance to passtimeoutwhen a hard client-side bound is required.
- Management operations (peek, deferred receive, message settlement over the management link, lock renewal, session state, session listing, schedule/cancel) now send `com.microsoft:server-timeout`: the caller's remaining time less one second, or 60 seconds when none was given. Previously no bound was sent, so a stalled service held the call until the AMQP link failed; it now raises a retryable `OperationTimeoutError`, so a persistently stalled service surfaces after roughly four minutes at default retry settings. Matches the .NET, Java and Go SDKs.
Description
Management operations previously sent no timeout bound to the service at all, so a stalled service could hold a management call until the AMQP link itself failed.
They now send the
com.microsoft:server-timeoutapplication property, asking the service to bound the operation on its side. This matches the .NET, Java and Go SDKs.Ported from the merged Go change Azure/azure-sdk-for-go#26499.
Behavior
DEFAULT_SERVER_TIMEOUT_MS)The 1 s buffer means the service answers before the client gives up, so the caller gets a service-side answer rather than an ambiguous local timeout. Java subtracts the same second in
MessageUtils.adjustServerTimeout. Values match Go's test table exactly: no deadline → 60000, 10 s → 9000, 60 s → 59000, ≤ 1 s → 0.The value is clamped at both ends because it is encoded as an AMQP unsigned int: a negative would underflow to a huge positive and invert the intent, and the public APIs bound
timeoutonly at zero, so a sufficiently large value would exceed2**32 - 1and make the encoder raise while packing the message instead of performing the operation.Affected operations
Management path only, sync and async:
peek_messages()session.get_state()schedule_messages()receive_deferred_messages()session.set_state()cancel_scheduled_messages()renew_message_lock()session.renew_lock()list_sessions()Settlement (
complete/abandon/defer/dead_letter) reaches the service over the management link when it cannot be settled on the receive link — for example after the lock has expired, or for a deferred message. That path calls the common handler directly and passes no timeout, so it takes the 60 second default.Behavior note for settlement. A settle over the management link that would previously have blocked now bounds at 60 seconds and is retried. Because the lock can expire during that window, the caller can surface
MessageLockLostError(ERROR_CODE_MESSAGE_LOCK_LOSTis mapped in_ERROR_CODE_TO_ERROR_MAPPING) where the call used to block instead. That is the intended trade: a bounded, diagnosable failure rather than an open-ended hang.Not affected:
receive_messages(), receiver iteration, andsend_messages()— the data path is unchanged. CBS token auth is also untouched: it runs through_pyamqp/cbs.py, a separate path, which is the same exclusion the Go change makes explicitly.What a caller sees on a stalled service
The service returns
com.microsoft:timeout, which maps toOperationTimeoutErrorvia the existing_ERROR_CODE_TO_ERROR_MAPPING. That error is alreadyretryable=True, so the retry loop retries it — with default settings a persistently stalled service surfaces after roughly four minutes (4 attempts × 60 s + backoff) rather than hanging indefinitely.No new exception type was needed. Go added an
ErrTryTimeoutExhaustedsentinel to force a retryable classification; Python'sOperationTimeoutErroralready has it.Implementation notes
_base_handler._mgmt_request_response(and the async twin), after the retry loop computes the remaining time, so the value reflects the time actually left on the attempt rather than when the operation was first requested.application_propertiesthrough unchanged and both exposeAMQP_UINT_VALUE, so no transport change is needed.REQUEST_RESPONSE_TIMEOUTalready existed asb"com.microsoft:server-timeout"— the exact key .NET and Java send — but was unused. This change finally uses it.Testing
New
tests/unittests/test_server_timeout.py— 15 unit tests, no credentials or live namespace required:com.microsoft:server-timeout, encoded as an AMQP uint of millisecondskeep_alive_associated_link=Falsepathsapi.mdandapi.metadata.ymlare unchanged — verified by runningazpysdk apistub .;apiMdSha256is identical. Everything added lives in private modules, so there is no public API surface change.Local results: 109 unit tests pass, pylint 10.00/10, black clean.
Related
try_timeout(the Python analogue of the still-open Azure/azure-sdk-for-go#27176) is deliberately not in this PR and will follow separately.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines