Revert "Revert "shell: keep shell usable after token refreshes""#290
Open
Revert "Revert "shell: keep shell usable after token refreshes""#290
Conversation
Contributor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds proactive lease-aware token monitoring and recovery (OIDC refresh + disk reload), updates Lease to support refreshing its gRPC channel and improved end-time handling, expands async/unit tests, and adds an AnyIO pytest backend fixture. Changes
Sequence DiagramsequenceDiagram
participant Monitor as _monitor_token_expiry
participant Config as Config
participant OIDC as OIDC_Server
participant Lease as Lease_Channel
participant Disk as Disk_Storage
Monitor->>Config: check token remaining
alt remaining <= threshold
Monitor->>Monitor: _attempt_token_recovery()
Monitor->>OIDC: try refresh using refresh_token
alt refresh success
OIDC-->>Config: return new token / refresh_token
Monitor->>Config: update in-memory token (best-effort save)
Monitor->>Lease: _update_lease_channel(config, lease)
else refresh failed
Monitor->>Disk: try reload token from disk
alt disk reload success
Disk-->>Config: provide valid token
Monitor->>Lease: _update_lease_channel(config, lease)
else disk reload failed
Monitor->>Monitor: _warn_refresh_failed(remaining)
end
end
end
Monitor->>Monitor: sleep (5s if near threshold, else 30s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py`:
- Around line 339-341: The test currently mutates the global Mock class via
type(cancel_scope).cancel_called = property(...), which leaks state; instead
define a local subclass (e.g., class CancelMock(Mock): cancel_called =
property(lambda self: check_cancelled())) and instantiate cancel_scope =
CancelMock() (or similarly create the subclass inline) so the cancel_called
property is only on that test's instance/class and does not modify
unittest.mock.Mock globally; replace references to the existing instance
creation and the type(...) assignment with this local subclass instantiation.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 180-199: The refresh-failure flag warned_refresh_failed can
permanently suppress the final expired warning; update the logic so whenever you
emit the expired warning you also clear warned_refresh_failed (and set
warned_expiry appropriately). Concretely, in the loop branches that call
_warn_expired (or otherwise display the token-expired red message) ensure you
set warned_refresh_failed = False and warned_expiry = True (and mirror the same
reset in the similar block around lines 211-216) so a prior refresh-failure
warning cannot hide the later expired warning.
In `@python/packages/jumpstarter/jumpstarter/client/lease.py`:
- Around line 290-296: The current _get_lease_end_time method returns None
whenever duration or effective_begin_time is missing, even if
lease.effective_end_time is present; change the logic in _get_lease_end_time to
first check and return lease.effective_end_time if it exists, and only if it
does not exist then require both lease.effective_begin_time and lease.duration
(return None if those are missing) and otherwise return
lease.effective_begin_time + lease.duration so monitor_async can use explicit
end times.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccedc8d2-f256-479e-a629-f46aefaf66d3
📒 Files selected for processing (5)
python/packages/jumpstarter-cli/conftest.pypython/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.py
bennyz
force-pushed
the
revert-210-revert-199-jmp-shell-refresh
branch
from
e288984 to
ec8a54c
Compare
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bennyz
force-pushed
the
revert-210-revert-199-jmp-shell-refresh
branch
from
90676bb to
0e45e8b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
python/packages/jumpstarter/jumpstarter/client/lease.py (1)
350-355:⚠️ Potential issue | 🟠 MajorCheck
effective_end_timebefore the fallback guard.Line 352 returns
Nonebefore looking atlease.effective_end_time, so leases that carry an explicit end time but omiteffective_begin_timeordurationnever report expiry tomonitor_async().💡 Proposed fix
def _get_lease_end_time(self, lease) -> datetime | None: """Extract the end time from a lease response, or None if not available.""" - if not (lease.effective_begin_time and lease.duration): - return None if lease.effective_end_time: return lease.effective_end_time + if not (lease.effective_begin_time and lease.duration): + return None return lease.effective_begin_time + lease.duration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/lease.py` around lines 350 - 355, In _get_lease_end_time, check lease.effective_end_time first and return it if present, before the current guard that returns None when effective_begin_time or duration are missing; otherwise fall back to computing end time from lease.effective_begin_time + lease.duration (if both present) and return None only when neither an explicit effective_end_time nor the begin+duration can be derived—this ensures monitor_async() sees explicit expiries even when effective_begin_time or duration are omitted.python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py (1)
339-340:⚠️ Potential issue | 🟠 MajorKeep
cancel_calledoff the baseMockclass.Line 340 mutates
unittest.mock.Mockitself, so laterMock(cancel_called=False)instances in this file can read the property instead of their own value. That makes the suite order-dependent.💡 Proposed fix
- cancel_scope = Mock() - type(cancel_scope).cancel_called = property(lambda self: check_cancelled()) + class _CancelScope: + `@property` + def cancel_called(self): + return check_cancelled() + + cancel_scope = _CancelScope()Run this to confirm the leak:
#!/bin/bash python - <<'PY' from unittest.mock import Mock first = Mock() type(first).cancel_called = property(lambda self: True) second = Mock(cancel_called=False) print(second.cancel_called) PYIf this prints
True, the class-level property leaked into laterMockinstances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py` around lines 339 - 340, The test is mutating unittest.mock.Mock by assigning type(cancel_scope).cancel_called = property(...), leaking into other Mock instances; instead define a dedicated subclass (e.g. class CancelScopeMock(Mock): `@property` def cancel_called(self): return check_cancelled()) and instantiate that (CancelScopeMock()) for cancel_scope, or attach the needed boolean/result directly to the instance (cancel_scope.cancel_called = <value>) so you do not modify Mock itself; update the creation of cancel_scope where type(cancel_scope).cancel_called is used to use the new subclass or instance attribute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py`:
- Line 395: The test file contains a redundant inner import of
_warn_refresh_failed; remove the unused "from jumpstarter_cli.shell import
_warn_refresh_failed" statement in jumpstarter_cli/shell_test.py (the inner
import at the indicated location) so Ruff F401 is resolved—ensure any references
to _warn_refresh_failed use the existing valid import scope (or add a single
top-level import if the test actually needs it) and then run make lint to verify
the warning is gone.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 129-133: The current logic conditionally copies
disk_config.refresh_token into config only when disk_refresh is not None,
causing in-memory config to retain an old refresh token if the disk value is
explicitly None; change the behavior in the block that currently sets
config.token and reads disk_refresh (referencing config, disk_config, and
refresh_token) to always assign config.refresh_token = getattr(disk_config,
"refresh_token", None) (i.e., mirror the disk refresh_token exactly, removing
the conditional) so the in-memory session matches the reloaded disk credentials.
- Around line 184-239: _extract_monitor_token_expiry complexity by moving the
recovery/warning logic and sleep selection out of _monitor_token_expiry into
small helper functions (e.g., _handle_recovery_and_warnings(config, lease,
remaining, warned_expiry, warned_refresh_failed, warned_token_expired) which
returns updated warned_* flags and any recovery message, and
_select_sleep_duration(remaining) which returns the sleep seconds); update
_monitor_token_expiry to call these helpers and await anyio.sleep(duration)
while preserving all existing behaviors and state transitions for warned_expiry,
warned_refresh_failed, and warned_token_expired so Ruff’s C901 complexity is no
longer exceeded.
---
Duplicate comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py`:
- Around line 339-340: The test is mutating unittest.mock.Mock by assigning
type(cancel_scope).cancel_called = property(...), leaking into other Mock
instances; instead define a dedicated subclass (e.g. class
CancelScopeMock(Mock): `@property` def cancel_called(self): return
check_cancelled()) and instantiate that (CancelScopeMock()) for cancel_scope, or
attach the needed boolean/result directly to the instance
(cancel_scope.cancel_called = <value>) so you do not modify Mock itself; update
the creation of cancel_scope where type(cancel_scope).cancel_called is used to
use the new subclass or instance attribute.
In `@python/packages/jumpstarter/jumpstarter/client/lease.py`:
- Around line 350-355: In _get_lease_end_time, check lease.effective_end_time
first and return it if present, before the current guard that returns None when
effective_begin_time or duration are missing; otherwise fall back to computing
end time from lease.effective_begin_time + lease.duration (if both present) and
return None only when neither an explicit effective_end_time nor the
begin+duration can be derived—this ensures monitor_async() sees explicit
expiries even when effective_begin_time or duration are omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfe3f43c-ef68-4dbe-9a1f-b4ceea5f31c8
📒 Files selected for processing (5)
python/packages/jumpstarter-cli/conftest.pypython/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.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-cli/conftest.py
raballew
requested changes
Mar 18, 2026
Contributor
raballew
left a comment
raballew
left a comment
There was a problem hiding this comment.
_attempt_token_recovery accepts a remaining parameter that's never used in the function body.
_try_refresh_token hardcodes client_id="jumpstarter-cli", but the login flow supports server-configured client IDs (login.py line 181). If someone logged in with a different client ID, token refresh will silently fail and the user won't know why. Same issue exists in relogin_client (has a TODO comment about it).
In _try_refresh_token, if _update_lease_channel fails after config.token was already updated, the rollback restores config.token and config.refresh_token but doesn't undo the channel swap on the lease object -- the lease could be left with a broken channel even though the function returns False.
monitor_async does await sleep(min(remain.total_seconds(), check_interval)) -- when remain is tiny (e.g. 0.001s near lease end), this creates a tight polling loop hitting the gRPC server rapidly. A minimum sleep (e.g. 1s) would prevent this.
bennyz
force-pushed
the
revert-210-revert-199-jmp-shell-refresh
branch
from
6a4fb1d to
44a4190
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
131-135:⚠️ Potential issue | 🟡 MinorMirror the disk
refresh_tokenexactly.The conditional assignment
if disk_refresh is not Nonecan leave stale credentials when the disk config explicitly hasrefresh_token=None. This creates a mixed credential state where the session uses a new access token with an old refresh token.💡 Proposed fix
# Token on disk is valid and different - use it config.token = disk_token - disk_refresh = getattr(disk_config, "refresh_token", None) - if disk_refresh is not None: - config.refresh_token = disk_refresh + config.refresh_token = getattr(disk_config, "refresh_token", None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py` around lines 131 - 135, The current code conditionally sets config.refresh_token only when disk_refresh is not None which can leave a stale refresh token; instead always mirror the disk value by assigning config.refresh_token = getattr(disk_config, "refresh_token", None) (remove the if disk_refresh is not None check) so the in-memory config exactly matches disk_config.refresh_token even when it is explicitly None; update the block that currently sets config.token and disk_refresh to unconditionally set config.refresh_token using disk_config/ disk_refresh.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/lease_test.py (1)
488-523: Consider increasing the timing margins for CI stability.The test uses a 50ms future end time (line 496) and a 200ms sleep (line 518). While this works locally, tight timing can cause flaky failures in slow CI environments. Consider using slightly larger margins (e.g., 200ms future, 500ms sleep) to improve reliability without significantly increasing test duration.
💡 Suggested adjustment
- future_end = datetime.now(tz=timezone.utc) + timedelta(milliseconds=50) + future_end = datetime.now(tz=timezone.utc) + timedelta(milliseconds=200) ... - await asyncio.sleep(0.2) + await asyncio.sleep(0.5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py` around lines 488 - 523, The timing in test_estimates_expiry_from_last_known_end_time is tight; increase the future_end margin and the monitor sleep to make the test stable in CI — specifically, change the future_end creation (currently +50ms) to a larger value (e.g., +200ms) and increase the await asyncio.sleep(...) (currently 0.2s) to a larger value (e.g., 0.5s) so the monitor loop (lease.monitor_async and the get_then_fail sequence) has enough time to run reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 131-135: The current code conditionally sets config.refresh_token
only when disk_refresh is not None which can leave a stale refresh token;
instead always mirror the disk value by assigning config.refresh_token =
getattr(disk_config, "refresh_token", None) (remove the if disk_refresh is not
None check) so the in-memory config exactly matches disk_config.refresh_token
even when it is explicitly None; update the block that currently sets
config.token and disk_refresh to unconditionally set config.refresh_token using
disk_config/ disk_refresh.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py`:
- Around line 488-523: The timing in
test_estimates_expiry_from_last_known_end_time is tight; increase the future_end
margin and the monitor sleep to make the test stable in CI — specifically,
change the future_end creation (currently +50ms) to a larger value (e.g.,
+200ms) and increase the await asyncio.sleep(...) (currently 0.2s) to a larger
value (e.g., 0.5s) so the monitor loop (lease.monitor_async and the
get_then_fail sequence) has enough time to run reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 220fe33f-fa60-402b-b130-8d4db26f64da
📒 Files selected for processing (5)
python/packages/jumpstarter-cli/conftest.pypython/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.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-cli/conftest.py
bennyz
force-pushed
the
revert-210-revert-199-jmp-shell-refresh
branch
from
44a4190 to
bce68fe
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/lease_test.py (1)
488-523: Potential test flakiness due to tight timing.The test uses a 50ms future end time and a 200ms sleep, which could be flaky in slow CI environments. The test relies on the monitor loop iterating twice within this window.
Consider increasing the timing margins or using more deterministic mocking of sleep/time to avoid intermittent failures.
💡 Suggested improvement
- # End time slightly in the future so the monitor caches it and sleeps - future_end = datetime.now(tz=timezone.utc) + timedelta(milliseconds=50) + # End time slightly in the future so the monitor caches it and sleeps + # Use larger margins to avoid flakiness in slow CI environments + future_end = datetime.now(tz=timezone.utc) + timedelta(milliseconds=200) call_count = 0 ... - # through the first get(), sleep, second get() (fails), and - # error handler using the cached end time. - await asyncio.sleep(0.2) + # through the first get(), sleep, second get() (fails), and + # error handler using the cached end time. + await asyncio.sleep(0.5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py` around lines 488 - 523, The test test_estimates_expiry_from_last_known_end_time is fragile due to tight timing (future_end + 50ms and await asyncio.sleep(0.2)); make it deterministic by increasing margins or mocking sleep/time: either bump future_end and the asyncio.sleep duration (e.g., use larger milliseconds) so lease.monitor_async has ample time to call get_then_fail twice, or patch/monkeypatch the monitor's sleep/timer (or asyncio.sleep) inside the test to a fast, controlled stub so the loop iterations and cached-end-time error path reliably execute; update references to get_then_fail, lease.monitor_async, and caplog accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py`:
- Around line 488-523: The test test_estimates_expiry_from_last_known_end_time
is fragile due to tight timing (future_end + 50ms and await asyncio.sleep(0.2));
make it deterministic by increasing margins or mocking sleep/time: either bump
future_end and the asyncio.sleep duration (e.g., use larger milliseconds) so
lease.monitor_async has ample time to call get_then_fail twice, or
patch/monkeypatch the monitor's sleep/timer (or asyncio.sleep) inside the test
to a fast, controlled stub so the loop iterations and cached-end-time error path
reliably execute; update references to get_then_fail, lease.monitor_async, and
caplog accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2862e6ae-fda1-498b-b143-9f267b25c9c8
📒 Files selected for processing (5)
python/packages/jumpstarter-cli/conftest.pypython/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.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-cli/conftest.py
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Consider effective_begin_time or duration Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Make it clear when jmp shell may leave a lease behind after token recovery fails, and cover the normal-exit regression with tests. Assisted-by: claude-sonnet-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
revert-210-revert-199-jmp-shell-refresh
branch
from
bce68fe to
c9f50c1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
144-146:⚠️ Potential issue | 🟡 MinorMirror the disk
refresh_tokenexactly.The conditional assignment can leave a stale
refresh_tokenin memory when the disk config hasrefresh_token=None. This could cause the next auto-refresh attempt to use an outdated refresh token against a token from a different login session.💡 Proposed fix
- disk_refresh = getattr(disk_config, "refresh_token", None) - if disk_refresh is not None: - config.refresh_token = disk_refresh + config.refresh_token = getattr(disk_config, "refresh_token", None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py` around lines 144 - 146, The current conditional leaves a stale in-memory token when disk_config.refresh_token is None; instead mirror the disk value exactly by assigning config.refresh_token = getattr(disk_config, "refresh_token", None) unconditionally (remove the if check that only sets when disk_refresh is not None) so that config.refresh_token is cleared when the disk refresh_token is None; reference the disk_refresh variable, disk_config, and config.refresh_token when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 144-146: The current conditional leaves a stale in-memory token
when disk_config.refresh_token is None; instead mirror the disk value exactly by
assigning config.refresh_token = getattr(disk_config, "refresh_token", None)
unconditionally (remove the if check that only sets when disk_refresh is not
None) so that config.refresh_token is cleared when the disk refresh_token is
None; reference the disk_refresh variable, disk_config, and config.refresh_token
when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b6543cc-dbad-4790-878c-9a2cd42427eb
📒 Files selected for processing (5)
python/packages/jumpstarter-cli/conftest.pypython/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.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-cli/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter/jumpstarter/client/lease_test.py
- python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/lease_test.py (1)
342-350: Tighten therefresh_channel()regression assertions.This currently only proves the constructors ran. A regression that forgets to assign
lease.svc, passes the wrong namespace, or closes the original channel would still pass while cleanup RPCs keep using stale credentials or in-flight RPCs get cut off.💡 Proposed test tightening
def _make_lease(self): """Create a Lease with mocked dependencies.""" channel = Mock(name="original_channel") lease = object.__new__(Lease) lease.channel = channel lease.namespace = "default" lease.controller = Mock(name="original_controller") lease.svc = Mock(name="original_svc") - return lease + return lease, channel @@ - lease = self._make_lease() + lease, old_channel = self._make_lease() new_channel = Mock(name="new_channel") @@ assert lease.channel is new_channel mock_stub_cls.assert_called_once_with(new_channel) assert lease.controller is mock_stub_cls.return_value - mock_svc_cls.assert_called_once() + mock_svc_cls.assert_called_once_with(channel=new_channel, namespace="default") + assert lease.svc is mock_svc_cls.return_value + old_channel.close.assert_not_called()Based on learnings:
refresh_channel()must not close the old channel because token refresh can overlap with in-flight RPCs.Also applies to: 354-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py` around lines 342 - 350, Tighten the refresh_channel() test in _make_lease / tests around Lease.refresh_channel by asserting that after calling refresh_channel the lease.svc is replaced with a new svc object (not None), lease.namespace remains exactly "default" and lease.controller is unchanged, the new lease.channel is a different object than the original channel, and the original channel's close() was NOT called (i.e. refresh must not close old channel); also assert any stub/credential used for cleanup was created for the new channel rather than the old one to prevent stale-credential RPCs. Use the existing symbols Lease, _make_lease, lease.channel, lease.svc, lease.namespace and refresh_channel to locate and update the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 448-449: The warning is shown even when the lease has already
ended because the condition only checks token_state["expired_unrecovered"];
update the conditional in the block that calls _warn_about_expired_token to also
verify the lease hasn't already ended (e.g., require not lease.lease_ended) so
you only warn about orphaned leases that are still active; locate the check
around lease.release, lease.name and token_state in the function/method that
handles lease state and add the extra lease.lease_ended guard before calling
_warn_about_expired_token.
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py`:
- Around line 482-486: The test's async context for lease.monitor_async() can
exit before the background worker completes the third failing_get call, making
assert call_count == 3 flaky; modify the test to keep the monitor context open
until the third call occurs by having failing_get signal (e.g., set an
asyncio.Event or similar) when it increments call_count the third time and
awaiting that signal inside the test before exiting the async with block,
ensuring monitor_async() runs long enough for call_count to reach 3; update
references to failing_get, call_count and monitor_async in the test accordingly.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py`:
- Around line 342-350: Tighten the refresh_channel() test in _make_lease / tests
around Lease.refresh_channel by asserting that after calling refresh_channel the
lease.svc is replaced with a new svc object (not None), lease.namespace remains
exactly "default" and lease.controller is unchanged, the new lease.channel is a
different object than the original channel, and the original channel's close()
was NOT called (i.e. refresh must not close old channel); also assert any
stub/credential used for cleanup was created for the new channel rather than the
old one to prevent stale-credential RPCs. Use the existing symbols Lease,
_make_lease, lease.channel, lease.svc, lease.namespace and refresh_channel to
locate and update the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 201d793f-088e-4309-80e7-51f4df897472
📒 Files selected for processing (5)
python/packages/jumpstarter-cli/conftest.pypython/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.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-cli/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter/jumpstarter/client/lease.py
Comment on lines
+448
to
+449
| if lease.release and lease.name and token_state["expired_unrecovered"]: | ||
| _warn_about_expired_token(lease.name, selector) |
Contributor
There was a problem hiding this comment.
Don’t warn about an orphaned lease after natural expiry.
token_state["expired_unrecovered"] only says auth-based cleanup cannot run. If lease.monitor_async() has already set lease.lease_ended, this branch still tells the user the lease will remain active and suggests reconnecting to a lease that no longer exists.
💡 Proposed fix
- if lease.release and lease.name and token_state["expired_unrecovered"]:
+ if (
+ lease.release
+ and lease.name
+ and token_state["expired_unrecovered"]
+ and not lease.lease_ended
+ ):
_warn_about_expired_token(lease.name, selector)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py` around lines 448 -
449, The warning is shown even when the lease has already ended because the
condition only checks token_state["expired_unrecovered"]; update the conditional
in the block that calls _warn_about_expired_token to also verify the lease
hasn't already ended (e.g., require not lease.lease_ended) so you only warn
about orphaned leases that are still active; locate the check around
lease.release, lease.name and token_state in the function/method that handles
lease state and add the extra lease.lease_ended guard before calling
_warn_about_expired_token.
Comment on lines
+482
to
+486
| with patch("jumpstarter.client.lease.sleep", new_callable=AsyncMock): | ||
| async with lease.monitor_async(): | ||
| pass | ||
|
|
||
| assert call_count == 3 # two failures + one success |
Contributor
There was a problem hiding this comment.
Wait for the monitor task before asserting call_count.
The empty async with lease.monitor_async(): pass body can cancel the background worker before it reaches the third get(). That makes assert call_count == 3 scheduler-dependent and flaky on slower CI runs. Hold the context open until failing_get() signals the third call, then assert the final count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py` around lines
482 - 486, The test's async context for lease.monitor_async() can exit before
the background worker completes the third failing_get call, making assert
call_count == 3 flaky; modify the test to keep the monitor context open until
the third call occurs by having failing_get signal (e.g., set an asyncio.Event
or similar) when it increments call_count the third time and awaiting that
signal inside the test before exiting the async with block, ensuring
monitor_async() runs long enough for call_count to reach 3; update references to
failing_get, call_count and monitor_async in the test accordingly.
mangelajo
approved these changes
Mar 25, 2026
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
3 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.
Reverts #210