Lease handling user enhancements #671
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds async orchestration for CLI shell execution with signal-aware cancellation, lease acquisition with configurable timeout and exporter tracking, exporter-log streaming integration, expanded exception handling for BaseExceptionGroup and reauthentication helpers, client config for lease timeouts, and corresponding test updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI command
participant AnyIO as anyio.run
participant Portal as BlockingPortal / thread
participant Lease as Lease manager
participant Exporter as Exporter logs
participant Shell as Remote shell
User->>CLI: invoke shell
CLI->>AnyIO: start _shell_with_signal_handling()
AnyIO->>Portal: enter portal to run blocking lease task
AnyIO->>Lease: acquire (timeout = config.leases.acquisition_timeout)
alt lease acquired
par stream logs (optional)
Portal->>Exporter: stream exporter logs
and launch shell
Portal->>Shell: start remote shell session
Shell-->>Portal: exit status
end
else invalid / timeout
Lease-->>Portal: raises LeaseError / TimeoutError
end
AnyIO->>AnyIO: signal handler -> cancel task group
AnyIO->>Lease: cleanup (shielded, 30s)
AnyIO-->>CLI: return mapped exit code (0 / 2 / error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code graph analysis (3)packages/jumpstarter/jumpstarter/client/lease.py (3)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (5)
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 |
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
8d9846d to
ebed8db
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/client/lease.py (2)
148-149: Consider moving CancelScope to top-level imports.While the local import works correctly, moving
CancelScopeto the top-level anyio import block (lines 12-18) would improve consistency since other anyio symbols are already imported there.Apply this diff to move the import:
from anyio import ( AsyncContextManagerMixin, ContextManagerMixin, + CancelScope, create_task_group, fail_after, sleep, )And remove the local import:
@asynccontextmanager async def __asynccontextmanager__(self) -> AsyncGenerator[Self]: - from anyio import CancelScope - try:
156-160: Consider adding a timeout within the cancellation shield.While the cancellation shield ensures cleanup completes, if
DeleteLeasehangs indefinitely, it could delay application shutdown. Consider adding a timeout within the shielded scope for robustness.Apply this diff to add a timeout:
# Shield cleanup from cancellation to ensure it completes with CancelScope(shield=True): - await self.svc.DeleteLease( - name=self.name, - ) + with fail_after(30): # Timeout for cleanup + await self.svc.DeleteLease( + name=self.name, + )packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
49-63: Consider using async context managers directly.Since
lease_acquisitionis already async, you could simplify by using async context managers (serve_unix_async,monitor_async,connect_async) directly instead of creating aBlockingPortaland running sync wrappers in a thread. This would reduce complexity and eliminate the thread boundary.Example refactor:
async def lease_acquisition(): nonlocal exit_code try: async with config.lease_async(selector, lease_name, duration) as lease: async with lease.serve_unix_async() as path: async with lease.monitor_async(): if exporter_logs: from contextlib import AsyncExitStack async with AsyncExitStack() as stack: async with lease.connect_async(stack) as client: async with client.log_stream(): exit_code = await anyio.to_thread.run_sync(shell_launcher, path) else: exit_code = await anyio.to_thread.run_sync(shell_launcher, path) except BaseExceptionGroup as eg: for exc in eg.exceptions: if isinstance(exc, TimeoutError): raise exc from None raise except get_cancelled_exc_class(): exit_code = 2Note: This requires verifying that
config.lease_asyncdoesn't require a portal parameter, or adjusting the signature accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (8)
packages/jumpstarter/jumpstarter/client/lease.py (2)
12-18: LGTM! Import formatting improved.The anyio imports are properly formatted and all are used within the file.
154-160: Cancellation shield correctly ensures cleanup completes.The addition of
CancelScope(shield=True)aroundDeleteLeaseensures that lease cleanup completes even when the task is cancelled, which directly addresses the PR objective of cleaning up leases on timeout and termination.packages/jumpstarter-cli/jumpstarter_cli/shell.py (6)
4-4: LGTM! New imports are properly used.The new anyio and signal handling imports are correctly utilized in the refactored async flow.
Also applies to: 6-6, 9-9
18-29: LGTM! Helper function correctly encapsulates shell launching.The closure properly captures configuration and command parameters for later execution.
32-41: LGTM! Context manager nesting is correct.The function properly orchestrates lease context managers and conditionally enables log streaming.
44-76: Async orchestration correctly implements signal-driven cancellation.The function properly coordinates lease acquisition, shell execution, signal handling, and cleanup with appropriate exit codes for different termination scenarios.
62-63: LGTM! Cancellation exception handling is correct.The handler properly catches backend-specific cancellation exceptions via
get_cancelled_exc_class()and sets the appropriate exit code for signal-driven termination.
105-108: LGTM! Integration with async flow is correct.The shell command properly delegates to the new async orchestration and exits with the appropriate code.
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
ebed8db to
33dc02b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
39-39: The cancellation exception handling is correct; past review comment is incorrect.Lines 39 and 56-57 correctly handle cancellation:
- Line 39:
get_cancelled_exc_class()returns the exception class itself (eitherasyncio.CancelledErrorortrio.Cancelleddepending on backend)- Line 56: The stored class is used directly in the
exceptclauseThis pattern is valid and equivalent to calling
get_cancelled_exc_class()directly in theexceptclause. The past review suggestingget_cancelled_exc_class()()is incorrect—the function returns a class, not a factory function.Based on learnings
Also applies to: 56-57
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (6)
packages/jumpstarter/jumpstarter/client/lease.py (2)
12-19: LGTM!The added imports (
CancelScope,fail_after) correctly support the cancellation-shielded cleanup logic introduced in the__asynccontextmanager__method.
147-163: Excellent cancellation-safe cleanup implementation!The changes ensure robust lease cleanup:
- Line 150: Moving
request_async()inside thetryblock guarantees cleanup runs even if acquisition fails- Lines 155-163:
CancelScope(shield=True)correctly prevents outer cancellation from interrupting theDeleteLeaseoperation- Line 158: The 30-second
fail_aftertimeout prevents indefinite blocking during cleanup- Lines 162-163: Appropriate warning on timeout—the server will eventually clean up orphaned leases
This aligns with anyio best practices for resource cleanup under cancellation.
Based on learnings
packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
4-4: LGTM!The new imports correctly support the async orchestration:
anyiofor the event loop,create_task_groupandget_cancelled_exc_classfor structured concurrency and cancellation handling, andsignal_handlerfor graceful shutdown.Also applies to: 6-6, 9-9
18-33: LGTM!The function correctly extracts the synchronous lease context management chain, preserving the original behavior while enabling integration with the async orchestration. The nested
launch_remote_shellhelper appropriately captures the path parameter.
36-64: Excellent async orchestration with proper signal handling!The function correctly implements structured concurrency with three exit paths:
- Normal exit (code 0): Shell completes successfully
- Timeout (code 1): Lease acquisition timeout (300s in
_acquire) extracted fromBaseExceptionGroupand caught byexcept* TimeoutError- Cancellation (code 2): Signal handler cancels the scope, raising a cancellation exception
The nested try-finally structure ensures the task group is always cancelled, preventing the signal handler task from lingering after normal completion.
Based on learnings
93-96: LGTM!The integration point correctly launches the async orchestration via
anyio.runand propagates the exit code tosys.exit, ensuring proper process termination with timeout (1), cancellation (2), or success (0) codes.
michalskrivanek
force-pushed
the
lease-cleanup
branch
2 times, most recently
from
c2f233a to
44038b4
Compare
michalskrivanek
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)packages/jumpstarter/jumpstarter/common/utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter/jumpstarter/common/utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
Manual verification of your patch since it's a bit complex to add to E2E.. |
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
f6e1dcd to
9f90775
Compare
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
9f90775 to
ec1a83b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
50-57: Plain TimeoutError will crash CLI; map it to exit code 1.A timeout during lease acquisition can raise a plain TimeoutError (not in an ExceptionGroup). It bypasses current handlers and propagates. Catch it explicitly before the cancellation branch.
except BaseExceptionGroup as eg: for exc in eg.exceptions: if isinstance(exc, TimeoutError): raise exc from None raise + except TimeoutError: + exit_code = 1 except cancelled_exc_class: exit_code = 2
🧹 Nitpick comments (6)
packages/jumpstarter/jumpstarter/config/client.py (1)
89-99: Set an explicit env prefix for nested Lease settings (consistency).Other nested settings (e.g., Drivers) define an env prefix. Consider adding one here so users can override via env (e.g., JMP_LEASES_ACQUISITION_TIMEOUT) and to avoid accidental global env binding.
class ClientConfigV1Alpha1Lease(BaseSettings): """Configuration for lease operations.""" + model_config = SettingsConfigDict(env_prefix="JMP_LEASES_") + acquisition_timeout: int = Field( default=300, description="Timeout in seconds for lease acquisition", ge=5, # Must be at least 5 seconds (polling interval) le=43200, # Maximum 12 hours )packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
18-34: Optional: avoid portal by using async lease APIs and offload only the shell.You could run serve_unix_async/monitor_async/log streaming directly in the event loop and use to_thread.run_sync just for launch_shell. Reduces cross-thread complexity.
Example sketch:
async with lease.serve_unix_async() as path, lease.monitor_async(): if exporter_logs: async with lease.connect_async(ExitStack()) as client, client.log_stream_async(): exit_code = await anyio.to_thread.run_sync(launch_remote_shell, path) else: exit_code = await anyio.to_thread.run_sync(launch_remote_shell, path)packages/jumpstarter/jumpstarter/client/lease.py (4)
50-52: LGTM: configurable acquisition timeout and exporter name.Good defaults and clear semantics. Minor: keep "remote" if exporter is empty.
- self.exporter_name = result.exporter + if result.exporter: + self.exporter_name = result.exporter
119-157: Robust acquisition loop with clear failure modes.Covers Ready/Unsatisfiable/Invalid/Released and times out via fail_after. Consider adaptive sleep to reduce overshoot near timeout (nice-to-have).
- await sleep(5) + # Sleep up to 5s but do not overshoot the remaining timeout budget + await sleep(min(5, max(0.1, self.acquisition_timeout)))
166-175: Don’t let cleanup errors (other than timeout) escape.DeleteLease errors during teardown could mask the original exception or interrupt shutdown. Log and continue after the shielded timeout.
with CancelScope(shield=True): try: with fail_after(30): await self.svc.DeleteLease( name=self.name, ) except TimeoutError: logger.warning("Timeout while deleting lease %s during cleanup", self.name) + except Exception as e: + logger.warning("Error while deleting lease %s during cleanup: %s", self.name, e)
153-156: Minor: prefer lazy logging formatting for consistency.Avoid eager f-strings in debug to skip formatting when disabled.
- logger.debug(f"Lease {self.name} acquisition timed out after {self.acquisition_timeout} seconds") + logger.debug("Lease %s acquisition timed out after %s seconds", self.name, self.acquisition_timeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/common/utils.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter/jumpstarter/common/utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter/jumpstarter/client/lease.py (3)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)packages/jumpstarter/jumpstarter/client/exceptions.py (1)
LeaseError(4-5)packages/jumpstarter/jumpstarter/client/grpc.py (1)
DeleteLease(383-389)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/signal.py (1)
signal_handler(9-22)packages/jumpstarter/jumpstarter/config/client.py (2)
lease(136-144)lease_async(248-285)packages/jumpstarter/jumpstarter/common/utils.py (1)
launch_shell(49-127)packages/jumpstarter/jumpstarter/client/lease.py (3)
serve_unix(237-239)monitor(242-244)connect(231-234)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (3)
JumpstarterException(4-39)get_config(32-33)ConnectionError(42-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter/jumpstarter/config/client.py (2)
122-123: LGTM: new leases section on client config.Adds a clear place for lease-related knobs; defaults are sane.
274-275: LGTM: timeout plumbed into Lease.Timeout is taken from config and enforced by Lease. Boundaries are already validated.
packages/jumpstarter-cli/jumpstarter_cli/shell.py (3)
45-49: LGTM: BlockingPortal usage to bridge the worker thread.Creating a portal bound to the running loop and passing it into lease_async is the right pattern for wrapping async CMs used from a worker thread.
41-43: LGTM: signal-driven cancellation via TaskGroup.signal_handler cancels the group; finally-block ensures cleanup. Good structured concurrency.
90-93: LGTM: anyio.run entry point and exit code propagation.Clean mapping of async result to process exit.
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
Outdated
Show resolved
Hide resolved
|
|
||
| await sleep(5) | ||
| except TimeoutError: | ||
| logger.debug(f"Lease {self.name} acquisition timed out after {self.acquisition_timeout} seconds") |
There was a problem hiding this comment.
| logger.debug(f"Lease {self.name} acquisition timed out after {self.acquisition_timeout} seconds") | |
| logger.error(f"Lease {self.name} acquisition timed out after {self.acquisition_timeout} seconds") |
There was a problem hiding this comment.
it will be obvious by the exception anyway
There was a problem hiding this comment.
i was trying to compare both debug and normal output and it makes IMHO more sense like this:
$ jmp --log-level=DEBUG shell -l device=ti-jacinto-j784s4xevm-22; echo $?
[10/01/25 13:16:34] DEBUG DEBUG:asyncio:Using selector: KqueueSelector selector_events.py:64
DEBUG DEBUG:grpc._cython.cygrpc:Using AsyncIOEngine.POLLER as I/O engine _channel.py:368
DEBUG DEBUG:jumpstarter.client.lease:Creating lease request for selector device=ti-jacinto-j784s4xevm-22 for duration 0:30:00 lease.py:61
[10/01/25 13:16:35] INFO INFO:jumpstarter.client.lease:Acquiring lease 01999f7d-066e-7538-8fff-b56da6c65d9e for selector device=ti-jacinto-j784s4xevm-22 for duration 0:30:00 lease.py:69
DEBUG DEBUG:jumpstarter.client.lease:Polling Lease 01999f7d-066e-7538-8fff-b56da6c65d9e lease.py:122
[10/01/25 13:16:40] DEBUG DEBUG:jumpstarter.client.lease:Lease 01999f7d-066e-7538-8fff-b56da6c65d9e acquisition timed out after 5 seconds lease.py:153
INFO INFO:jumpstarter.client.lease:Releasing Lease 01999f7d-066e-7538-8fff-b56da6c65d9e lease.py:165
Error: lease 01999f7d-066e-7538-8fff-b56da6c65d9e acquisition timed out after 5 seconds
1
and without debug:
$ jmp shell -l device=ti-jacinto-j784s4xevm-22; echo $?
[10/01/25 13:17:25] INFO INFO:jumpstarter.client.lease:Acquiring lease 01999f7d-ca41-78d7-a75f-ea5b15af5f6b for selector device=ti-jacinto-j784s4xevm-22 for duration 0:30:00 lease.py:69
[10/01/25 13:17:30] INFO INFO:jumpstarter.client.lease:Releasing Lease 01999f7d-ca41-78d7-a75f-ea5b15af5f6b lease.py:165
Error: lease 01999f7d-ca41-78d7-a75f-ea5b15af5f6b acquisition timed out after 5 seconds
1
where the "Error: lease...." is red. I.e. the end-user error message is already there, just through the LeaseError handler
| Args: | ||
| host: The jumpstarter host path | ||
| context: The context of the shell ("local" or "remote") | ||
| context: The context of the shell (e.g. "local" or exporter name) |
There was a problem hiding this comment.
Has this really haned?
I think it.s local/remote
There was a problem hiding this comment.
but it could make sense to set the exporter name instead of remote :D that would be cool
There was a problem hiding this comment.
This has been ideated by AI! 🌊
There was a problem hiding this comment.
so that's all contained in fairly small jumpstarter-dev/jumpstarter@62d1c9e
using the exporter name from when the lease becomes ready, propagated through so we do not have to fetch it again.
or do you mean anything else?
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
ec1a83b to
86d8885
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/signal.py (1)
signal_handler(9-22)packages/jumpstarter/jumpstarter/config/client.py (2)
lease(135-143)lease_async(247-284)packages/jumpstarter/jumpstarter/common/utils.py (1)
launch_shell(49-127)packages/jumpstarter/jumpstarter/client/lease.py (3)
serve_unix(237-239)monitor(242-244)connect(231-234)
packages/jumpstarter/jumpstarter/client/lease.py (3)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)packages/jumpstarter/jumpstarter/client/exceptions.py (1)
LeaseError(4-5)packages/jumpstarter/jumpstarter/client/grpc.py (1)
DeleteLease(383-389)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
86d8885 to
5e50735
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
164-174: Guard cleanup when no lease name was allocated.The cleanup unconditionally attempts to release the lease even when
self.nameis None (e.g., if lease creation fails or is cancelled before name assignment). This will log "Releasing Lease None" and callDeleteLease(name=None), potentially masking the original error.Based on past review comments, apply this guard to skip cleanup when no lease was created:
if self.release: - logger.info("Releasing Lease %s", self.name) - # Shield cleanup from cancellation to ensure it completes - with CancelScope(shield=True): - try: - with fail_after(30): - await self.svc.DeleteLease( - name=self.name, - ) - except TimeoutError: - logger.warning("Timeout while deleting lease %s during cleanup", self.name) + if not self.name: + logger.debug("Skipping lease cleanup; no lease was created") + else: + logger.info("Releasing Lease %s", self.name) + # Shield cleanup from cancellation to ensure it completes + with CancelScope(shield=True): + try: + with fail_after(30): + await self.svc.DeleteLease(name=self.name) + except TimeoutError: + logger.warning("Timeout while deleting lease %s during cleanup", self.name)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
87-87: Good fix: re‑raise the original ExceptionGroup.This addresses the earlier “bare raise outside except” concern noted in a past review.
🧹 Nitpick comments (5)
pyproject.toml (1)
27-30: Keep workspace sources alphabetizedPlease keep the driver list alphabetized to ease scanning. Swapping the two entries restores the existing ordering convention.
jumpstarter-driver-tasmota = { workspace = true } -jumpstarter-driver-tmt = { workspace = true } jumpstarter-driver-tftp = { workspace = true } +jumpstarter-driver-tmt = { workspace = true }packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (4)
22-30: Make leaf traversal non‑mutating and explicitly re‑raise the group.You re‑raise the original group unchanged, so don’t mutate leaf tracebacks here; also be explicit with
raise egfor clarity.Apply this diff:
- except BaseExceptionGroup as eg: - # Handle exceptions wrapped in ExceptionGroup (e.g., from task groups) - for exc in leaf_exceptions(eg): + except BaseExceptionGroup as eg: + # Handle exceptions wrapped in ExceptionGroup (e.g., from task groups) + for exc in leaf_exceptions(eg, fix_tracebacks=False): if isinstance(exc, JumpstarterException): raise ClickExceptionRed(str(exc)) from None elif isinstance(exc, click.ClickException): raise exc from None # If no handled exceptions, re-raise the original group - raise + raise eg
69-79: Drop boolean return contract; this function either raises or does nothing.
return Trueafter_handle_connection_error_with_reauth()is unreachable (it always raises). ReturningFalseelsewhere isn’t used reliably by the caller. Simplify to “raise or no‑op”.-def _handle_single_exception_with_reauth(exc, login_func): - """Handle a single exception and return True if handled.""" +def _handle_single_exception_with_reauth(exc, login_func): + """Handle a single exception (may raise).""" if isinstance(exc, ConnectionError): - _handle_connection_error_with_reauth(exc, login_func) - return True + _handle_connection_error_with_reauth(exc, login_func) elif isinstance(exc, JumpstarterException): raise ClickExceptionRed(str(exc)) from None elif isinstance(exc, click.ClickException): raise exc from None - return False # Not handled + # Not handled: fall through
81-88: ExceptionGroup handler: iterate non‑mutating leaves; remove unreachable return.The current
returnis dead code, and mutating leaf tracebacks isn’t needed since you re‑raise the original group when nothing matches.-def _handle_exception_group_with_reauth(eg, login_func): +def _handle_exception_group_with_reauth(eg, login_func): """Handle exceptions wrapped in BaseExceptionGroup.""" - for exc in leaf_exceptions(eg): - if _handle_single_exception_with_reauth(exc, login_func): - return # Exception was handled and re-raised + for exc in leaf_exceptions(eg, fix_tracebacks=False): + _handle_single_exception_with_reauth(exc, login_func) # If no handled exceptions, re-raise the original group raise egOptionally annotate this as NoReturn to aid type checkers (add import at top):
from typing import NoReturn def _handle_exception_group_with_reauth(eg, login_func) -> "NoReturn": ...
110-110: Update PEP link to PEP 654 Replace thepep-0785URL withhttps://peps.python.org/pep-0654/to reference the Exception Groups PEP.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py(6 hunks)packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/common/utils.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(3 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/common/utils.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
packages/jumpstarter-all/pyproject.toml
🧬 Code graph analysis (3)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/signal.py (1)
signal_handler(9-22)packages/jumpstarter/jumpstarter/config/client.py (2)
lease(135-143)lease_async(247-284)packages/jumpstarter/jumpstarter/common/utils.py (1)
launch_shell(49-127)packages/jumpstarter/jumpstarter/client/lease.py (3)
serve_unix(237-239)monitor(242-244)connect(231-234)
packages/jumpstarter/jumpstarter/client/lease.py (3)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)packages/jumpstarter/jumpstarter/client/exceptions.py (1)
LeaseError(4-5)packages/jumpstarter/jumpstarter/client/grpc.py (1)
DeleteLease(383-389)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (3)
JumpstarterException(4-39)get_config(32-33)ConnectionError(42-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (11)
packages/jumpstarter-all/pyproject.toml (1)
33-34: Thanks for wiring the new driver into the bundleThe meta package now pulls in
jumpstarter-driver-tmt, so consumers ofjumpstarter-allautomatically get the new functionality.packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
129-132: LGTM!The addition of the
--feeling-safeflag before--hard-rebootis correctly conditional onhard_reboot_cmdbeing truthy. This aligns with the updated test expectations.packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (1)
275-275: LGTM!Test expectations correctly updated to include the new
--feeling-safeflag before--hard-rebootacross all provision and run command test cases. The updates are consistent and align with the implementation changes.Also applies to: 288-288, 304-305, 318-318, 331-331, 356-356
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
217-218: LGTM!Test expectations correctly updated to include the new
leases.acquisition_timeoutfield with default value 7200 in the serialized YAML output. The updates are consistent across all three test cases.Also applies to: 258-259, 295-296
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
2-2: Pragmatic argv-based command detection for logging configuration.The sys.argv-based detection to conditionally disable RichHandler for the "run" command is a reasonable workaround given that ctx/params are not available at callback time. The comment clearly documents the constraint.
Also applies to: 13-19
packages/jumpstarter/jumpstarter/client/lease.py (4)
12-19: LGTM!The updated anyio imports include the necessary utilities (AsyncContextManagerMixin, CancelScope, fail_after, etc.) for the enhanced lease orchestration and timeout handling.
50-51: LGTM!The new fields are well-designed:
acquisition_timeoutmakes the timeout configurable with a reasonable 300s defaultexporter_nameis correctly marked withinit=Falsesince it's populated during acquisition
69-69: LGTM!The updated log message "Acquiring lease" is more accurate than "Created lease request" and better reflects the actual operation.
119-156: LGTM!The reworked acquisition loop introduces several improvements:
- Configurable timeout via
self.acquisition_timeoutwith clear error messaging- Increased polling interval to 5 seconds reduces server load
- Sets
exporter_nameon successful acquisition- Handles the "Invalid" condition explicitly
- Clean conversion of TimeoutError to LeaseError
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (2)
16-16: Docstring update looks good.Accurately reflects ExceptionGroup support in the async decorator.
58-66: Narrow expired token detection and emit to stderr
Bare matching on “expired” risks misclassifying certificate-or other expiry errors and prints to stdout. Restrict to both “token” and “expired” in the message and use stderr:--- a/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py:58 +++ b/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py:58 def _handle_connection_error_with_reauth(exc, login_func): """Handle ConnectionError with reauthentication logic.""" - if "expired" in str(exc).lower(): - click.echo(click.style("Token is expired, triggering re-authentication", fg="red")) + msg = str(exc).lower() + if "token" in msg and "expired" in msg: + click.secho("Token is expired, triggering re-authentication", fg="red", err=True) config = exc.get_config() login_func(config) raise ClickExceptionRed("Please try again now") from NonePlease confirm that all token-expiry errors include both “token” and “expired”; if not, add a dedicated
TokenExpiredexception and key off its type.
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
5e50735 to
93174e1
Compare
When lease is not available we try for 300s to acquire it. Users can also interrupt this with Ctrl+C while waiting. Let's remove such lease requests since after termination the shell is gone and it leaves behind a mostly useless lease.
should also result in immediate error, there's no chance the lease will ever be satisfied
when trying to acquire a lease it'suseful to know the lease id. Lease changes are not so frequent (and not that fast either), let's poll only once in 5s to save bandwidth a bit
it's useful to know where the user is connecting to instead of just "remote". Especially for lease selectors that can be satisfied by multiple exporters.
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
93174e1 to
32d728b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
162-165: Bug:types.TracebackType(...)called with keyword args — raisesTypeErrorat runtime
types.TracebackTypeis positional‑only; using keyword arguments will fail (“takes no keyword arguments”). Switch to positional arguments.- for frame, lasti, lineno in reversed(frames): - new_tb = types.TracebackType(tb_next=new_tb, tb_frame=frame, tb_lasti=lasti, tb_lineno=lineno) + for frame, lasti, lineno in reversed(frames): + new_tb = types.TracebackType(new_tb, frame, lasti, lineno)
♻️ Duplicate comments (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
58-67: Make re‑auth robust iflogin_funcis async; also guard config availabilityIf callers pass an async relogin function,
login_func(config)won’t run. Detect and run coroutine functions/objects (e.g., viaasyncio.run(...)), and handleconfig=Nonegracefully.Proposed minimal patch:
+import inspect +import asyncio @@ def _handle_connection_error_with_reauth(exc, login_func): @@ - config = exc.get_config() - login_func(config) + config = getattr(exc, "get_config", lambda: None)() # may be None + result = login_func(config) if config is not None else login_func() + # If the login func is async or returned a coroutine, run it + if inspect.iscoroutinefunction(login_func): + asyncio.run(login_func(config) if config is not None else login_func()) + elif inspect.iscoroutine(result): + asyncio.run(result) raise ClickExceptionRed("Please try again now") from NoneTo confirm whether any async
login_funcis passed today, please run:#!/bin/bash rg -nP -C2 'handle_exceptions_with_reauthentication\s*\(' rg -nP -C2 'async\s+def\s+(relogin|login)\w*\(|def\s+(relogin|login)\w*\('
🧹 Nitpick comments (7)
packages/jumpstarter/jumpstarter/client/lease.py (1)
119-156: Well-structured acquisition with comprehensive condition handling.The timeout enforcement via
fail_afterand the polling loop correctly handle all lease states (Ready, Unsatisfiable, Invalid, Released). The 5-second polling interval is appropriate for reducing server load. ConvertingTimeoutErrortoLeaseErrorwith a clear message aligns with user expectations.Minor spacing nit: Line 138 has an extra space after the opening parenthesis in
logger.debug(.Apply this diff to fix the spacing:
- logger.debug( "Lease %s is invalid: %s", self.name, message) + logger.debug("Lease %s is invalid: %s", self.name, message)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (4)
22-30: Consider usingexcept*or aggregating multiple leavesLooping leaves raises on the first handled leaf, potentially masking other errors in the group. Two options:
- Use
except* JumpstarterException/except* click.ClickExceptionto let Python split groups automatically.- Or aggregate handled leaf messages and present a concise summary before re‑raising the group when unhandled.
69-79: Unreachablereturn True; clarify contractFor handled cases you always raise; the “return True if handled” path is unreachable. Either (a) remove boolean returns and document that the function raises on handled exceptions, or (b) change the control flow to actually return without raising.
Minimal clean‑up:
def _handle_single_exception_with_reauth(exc, login_func): - """Handle a single exception and return True if handled.""" + """Handle a single exception; raises on handled cases, returns False otherwise.""" if isinstance(exc, ConnectionError): _handle_connection_error_with_reauth(exc, login_func) - return True elif isinstance(exc, JumpstarterException): raise ClickExceptionRed(str(exc)) from None elif isinstance(exc, click.ClickException): raise exc from None return False # Not handled
81-88: This handler never returns; type and comment it as such and drop deadreturnMake non‑returning intent explicit and remove the dead path.
+from typing import NoReturn @@ -def _handle_exception_group_with_reauth(eg, login_func): - """Handle exceptions wrapped in BaseExceptionGroup.""" - for exc in leaf_exceptions(eg): - if _handle_single_exception_with_reauth(exc, login_func): - return # Exception was handled and re-raised - # If no handled exceptions, re-raise the original group - raise eg +def _handle_exception_group_with_reauth(eg, login_func) -> NoReturn: + """Handle exceptions wrapped in BaseExceptionGroup. + Note: this function never returns; it always raises.""" + for exc in leaf_exceptions(eg): + # Will raise if handled; returns False otherwise + _handle_single_exception_with_reauth(exc, login_func) + raise eg # nothing was handled
98-101: Non‑returning helpers: add a brief note so readers know control flow stops hereSince both helpers raise on handled cases, add a short inline comment or
typing.NoReturnannotation to avoid confusion for future readers and type checkers.packages/jumpstarter-cli/jumpstarter_cli/shell.py (2)
50-54: Drop the ExceptionGroup special-casing (now likely dead code).With timeout handling centralized in lease acquisition (raising LeaseError), peeling out TimeoutError from BaseExceptionGroup is no longer needed and slightly obscures error flow. Let exceptions bubble to the decorator uniformly.
Apply this diff:
- except BaseExceptionGroup as eg: - for exc in eg.exceptions: - if isinstance(exc, TimeoutError): - raise exc from None - raise
18-34: Minor: reduce duplication around launching the shell.Small readability tweak to avoid duplicate returns.
def _run_shell_with_lease(lease, exporter_logs, config, command): """Run shell with lease context managers.""" def launch_remote_shell(path: str) -> int: return launch_shell( path, lease.exporter_name, config.drivers.allow, config.drivers.unsafe, config.shell.use_profiles, command=command ) with lease.serve_unix() as path: with lease.monitor(): - if exporter_logs: - with lease.connect() as client: - with client.log_stream(): - return launch_remote_shell(path) - else: - return launch_remote_shell(path) + if exporter_logs: + with lease.connect() as client, client.log_stream(): + exit_code = launch_remote_shell(path) + else: + exit_code = launch_remote_shell(path) + return exit_code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/common/utils.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/config/client_config_test.py
- packages/jumpstarter/jumpstarter/common/utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter/jumpstarter/client/lease.py (3)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)packages/jumpstarter/jumpstarter/client/exceptions.py (1)
LeaseError(4-5)packages/jumpstarter/jumpstarter/client/grpc.py (1)
DeleteLease(383-389)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (3)
JumpstarterException(4-39)get_config(32-33)ConnectionError(42-45)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/signal.py (1)
signal_handler(9-22)packages/jumpstarter/jumpstarter/config/client.py (2)
lease(135-143)lease_async(247-284)packages/jumpstarter/jumpstarter/common/utils.py (1)
launch_shell(49-127)packages/jumpstarter/jumpstarter/client/lease.py (3)
serve_unix(237-239)monitor(242-244)connect(231-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter/jumpstarter/client/lease.py (4)
12-19: LGTM! Clean anyio imports for structured concurrency.The imported primitives (
CancelScope,fail_after,sleep) are used correctly throughout the file for timeout handling and cleanup shielding.
50-51: LGTM! Well-documented fields with sensible defaults.The
acquisition_timeoutdefault (300s) aligns with the PR objectives, andexporter_namecorrectly usesinit=Falsesince it's populated during acquisition at line 127.
69-69: LGTM! More accurate log message."Acquiring lease" better reflects the state—the lease request is created but not yet ready. This aligns well with the subsequent polling loop.
164-174: Excellent fix! Cleanup now guards against missing lease name and shields from cancellation.The addition of
self.namecheck at line 164 directly addresses the past review concern—preventingDeleteLease(name=None)calls and misleading logs. TheCancelScope(shield=True)pattern is critical for the PR objective, ensuring lease cleanup completes even when interrupted (e.g., Ctrl+C), and the 30-second timeout with warning provides robustness against hung cleanup.packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
16-16: Docstring update reads wellClearer scope for async ExceptionGroup handling. LGTM.
packages/jumpstarter-cli/jumpstarter_cli/shell.py (2)
36-61: Cancellation flow and lease cleanup look solid.Signal-driven cancellation via task group scope, catching the AnyIO cancellation class and mapping to exit code 2, should release the lease cleanly on SIGINT/SIGTERM. LGTM.
36-61: No plain TimeoutError catches in CLI code
Search across packages/jumpstarter-cli found no directTimeoutErrorhandlers; remaining catches inlease.py,grpc.py, anddriver/base.pyreside in their respective layers and wrap timeouts as intended.
|
addressed most nitpicks, let's see now.... |
|
I will test the new changes asap |
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)
packages/jumpstarter/jumpstarter/config/client.py (1)
56-61: Attach config on expired-token errors using a case‑insensitive check.This decorator sets the config only if the exact phrase “token is expired” matches. Make it case‑insensitive and robust so reauth consistently works downstream.
- except ConnectionError as e: - if "token is expired" in str(e): + except ConnectionError as e: + if "expired" in str(e).lower(): # args[0] should be self for instance methods e.set_config(args[0]) raise e
🧹 Nitpick comments (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
151-165: Building TracebackType objects is implementation‑specific; add a safe fallback.Directly instantiating types.TracebackType can be fragile across Python implementations. Recommend guarding construction and falling back to the original tb chain if creation fails.
- # Add frames from tb1 to the beginning (in reverse order) - for frame, lasti, lineno in reversed(frames): - new_tb = types.TracebackType(tb_next=new_tb, tb_frame=frame, tb_lasti=lasti, tb_lineno=lineno) + # Add frames from tb1 to the beginning (in reverse order) + for frame, lasti, lineno in reversed(frames): + try: + new_tb = types.TracebackType(tb_next=new_tb, tb_frame=frame, tb_lasti=lasti, tb_lineno=lineno) + except Exception: + # Fallback: if constructing a synthetic traceback is not supported, return tb2 as-is + return tb2packages/jumpstarter/jumpstarter/config/client.py (1)
89-97: Enable env overrides for leases config (consistent with drivers.*).Drivers has JMP_DRIVERS_ env prefix; consider the same for leases so users can do JMP_LEASES_ACQUISITION_TIMEOUT=… without editing YAML.
-class ClientConfigV1Alpha1Lease(BaseSettings): +class ClientConfigV1Alpha1Lease(BaseSettings): + model_config = SettingsConfigDict(env_prefix="JMP_LEASES_")packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
47-50: Make the worker thread cancellable.Ensure Ctrl+C (task-group cancel) promptly aborts the blocking shell runner.
- exit_code = await anyio.to_thread.run_sync( - _run_shell_with_lease, lease, exporter_logs, config, command - ) + exit_code = await anyio.to_thread.run_sync( + _run_shell_with_lease, lease, exporter_logs, config, command, + cancellable=True + )packages/jumpstarter/jumpstarter/client/lease.py (1)
138-138: Minor formatting: extra space afterlogger.debug(.There's an extra space between
logger.debug(and the opening quote.Apply this diff to fix the formatting:
- logger.debug( "Lease %s is invalid: %s", self.name, message) + logger.debug("Lease %s is invalid: %s", self.name, message)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/common/utils.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter/jumpstarter/common/utils.py
- packages/jumpstarter/jumpstarter/config/client_config_test.py
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (3)
JumpstarterException(4-39)get_config(32-33)ConnectionError(42-45)
packages/jumpstarter/jumpstarter/client/lease.py (3)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)packages/jumpstarter/jumpstarter/client/exceptions.py (1)
LeaseError(4-5)packages/jumpstarter/jumpstarter/client/grpc.py (1)
DeleteLease(383-389)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (10)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
23-31: Async EG handling looks solid.Flattening EG leaves and mapping Jumpstarter/Click exceptions to CLI errors is correct; re-raising the original group preserves non-handled failures.
packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
18-34: Lease-run orchestration is clean.The serve_unix/monitor/log_stream layering is tidy and keeps the lease context scoped around the shell execution.
41-60: Signal-driven cancellation path LGTM.Using get_cancelled_exc_class with a task group + signal handler is correct; the final cancel ensures teardown.
90-92: Entrypoint integration looks good.anyio.run wrapper cleanly isolates async flow; exit_code propagation is clear.
50-55: TimeoutError propagation: no plain TimeoutError escapes
Search confirms the only plainTimeoutErrorin the CLI package is in this snippet and all upstream timeouts are converted to custom exceptions.packages/jumpstarter/jumpstarter/client/lease.py (5)
12-19: LGTM! Clean anyio imports for structured concurrency.The imports correctly bring in the necessary anyio primitives for timeout enforcement (
fail_after), cancellation shielding (CancelScope), and async sleep. This aligns with anyio's structured concurrency model.
50-51: LGTM! Well-structured field additions.The
acquisition_timeoutfield with a 5-minute default provides sensible behavior for lease polling, andexporter_namecorrectly usesinit=Falsesince it's populated during the acquisition phase.
69-69: LGTM! Clearer progress messaging.The updated log message "Acquiring lease" better reflects the active acquisition process compared to the previous "Created lease request" wording.
119-156: LGTM! Robust acquisition logic with proper timeout handling.The refactored
_acquire()method correctly:
- Enforces the configured timeout using
fail_after()- Polls at a reasonable 5-second interval to reduce server load
- Handles all lease condition states (Ready, Unsatisfiable, Invalid, Pending, Released)
- Converts timeout to a user-friendly
LeaseErrorwith clear messaging- Populates
exporter_nameon successful acquisitionThe use of
from Noneto suppress exception chaining is appropriate here since the timeout is being translated into a domain-specific exception.
164-174: LGTM! Robust cleanup with proper cancellation shielding.The cleanup logic correctly:
- Guards deletion by checking
self.nameexists (addressing the previous review concern)- Uses
CancelScope(shield=True)to ensure cleanup completes even if the parent scope is cancelled- Enforces a 30-second timeout on cleanup to prevent hanging
- Logs a warning (rather than raising) if cleanup times out, which is appropriate for best-effort cleanup
This follows anyio structured concurrency best practices for resource cleanup.
| def _handle_connection_error_with_reauth(exc, login_func): | ||
| """Handle ConnectionError with reauthentication logic.""" | ||
| if "expired" in str(exc).lower(): | ||
| click.echo(click.style("Token is expired, triggering re-authentication", fg="red")) | ||
| config = exc.get_config() | ||
| login_func(config) | ||
| raise ClickExceptionRed("Please try again now") from None | ||
| else: | ||
| raise ClickExceptionRed(str(exc)) from None |
There was a problem hiding this comment.
Reauth can fail if config wasn’t attached; harden and align token check.
If get_config() returns None (e.g., upstream didn’t tag the exception), login_func(config) may fail. Also, this block checks "expired" case-insensitively, while the upstream decorator checks "token is expired" case-sensitively, risking missed tagging.
Suggested changes:
- Gracefully handle missing config (fallback message).
- Keep the case-insensitive check consistent across layers.
def _handle_connection_error_with_reauth(exc, login_func):
- if "expired" in str(exc).lower():
+ if "expired" in str(exc).lower():
click.echo(click.style("Token is expired, triggering re-authentication", fg="red"))
- config = exc.get_config()
- login_func(config)
+ config = exc.get_config()
+ if config is None:
+ # No config attached; instruct user to login explicitly.
+ raise ClickExceptionRed("Authentication expired. Please run 'jmp login' and retry.") from None
+ login_func(config)
raise ClickExceptionRed("Please try again now") from None
else:
raise ClickExceptionRed(str(exc)) from NonePair with the client-side change below to ensure config is always attached.
📝 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.
| def _handle_connection_error_with_reauth(exc, login_func): | |
| """Handle ConnectionError with reauthentication logic.""" | |
| if "expired" in str(exc).lower(): | |
| click.echo(click.style("Token is expired, triggering re-authentication", fg="red")) | |
| config = exc.get_config() | |
| login_func(config) | |
| raise ClickExceptionRed("Please try again now") from None | |
| else: | |
| raise ClickExceptionRed(str(exc)) from None | |
| def _handle_connection_error_with_reauth(exc, login_func): | |
| """Handle ConnectionError with reauthentication logic.""" | |
| if "expired" in str(exc).lower(): | |
| click.echo(click.style("Token is expired, triggering re-authentication", fg="red")) | |
| config = exc.get_config() | |
| if config is None: | |
| # No config attached; instruct user to login explicitly. | |
| raise ClickExceptionRed("Authentication expired. Please run 'jmp login' and retry.") from None | |
| login_func(config) | |
| raise ClickExceptionRed("Please try again now") from None | |
| else: | |
| raise ClickExceptionRed(str(exc)) from None |
🤖 Prompt for AI Agents
In packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py around
lines 59 to 67, the reauth path assumes exc.get_config() always returns a config
and uses a loose "expired" check; update it to check for the upstream message
case-insensitively by testing if "token is expired" appears in str(exc).lower(),
then call exc.get_config() and if config is None avoid calling
login_func(config) directly — instead print a clear fallback error message and
raise ClickExceptionRed asking the user to reauth manually; if config exists,
invoke login_func(config) inside a try/except and on any failure raise
ClickExceptionRed with the caught error message so reauth failures are surfaced
cleanly.
| tls_config=self.tls, | ||
| grpc_options=self.grpcOptions, | ||
| acquisition_timeout=self.leases.acquisition_timeout, | ||
| ) as lease: |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm Lease(...) supports acquisition_timeout.
Wiring looks right; verify the Lease constructor accepts acquisition_timeout and that units (seconds) match its API.
🏁 Script executed:
#!/bin/bash
# Verify Lease.__init__ signature and usages for acquisition_timeout (seconds)
rg -nC2 -P 'class\s+Lease\b' packages
rg -nC3 -P '__init__\s*\(.*acquisition_timeout' packages
rg -nC2 -P 'acquisition_timeout\s*=' packagesLength of output: 1187
Fix parameter mismatch: Lease.__init__ only accepts channel and duration, not acquisition_timeout. Either pass the timeout as duration=self.leases.acquisition_timeout (wrapped in a timedelta) or extend the Lease constructor to accept an acquisition_timeout parameter.
🤖 Prompt for AI Agents
In packages/jumpstarter/jumpstarter/config/client.py around lines 271 to 274,
the code passes acquisition_timeout to Lease.__init__ but Lease only accepts
channel and duration; replace the acquisition_timeout kwarg by passing
duration=timedelta(seconds=self.leases.acquisition_timeout) (or appropriate
units) when constructing Lease, or alternatively modify the Lease class
signature to accept acquisition_timeout and map it internally to duration;
ensure imports include datetime.timedelta if using the timedelta approach and
remove the unsupported acquisition_timeout argument from the call.
increase the default time we want for lease to become availble to 2h. Add new client config for timeout on lease acquisition: leases: acquisition_timeout: 7200 Also print a clear error message when timeout happens.
michalskrivanek
force-pushed
the
lease-cleanup
branch
from
32d728b to
f20fd63
Compare
|
Verified including config changes. |
|
Successfully created backport PR for |
2 participants
When lease is not available we try for 300s to acquire it. Users can also iterrupt this with Ctrl+C while waiting. Let's remove such lease requests since after termination the shell is gone and it leaves behind a mostly useless lease.
Fixes-Issue: jumpstarter-dev/jumpstarter#119
Fixes-Issue: jumpstarter-dev/jumpstarter#120
Summary by CodeRabbit
New Features
Bug Fixes
Documentation