Skip to content

Commit 1bf9bf9

Browse files
Ambient Code Botclaude
andcommitted
fix: use status code for PERMISSION_DENIED, remove timeout floor, add tests
- Check e.code() == PERMISSION_DENIED instead of fragile string matching on e.details(), preventing false positives/negatives - Remove 0.5s floor on channel_ready_timeout to prevent overshooting dial_timeout when remaining time is small - Add test verifying channel_ready_timeout is bounded by remaining deadline - Add tests for PERMISSION_DENIED with custom details and UNAUTHENTICATED with "permission denied" text Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent f906fa2 commit 1bf9bf9

2 files changed

Lines changed: 82 additions & 32 deletions

File tree

python/packages/jumpstarter/jumpstarter/client/lease.py

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -315,12 +315,14 @@ def __contextmanager__(self) -> Generator[Self]:
315315

316316
# DEADLINE_EXCEEDED and CANCELLED are excluded: they indicate client-side
317317
# timeout or cancellation, not server/network transients worth retrying.
318-
_TRANSIENT_GRPC_CODES = frozenset({
319-
grpc.StatusCode.UNAVAILABLE,
320-
grpc.StatusCode.RESOURCE_EXHAUSTED,
321-
grpc.StatusCode.ABORTED,
322-
grpc.StatusCode.INTERNAL,
323-
})
318+
_TRANSIENT_GRPC_CODES = frozenset(
319+
{
320+
grpc.StatusCode.UNAVAILABLE,
321+
grpc.StatusCode.RESOURCE_EXHAUSTED,
322+
grpc.StatusCode.ABORTED,
323+
grpc.StatusCode.INTERNAL,
324+
}
325+
)
324326

325327
# UNKNOWN error messages that indicate transient tunnel teardowns.
326328
# We don't blanket-retry all UNKNOWN errors (they could be permanent
@@ -333,9 +335,7 @@ def _retry_delay(attempt: int, remaining: float, base: float = 0.3, cap: float =
333335
"""Compute exponential-backoff delay, capped by *cap* and *remaining* time."""
334336
return min(base * (2**attempt), cap, remaining)
335337

336-
async def _dial_and_connect(
337-
self, stream: SocketStream, channel_ready_timeout: float = 10.0
338-
) -> None:
338+
async def _dial_and_connect(self, stream: SocketStream, channel_ready_timeout: float = 10.0) -> None:
339339
"""Single attempt; raises on failure for caller-driven retry."""
340340
response = await self.controller.Dial(jumpstarter_pb2.DialRequest(lease_name=self.name))
341341
async with connect_router_stream(
@@ -360,7 +360,7 @@ async def handle_async(self, stream: SocketStream) -> None:
360360
attempt = 0
361361
while True:
362362
remaining = deadline - time.monotonic()
363-
channel_ready_timeout = max(min(10.0, remaining), 0.5)
363+
channel_ready_timeout = max(min(10.0, remaining), 0)
364364
try:
365365
await self._dial_and_connect(stream, channel_ready_timeout=channel_ready_timeout)
366366
return
@@ -409,7 +409,7 @@ async def handle_async(self, stream: SocketStream) -> None:
409409
await sleep(delay)
410410
attempt += 1
411411
continue
412-
if "permission denied" in str(e.details()).lower():
412+
if e.code() == grpc.StatusCode.PERMISSION_DENIED:
413413
self.lease_transferred = True
414414
logger.warning(
415415
"Lease %s has been transferred to another client. Your session is no longer valid.",

python/packages/jumpstarter/jumpstarter/client/lease_test.py

Lines changed: 71 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -678,9 +678,7 @@ async def dial_side_effect(req):
678678
nonlocal call_count
679679
call_count += 1
680680
if call_count == 1:
681-
raise _make_aio_rpc_error(
682-
grpc.StatusCode.UNKNOWN, "watch channel closed"
683-
)
681+
raise _make_aio_rpc_error(grpc.StatusCode.UNKNOWN, "watch channel closed")
684682
return dial_response
685683

686684
lease.controller.Dial = AsyncMock(side_effect=dial_side_effect)
@@ -703,9 +701,7 @@ async def test_unknown_without_known_message_not_retried(self):
703701
lease = _make_lease_for_handle()
704702

705703
lease.controller.Dial = AsyncMock(
706-
side_effect=_make_aio_rpc_error(
707-
grpc.StatusCode.UNKNOWN, "some unexpected server bug"
708-
),
704+
side_effect=_make_aio_rpc_error(grpc.StatusCode.UNKNOWN, "some unexpected server bug"),
709705
)
710706

711707
with patch("jumpstarter.client.lease.sleep", new_callable=AsyncMock):
@@ -874,9 +870,7 @@ async def dial_side_effect(req):
874870
nonlocal call_count
875871
call_count += 1
876872
if call_count <= total_failures:
877-
raise _make_aio_rpc_error(
878-
grpc.StatusCode.UNAVAILABLE, "tunnel dropped"
879-
)
873+
raise _make_aio_rpc_error(grpc.StatusCode.UNAVAILABLE, "tunnel dropped")
880874
return dial_response
881875

882876
lease.controller.Dial = AsyncMock(side_effect=dial_side_effect)
@@ -904,10 +898,7 @@ async def fake_router(*args, **kwargs):
904898
actual_delays = [call.args[0] for call in mock_sleep.call_args_list]
905899
assert len(actual_delays) == len(expected_delays)
906900
for actual, expected in zip(actual_delays, expected_delays, strict=True):
907-
assert actual == pytest.approx(expected), (
908-
f"Expected delay {expected}, got {actual}"
909-
)
910-
901+
assert actual == pytest.approx(expected), f"Expected delay {expected}, got {actual}"
911902

912903
@pytest.mark.anyio
913904
async def test_failed_precondition_not_ready_retries_then_succeeds(self):
@@ -920,9 +911,7 @@ async def dial_side_effect(req):
920911
nonlocal call_count
921912
call_count += 1
922913
if call_count == 1:
923-
raise _make_aio_rpc_error(
924-
grpc.StatusCode.FAILED_PRECONDITION, "exporter not ready"
925-
)
914+
raise _make_aio_rpc_error(grpc.StatusCode.FAILED_PRECONDITION, "exporter not ready")
926915
return dial_response
927916

928917
lease.controller.Dial = AsyncMock(side_effect=dial_side_effect)
@@ -946,9 +935,7 @@ async def test_failed_precondition_returns_after_timeout(self):
946935
lease.dial_timeout = 0.0 # already expired
947936

948937
lease.controller.Dial = AsyncMock(
949-
side_effect=_make_aio_rpc_error(
950-
grpc.StatusCode.FAILED_PRECONDITION, "exporter not ready"
951-
),
938+
side_effect=_make_aio_rpc_error(grpc.StatusCode.FAILED_PRECONDITION, "exporter not ready"),
952939
)
953940

954941
with patch("jumpstarter.client.lease.sleep", new_callable=AsyncMock):
@@ -963,16 +950,79 @@ async def test_permission_denied_sets_lease_transferred(self):
963950
lease = _make_lease_for_handle()
964951
assert lease.lease_transferred is False
965952

953+
lease.controller.Dial = AsyncMock(
954+
side_effect=_make_aio_rpc_error(grpc.StatusCode.PERMISSION_DENIED, "permission denied"),
955+
)
956+
957+
with patch("jumpstarter.client.lease.sleep", new_callable=AsyncMock):
958+
await lease.handle_async(Mock())
959+
960+
assert lease.lease_transferred is True
961+
962+
@pytest.mark.anyio
963+
async def test_permission_denied_with_custom_details_still_detected(self):
964+
"""PERMISSION_DENIED with non-standard detail text should still set lease_transferred."""
965+
lease = _make_lease_for_handle()
966+
assert lease.lease_transferred is False
967+
968+
lease.controller.Dial = AsyncMock(
969+
side_effect=_make_aio_rpc_error(grpc.StatusCode.PERMISSION_DENIED, "lease reassigned to another client"),
970+
)
971+
972+
with patch("jumpstarter.client.lease.sleep", new_callable=AsyncMock):
973+
await lease.handle_async(Mock())
974+
975+
assert lease.lease_transferred is True
976+
977+
@pytest.mark.anyio
978+
async def test_unauthenticated_with_permission_text_does_not_set_transferred(self):
979+
"""UNAUTHENTICATED with 'permission denied' in details should NOT set lease_transferred."""
980+
lease = _make_lease_for_handle()
981+
assert lease.lease_transferred is False
982+
966983
lease.controller.Dial = AsyncMock(
967984
side_effect=_make_aio_rpc_error(
968-
grpc.StatusCode.PERMISSION_DENIED, "permission denied"
985+
grpc.StatusCode.UNAUTHENTICATED,
986+
"permission denied: token expired",
969987
),
970988
)
971989

972990
with patch("jumpstarter.client.lease.sleep", new_callable=AsyncMock):
973991
await lease.handle_async(Mock())
974992

975-
assert lease.lease_transferred is True
993+
assert lease.lease_transferred is False
994+
995+
@pytest.mark.anyio
996+
async def test_channel_ready_timeout_bounded_by_remaining(self):
997+
"""channel_ready_timeout should decrease as the dial deadline approaches."""
998+
lease = _make_lease_for_handle()
999+
lease.dial_timeout = 3.0
1000+
1001+
call_count = 0
1002+
captured_timeouts = []
1003+
1004+
async def tracking_dial_and_connect(self_inner, stream, channel_ready_timeout=10.0):
1005+
nonlocal call_count
1006+
call_count += 1
1007+
captured_timeouts.append(channel_ready_timeout)
1008+
if call_count <= 3:
1009+
raise _make_aio_rpc_error(grpc.StatusCode.FAILED_PRECONDITION, "exporter not ready")
1010+
# Succeed on 4th attempt (won't normally reach here with 3s timeout)
1011+
1012+
with (
1013+
patch.object(type(lease), "_dial_and_connect", tracking_dial_and_connect),
1014+
patch("jumpstarter.client.lease.sleep", new_callable=AsyncMock),
1015+
):
1016+
await lease.handle_async(Mock())
1017+
1018+
# With a 3s dial_timeout, the first call should have channel_ready_timeout <= 3.0
1019+
# and subsequent calls should have progressively smaller values
1020+
assert len(captured_timeouts) >= 2
1021+
assert all(t <= 10.0 for t in captured_timeouts), f"All timeouts should be <= 10.0, got {captured_timeouts}"
1022+
# The first timeout should be bounded by remaining (~3.0), not the default 10.0
1023+
assert captured_timeouts[0] <= 3.1, (
1024+
f"First timeout should be bounded by dial_timeout (~3.0), got {captured_timeouts[0]}"
1025+
)
9761026

9771027

9781028
class TestRetryDelay:

0 commit comments

Comments
 (0)