fix: per-connection Dial retry with dual timeout budget - #951
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
e6e0add to
40c84b7
Compare
35c8335 to
17eb846
Compare
| while True: | ||
| try: | ||
| return await self.controller.Dial(jumpstarter_pb2.DialRequest(lease_name=self.name)) | ||
| response = await self.controller.Dial(jumpstarter_pb2.DialRequest(lease_name=self.name)) |
There was a problem hiding this comment.
No per-RPC timeout on the Dial call. The keepalive bound (200s) can silently exceed dial_timeout (60s).
| async with anyio.from_thread.BlockingPortal() as portal: | ||
| connect_deadline = None | ||
| connect_start = None | ||
| while True: |
There was a problem hiding this comment.
Token monitor tasks accumulate without cancellation across outer retry iterations. It would be better tying the monitoring task lifetime to the lease using a per-lease cancel scope, cancelling the monitor in a finally block before each continue.
| if connect_deadline is None: | ||
| connect_deadline = time.monotonic() + lease.retry_timeout | ||
| connect_start = time.monotonic() | ||
| connect_deadline = connect_start + lease.retry_timeout |
There was a problem hiding this comment.
connect_deadline in the shell retry loop never resets after successful reconnection.
| Raises ExporterUnreachableError on timeout or unrecoverable error. | ||
| """ | ||
| logger.debug("Dialing controller for lease %s", self.name) | ||
| async def handle_async(self, stream): # noqa: C901 |
There was a problem hiding this comment.
function parameters lack type annotations
| attempt += 1 | ||
| continue | ||
| # Exporter went offline or lease ended - raise immediately | ||
| if "permission denied" in str(e.details()).lower(): |
There was a problem hiding this comment.
Consider adding or e.code() == grpc.StatusCode.PERMISSION_DENIED for defense-in-depth.
| except OSError as e: | ||
| raise ExporterUnreachableError( | ||
| f"Router {response.router_endpoint} connection failed: {e}" | ||
| ) from e |
There was a problem hiding this comment.
stream is not closed when Dial fails before the router stream is entered. Consider wrapping the method body with try/except Exception: await stream.aclose(); raise.
4f46d3e to
6a6e4ff
Compare
2ed068b to
2f50df1
Compare
dc578c2 to
2b3d1ec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
515-532: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnter the monitor
CancelScopeinside_monitor_token_expiry.
monitor_scope.cancel()only updates the scope state; since_monitor_token_expiryhosts its own loop and checkscancel_scope.cancel_calledonly beforeanyio.sleep, the monitor task can keep polling for 30s after shell cleanup starts. Wrap the monitor task body inwith cancel_scope:so cancellation takes effect at the sleep checkpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py` around lines 515 - 532, Wrap the body of _monitor_token_expiry in with cancel_scope so the monitor task executes inside its CancelScope and observes monitor_scope.cancel() at the anyio.sleep checkpoint. Keep the existing polling logic and cancellation call in the shell lease flow unchanged.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/lease_test.py (1)
629-648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
retry_timeoutgreater thandial_timeout.This test sets
retry_timeout=0.5and keepsdial_timeout=5.0, so the retry budget expires before the dial budget. The production defaults are the opposite (dial_timeout=60,retry_timeout=300). No test exercises_connected=Truewithretry_timeout > dial_timeout, which is the path where the per-RPC deadline inhandle_asyncclamps to the 0.1s floor. Add a case with, for example,dial_timeout=0.2andretry_timeout=1.0, and assert thatDialkeeps retrying past the dial budget.Based on learnings, provide comprehensive package test coverage for the changed paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py` around lines 629 - 648, Add coverage in test_dial_unavailable_exceeds_retry_timeout or a nearby test for _connected=True with dial_timeout shorter than retry_timeout, such as 0.2 and 1.0. Make the mocked Dial continue returning UNAVAILABLE, assert retries continue past the dial timeout while remaining within the retry budget, and verify the resulting ExporterUnreachableError and stream cleanup; include coverage for the changed handle_async deadline-clamping path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py`:
- Around line 1172-1174: Strengthen the retry-exhaustion test around
_shell_with_signal_handling by asserting multiple attempts and verifying retries
stop at the configured lease.retry_timeout deadline, rather than only checking
the final error. Control the retry clock or sleep behavior deterministically to
avoid flaky timing, and ensure the changed Python test maintains comprehensive
package coverage.
In `@python/packages/jumpstarter/jumpstarter/client/lease.py`:
- Around line 336-347: Update the retry loop around Controller.Dial to calculate
each RPC timeout from the later of dial_deadline and unavailable_deadline,
preserving the existing minimum floor. Extend the UNAVAILABLE retry handling to
include DEADLINE_EXCEEDED while the active retry budget remains, so deadline
failures continue retrying instead of falling through to the abort path.
---
Outside diff comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 515-532: Wrap the body of _monitor_token_expiry in with
cancel_scope so the monitor task executes inside its CancelScope and observes
monitor_scope.cancel() at the anyio.sleep checkpoint. Keep the existing polling
logic and cancellation call in the shell lease flow unchanged.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py`:
- Around line 629-648: Add coverage in
test_dial_unavailable_exceeds_retry_timeout or a nearby test for _connected=True
with dial_timeout shorter than retry_timeout, such as 0.2 and 1.0. Make the
mocked Dial continue returning UNAVAILABLE, assert retries continue past the
dial timeout while remaining within the retry budget, and verify the resulting
ExporterUnreachableError and stream cleanup; include coverage for the changed
handle_async deadline-clamping path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ddcc8f40-96a7-4e07-ac74-639d7caace59
📒 Files selected for processing (7)
e2e/test/hooks_test.gopython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell_test.pypython/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/client/lease_test.pypython/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
💤 Files with no reviewable changes (1)
- python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
| assert "test-exporter" in str(exc) | ||
| assert "unreachable" in str(exc).lower() | ||
| assert state["call_count"] >= 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve coverage for retry exhaustion.
These assertions verify only the final error text. They do not prove that _shell_with_signal_handling retried until lease.retry_timeout; state["call_count"] >= 1 also passes if the first failure is propagated immediately. Assert multiple attempts and verify that retries stop at the configured deadline. If timing makes this flaky, control the retry clock or sleep in the test.
As per coding guidelines, changed Python tests must provide comprehensive package test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py` around lines
1172 - 1174, Strengthen the retry-exhaustion test around
_shell_with_signal_handling by asserting multiple attempts and verifying retries
stop at the configured lease.retry_timeout deadline, rather than only checking
the final error. Control the retry clock or sleep behavior deterministically to
avoid flaky timing, and ensure the changed Python test maintains comprehensive
package coverage.
Source: Coding guidelines
| unavail_budget = self.retry_timeout if self._connected else self.dial_timeout | ||
| unavailable_deadline = started + unavail_budget if unavail_budget > 0 else None | ||
| attempt = 0 | ||
| while True: | ||
| try: | ||
| return await self.controller.Dial(jumpstarter_pb2.DialRequest(lease_name=self.name)) | ||
| except AioRpcError as e: | ||
| if e.code() == grpc.StatusCode.FAILED_PRECONDITION and "not ready" in str(e.details()): | ||
| remaining = deadline - time.monotonic() | ||
| if remaining <= 0: | ||
| warned_unavailable = False | ||
| try: | ||
| while True: | ||
| try: | ||
| dial_remaining = max(dial_deadline - time.monotonic(), 0.1) | ||
| response = await self.controller.Dial( | ||
| jumpstarter_pb2.DialRequest(lease_name=self.name), | ||
| timeout=dial_remaining, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound the per-RPC timeout by the active deadline, not only by dial_deadline.
dial_remaining always derives from dial_deadline (started + dial_timeout, 60s by default). For an established session, unavailable_deadline uses retry_timeout (300s by default). After 60s of UNAVAILABLE retries, dial_remaining clamps to the 0.1s floor. Each further Dial then gets a 0.1s deadline and most likely fails with DEADLINE_EXCEEDED. DEADLINE_EXCEEDED matches no retry branch, so the loop falls through to Line 417 and aborts. The remaining 240s of the retry budget is then unusable, which contradicts the goal of surviving controller restarts.
Use the later of the two active deadlines for the per-RPC timeout, and treat DEADLINE_EXCEEDED like UNAVAILABLE while the retry budget remains.
🐛 Proposed fix for the per-RPC deadline
while True:
try:
- dial_remaining = max(dial_deadline - time.monotonic(), 0.1)
+ now = time.monotonic()
+ effective_deadline = dial_deadline
+ if unavailable_deadline is not None:
+ effective_deadline = max(effective_deadline, unavailable_deadline)
+ dial_remaining = max(effective_deadline - now, 0.1)
response = await self.controller.Dial(
jumpstarter_pb2.DialRequest(lease_name=self.name),
timeout=dial_remaining,
)
break
except AioRpcError as e:Then extend the availability branch:
- if e.code() == grpc.StatusCode.UNAVAILABLE:
+ if e.code() in (grpc.StatusCode.UNAVAILABLE, grpc.StatusCode.DEADLINE_EXCEEDED):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unavail_budget = self.retry_timeout if self._connected else self.dial_timeout | |
| unavailable_deadline = started + unavail_budget if unavail_budget > 0 else None | |
| attempt = 0 | |
| while True: | |
| try: | |
| return await self.controller.Dial(jumpstarter_pb2.DialRequest(lease_name=self.name)) | |
| except AioRpcError as e: | |
| if e.code() == grpc.StatusCode.FAILED_PRECONDITION and "not ready" in str(e.details()): | |
| remaining = deadline - time.monotonic() | |
| if remaining <= 0: | |
| warned_unavailable = False | |
| try: | |
| while True: | |
| try: | |
| dial_remaining = max(dial_deadline - time.monotonic(), 0.1) | |
| response = await self.controller.Dial( | |
| jumpstarter_pb2.DialRequest(lease_name=self.name), | |
| timeout=dial_remaining, | |
| ) | |
| unavail_budget = self.retry_timeout if self._connected else self.dial_timeout | |
| unavailable_deadline = started + unavail_budget if unavail_budget > 0 else None | |
| attempt = 0 | |
| warned_unavailable = False | |
| try: | |
| while True: | |
| try: | |
| now = time.monotonic() | |
| effective_deadline = dial_deadline | |
| if unavailable_deadline is not None: | |
| effective_deadline = max(effective_deadline, unavailable_deadline) | |
| dial_remaining = max(effective_deadline - now, 0.1) | |
| response = await self.controller.Dial( | |
| jumpstarter_pb2.DialRequest(lease_name=self.name), | |
| timeout=dial_remaining, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter/jumpstarter/client/lease.py` around lines 336 -
347, Update the retry loop around Controller.Dial to calculate each RPC timeout
from the later of dial_deadline and unavailable_deadline, preserving the
existing minimum floor. Extend the UNAVAILABLE retry handling to include
DEADLINE_EXCEEDED while the active retry budget remains, so deadline failures
continue retrying instead of falling through to the abort path.
35f742f to
d69330d
Compare
487210c to
8bd85ef
Compare
Replace pre-flight _dial_with_retry with handle_async that retries Dial
on every Unix socket connection independently. Use dial_timeout (60s)
for FAILED_PRECONDITION ("not ready") and retry_timeout (300s, only when
_connected=True) for UNAVAILABLE, so initial lease acquisition stays
fast while established sessions survive controller restarts.
Fix permission-denied detection: controller sends fmt.Errorf("permission
denied") which maps to UNKNOWN, not PERMISSION_DENIED — match on
details text instead of status code.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Assisted-by: claude-opus-4.6
2 participants
Replace pre-flight _dial_with_retry with handle_async that retries Dial on every Unix socket connection independently. Use dial_timeout (60s) for FAILED_PRECONDITION ("not ready") and retry_timeout (300s, only when _connected=True) for UNAVAILABLE, so initial lease acquisition stays fast while established sessions survive controller restarts.
Fix permission-denied detection: controller sends fmt.Errorf("permission denied") which maps to UNKNOWN, not PERMISSION_DENIED - match on details text instead of status code.
Depends on #950