acquisition timeout support and humanized durations#740
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds an acquisition-timeout end-to-end: enhanced duration parsing with minimum enforcement, a new CLI option Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (shell)
participant Parser as DurationParamType
participant Handler as _shell_with_signal_handling
participant Config as ClientConfigV1Alpha1
participant Lease as Lease
CLI->>Parser: parse --acquisition-timeout value
Parser-->>CLI: returns timedelta (enforces minimum 5s)
CLI->>Handler: shell(..., acquisition_timeout=td)
activate Handler
Handler->>Config: lease_async(..., acquisition_timeout=td)
activate Config
Config->>Config: compute acquisition_timeout_seconds\n(arg or config default)
Config->>Lease: new Lease(..., acquisition_timeout_seconds)
Lease-->>Config: Lease instance
Config-->>Handler: lease result
deactivate Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-15T08:18:48.571ZApplied to files:
🧬 Code graph analysis (1)packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
⏰ 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)
🔇 Additional comments (8)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
36-38: Consider adding type annotations to function parameters.For consistency with the rest of the codebase and improved IDE support, consider adding type hints to all parameters in
_shell_with_signal_handling.Apply this diff to add type annotations:
async def _shell_with_signal_handling( - config, selector, lease_name, duration, exporter_logs, command, acquisition_timeout + config: ClientConfigV1Alpha1, + selector: str, + lease_name: str | None, + duration: timedelta, + exporter_logs: bool, + command: tuple[str, ...], + acquisition_timeout: timedelta | None, ):packages/jumpstarter/jumpstarter/config/client.py (1)
267-272: Consider adding inline documentation for the new parameter.Adding a docstring or inline comment explaining the acquisition_timeout behavior would help future maintainers understand the fallback logic.
Example addition above the method:
@asynccontextmanager async def lease_async( self, selector: str, lease_name: str | None, duration: timedelta, portal: BlockingPortal, acquisition_timeout: timedelta | None = None, ): """ Acquire a lease asynchronously. Args: acquisition_timeout: Optional timeout for lease acquisition. If None, uses the configured default from self.leases.acquisition_timeout. """
📜 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 (4)
packages/jumpstarter-cli/jumpstarter_cli/common.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(5 hunks)packages/jumpstarter-cli/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.788Z
Learning: In the Jumpstarter client lease model (packages/jumpstarter/jumpstarter/client/grpc.py), `effective_duration` represents the elapsed time for an active lease so far, not the total duration. To calculate expected release time, use `effective_begin_time + duration` (where `duration` is the configured/requested duration), not `effective_begin_time + effective_duration`.
Applied to files:
packages/jumpstarter/jumpstarter/config/client.py
📚 Learning: 2025-11-05T13:45:58.271Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:15-15
Timestamp: 2025-11-05T13:45:58.271Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, pexpect is intentionally used as a transitive dependency through the jumpstarter-driver-pyserial package. The flashers package does not declare pexpect as a direct dependency because the pyserial driver package is intended to control the pexpect version.
Applied to files:
packages/jumpstarter-cli/pyproject.toml
🧬 Code graph analysis (1)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (3)
packages/jumpstarter/jumpstarter/config/client.py (2)
lease_async(252-296)lease(135-143)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
handle_exceptions_with_reauthentication(89-106)packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)
⏰ 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: build
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter-cli/pyproject.toml (1)
16-16: LGTM!The pytimeparse2 dependency addition properly supports the new human-readable duration parsing feature introduced in the CLI.
packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
36-38: LGTM!The acquisition_timeout parameter is correctly threaded through to the lease acquisition flow.
Also applies to: 48-48
packages/jumpstarter/jumpstarter/config/client.py (1)
258-258: LGTM!The acquisition_timeout parameter is correctly added with proper type hints, and the conversion logic appropriately falls back to the configured default when not specified.
Also applies to: 267-272, 285-285
packages/jumpstarter-cli/jumpstarter_cli/common.py (2)
19-21: LGTM!The enhanced duration parsing implementation is robust with multiple input formats (timedelta, int, string), clear error messages, and proper minimum validation. Backward compatibility is maintained.
Also applies to: 23-65
108-120: LGTM!The acquisition timeout option is properly configured with a sensible 5-second minimum and clear help text.
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 (1)
packages/jumpstarter-cli/jumpstarter_cli/common.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
🪛 GitHub Actions: Lint
packages/jumpstarter-cli/jumpstarter_cli/common.py
[error] 50-50: ruff check failed with E501: Line too long (128 > 120) on line 50. Command: ruff check /home/runner/work/jumpstarter/jumpstarter
🪛 GitHub Check: ruff
packages/jumpstarter-cli/jumpstarter_cli/common.py
[failure] 50-50: Ruff (E501)
packages/jumpstarter-cli/jumpstarter_cli/common.py:50:121: E501 Line too long (128 > 120)
⏰ 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 (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (5)
packages/jumpstarter-cli/jumpstarter_cli/common.py (5)
6-6: LGTM: pytimeparse2 integration.The import enables human-readable duration parsing as described in the PR objectives.
19-21: LGTM: Minimum duration support.The addition of a configurable minimum parameter enables validation for use cases like acquisition timeout where a floor is required.
23-28: LGTM: Backward-compatible handling of timedelta and int.Preserving int-as-seconds maintains compatibility while the new code variable enables downstream minimum validation.
54-68: LGTM: Type validation and minimum enforcement.The minimum check (lines 61-66) correctly enforces the floor when specified, and the error message clearly communicates the requirement in seconds.
78-87: LGTM: Documentation and acquisition timeout option.The updated help text clearly describes supported formats with examples, and the new
ACQUISITION_TIMEOUTconstant with a 5-second minimum is appropriate for preventing overly aggressive timeout values.Also applies to: 113-125
mangelajo
force-pushed
the
acquisition-timeout
branch
from
fd581e2 to
ced145a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/config/client.py (1)
134-143: Consider adding acquisition_timeout to synchronous API for consistency.The synchronous
lease()method doesn't expose theacquisition_timeoutparameter, creating an API inconsistency withlease_async(). Users calling the synchronous wrapper cannot override the timeout per call.If desired, apply this diff to add the parameter:
def lease( self, selector: str | None = None, lease_name: str | None = None, duration: timedelta = timedelta(minutes=30), + acquisition_timeout: timedelta | None = None, ): with start_blocking_portal() as portal: - with portal.wrap_async_context_manager(self.lease_async(selector, lease_name, duration, portal)) as lease: + with portal.wrap_async_context_manager( + self.lease_async(selector, lease_name, duration, portal, acquisition_timeout) + ) as lease: yield lease
📜 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 (4)
packages/jumpstarter-cli/jumpstarter_cli/common.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(5 hunks)packages/jumpstarter-cli/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-cli/pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.788Z
Learning: In the Jumpstarter client lease model (packages/jumpstarter/jumpstarter/client/grpc.py), `effective_duration` represents the elapsed time for an active lease so far, not the total duration. To calculate expected release time, use `effective_begin_time + duration` (where `duration` is the configured/requested duration), not `effective_begin_time + effective_duration`.
Applied to files:
packages/jumpstarter/jumpstarter/config/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
packages/jumpstarter/jumpstarter/config/client.py (2)
lease_async(252-296)lease(135-143)
⏰ 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: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
🔇 Additional comments (10)
packages/jumpstarter/jumpstarter/config/client.py (2)
89-96: LGTM! Configuration looks solid.The
ClientConfigV1Alpha1Leaseclass properly defines the acquisition timeout with sensible defaults (2 hours) and minimum constraints (5 seconds) that align with the CLI validation.
258-258: LGTM! Acquisition timeout wiring is correct.The parameter addition and conversion logic properly handles the timedelta-to-seconds conversion and falls back to the config default when not specified.
Also applies to: 267-272, 285-285
packages/jumpstarter-cli/jumpstarter_cli/shell.py (2)
11-11: LGTM! Import and signature update are correct.The acquisition timeout parameter is properly imported and added to the function signature.
Also applies to: 36-38
48-48: LGTM! Parameter wiring is complete and correct.The acquisition timeout flows properly from the CLI decorator through the call chain to the lease context.
Also applies to: 75-75, 78-78, 94-101
packages/jumpstarter-cli/jumpstarter_cli/common.py (6)
6-6: LGTM! Import is correct.The pytimeparse2 import enables human-readable duration parsing.
19-21: LGTM! Minimum parameter addition is clean.The
__init__method properly stores the optional minimum constraint.
23-63: LGTM! Three-tier parsing logic is sound.The parsing strategy maintains backward compatibility (int strings) while adding human-readable format support (pytimeparse2) and ISO 8601 fallback (pydantic). The line-length issue from the previous review has been resolved by splitting the error message.
64-71: LGTM! Minimum validation is correct.The validation properly checks against the minimum constraint and provides a clear error message.
85-90: LGTM! Help text is clear and comprehensive.The updated documentation clearly describes all supported duration formats with examples and a reference to the pytimeparse2 documentation.
116-128: LGTM! Acquisition timeout option is properly configured.The
ACQUISITION_TIMEOUTtype with a 5-second minimum aligns with the configuration validation inclient.py, and the CLI option is clearly documented.
mangelajo
changed the title
mangelajo
changed the title
|
i feel like |
|
I think it’s helpful to let us specify dynamically from a task. :-) |
I will split the humanized duration and the new flag, what do you think of a flag for create lease, one for waiting and another one to specify the max wait? |
I found a use case today where I missed it... for x in 01 02 03 04 05; do jmp shell -l device=qride-$x --acquisition-timeout 30s ./qcom-info.py; done |
|
Thank you benny |
mangelajo
force-pushed
the
acquisition-timeout
branch
from
9f15a9f to
9393bda
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/config/client.py (1)
135-143: Consider extending the synchronous lease() method for API consistency.The synchronous
lease()method doesn't accept anacquisition_timeoutparameter, so callers using this wrapper will always use the configured default. For API consistency, consider adding the parameter and passing it through tolease_async().Apply this diff to add the parameter:
@contextmanager def lease( self, selector: str | None = None, lease_name: str | None = None, duration: timedelta = timedelta(minutes=30), + acquisition_timeout: timedelta | None = None, ): with start_blocking_portal() as portal: - with portal.wrap_async_context_manager(self.lease_async(selector, lease_name, duration, portal)) as lease: + with portal.wrap_async_context_manager( + self.lease_async(selector, lease_name, duration, portal, acquisition_timeout) + ) as lease: yield lease
📜 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 (4)
packages/jumpstarter-cli/jumpstarter_cli/common.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(5 hunks)packages/jumpstarter-cli/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-cli/pyproject.toml
- packages/jumpstarter-cli/jumpstarter_cli/shell.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.788Z
Learning: In the Jumpstarter client lease model (packages/jumpstarter/jumpstarter/client/grpc.py), `effective_duration` represents the elapsed time for an active lease so far, not the total duration. To calculate expected release time, use `effective_begin_time + duration` (where `duration` is the configured/requested duration), not `effective_begin_time + effective_duration`.
Applied to files:
packages/jumpstarter/jumpstarter/config/client.py
📚 Learning: 2025-09-15T08:18:48.571Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:488-491
Timestamp: 2025-09-15T08:18:48.571Z
Learning: In the jumpstarter project, code review suggestions should stay focused on the specific scope of the PR. Suggestions about general improvements like timeout handling or error handling that are unrelated to the core changes being made should be avoided, even if they apply to modified code lines.
Applied to files:
packages/jumpstarter-cli/jumpstarter_cli/common.py
🔇 Additional comments (7)
packages/jumpstarter/jumpstarter/config/client.py (3)
258-258: LGTM! Clean signature extension.The optional
acquisition_timeoutparameter integrates well with the existing lease_async signature and enables callers to override the configured timeout on a per-call basis.
267-272: LGTM! Correct conversion and fallback logic.The conditional conversion from timedelta to seconds with fallback to the configured default is well-implemented. The minimum validation (≥5 seconds) is appropriately enforced in the CLI and model layers.
285-285: LGTM! Correct parameter passing.The computed
acquisition_timeout_secondsvalue is correctly passed to the Lease constructor.packages/jumpstarter-cli/jumpstarter_cli/common.py (4)
6-6: LGTM! Import enables humanized duration parsing.The pytimeparse2 import provides the enhanced duration parsing capability that supports human-readable formats like "1h30m" and "3h40m".
19-21: LGTM! Clean implementation of minimum validation support.The
__init__method properly stores the optional minimum duration constraint, enabling enforcement in theconvertmethod.
23-71: LGTM! Comprehensive duration parsing with backward compatibility.The enhanced
convertmethod implements a robust multi-level parsing strategy:
- Direct timedelta pass-through
- Integer as seconds (backward compatibility)
- String parsing: plain integer → pytimeparse2 → pydantic/ISO 8601
- Minimum validation enforcement
The fallback chain maintains backward compatibility while adding rich human-readable format support. Error messages are clear and provide helpful examples.
116-128: LGTM! Well-defined acquisition timeout option.The new
ACQUISITION_TIMEOUTconstant andopt_acquisition_timeoutoption are cleanly implemented:
- Minimum of 5 seconds aligns with the model-layer constraint
- Clear help text with examples and requirement documentation
- Default of None enables fallback to configured timeout
- Consistent with existing CLI option patterns
| Human-readable: 30m, 3h30m, 1d, 1d3h40m, etc. | ||
| ISO 8601: PT1H30M, P1DT2H30M, etc. | ||
| Time format: 01:30:00, 2 days, 01:30:00, etc. | ||
|
|
||
| See https://docs.rs/speedate/latest/speedate/ for details | ||
| See https://github.com/wroberts/pytimeparse2 for details | ||
| """, |
There was a problem hiding this comment.
Clarify the time format examples.
Line 87 reads "Time format: 01:30:00, 2 days, 01:30:00, etc." which appears to list "01:30:00" twice. Consider clarifying whether these are meant as:
- Two examples: "01:30:00" and "2 days, 01:30:00" (compound format)
- Or separate examples with an accidental duplicate
Suggested clarification:
-Time format: 01:30:00, 2 days, 01:30:00, etc.
+Time format: 01:30:00, or compound like '2 days, 01:30:00', etc.🤖 Prompt for AI Agents
In packages/jumpstarter-cli/jumpstarter_cli/common.py around lines 85 to 90, the
"Time format" examples are ambiguous and currently show "01:30:00" twice; update
the docstring to remove the duplicate and clarify intended examples (either list
separate examples like "01:30:00" and "2 days, 01:30:00" or show a compound
example "2 days, 01:30:00") so the examples are unambiguous and consistent with
the other formats.
Pull Request is not mergeable
Pull Request is not mergeable
|
the tests did not run for some reason, outages? |
mangelajo
force-pushed
the
acquisition-timeout
branch
from
9393bda to
2461a69
Compare
|
Just bumped the commit message to see if tests would trigger now. |
2 participants
now all this is possible:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation