feat: lease sharing - #942
Open
bennyz wants to merge 2 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds lease sharing with policy-based authorization across Go and Python APIs, clients, and CLIs. It also adds asynchronous fan-out streams with exclusive and observer modes, serial observe support, console token controls, buffering, reconnects, and status reporting. ChangesLease sharing
Serial fan-out
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientService
participant Lease
participant AccessPolicy
Client->>ClientService: update shared clients
ClientService->>Lease: check ownership and lease state
ClientService->>AccessPolicy: validate exporter and client labels
AccessPolicy-->>ClientService: authorization result
ClientService->>Lease: persist SharedWith
Lease-->>Client: return updated lease
sequenceDiagram
participant SerialClient
participant PySerial
participant StreamFanOut
participant ObserverStream
SerialClient->>PySerial: start console with observe
PySerial->>StreamFanOut: open observe stream
StreamFanOut->>ObserverStream: attach read-only observer
StreamFanOut-->>ObserverStream: forward serial output
SerialClient->>PySerial: release console or request status
PySerial->>StreamFanOut: release token or report status
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
bennyz
force-pushed
the
lease-sharing
branch
2 times, most recently
from
August 3, 2026 09:36
7d0ceee to
49e8dc0
Compare
Member
|
We will need to fix the serial multi-reader :D and may be other streams too (like the Ble ..) . :) |
bennyz
force-pushed
the
lease-sharing
branch
2 times, most recently
from
August 5, 2026 12:32
53bbaa1 to
90c1379
Compare
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
555-559: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd functional lease-sharing coverage.
The current tests only verify default
Nonearguments and a table header. They do not protect the new sharing contract.
python/packages/jumpstarter/jumpstarter/client/grpc_test.py#L555-L559: test protobuf deserialization, Rich row rendering, CreateLease serialization, and sharing-only UpdateLease requests.python/packages/jumpstarter/jumpstarter/config/client_config_test.py#L419-L450: pass a non-emptyshared_withlist and assert forwarding toClientService.CreateLease.python/packages/jumpstarter-cli/jumpstarter_cli/create_test.py#L11-L42: test--share alice,bobforwarding and malformed comma-separated input.python/packages/jumpstarter-cli/jumpstarter_cli/share.py#L16-L88: add tests forshare add,share remove, andshare list, including empty and missing lease results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter/jumpstarter/client/grpc_test.py` around lines 555 - 559, Expand lease-sharing test coverage: in python/packages/jumpstarter/jumpstarter/client/grpc_test.py:555-559, cover protobuf deserialization, Rich row rendering, CreateLease serialization, and sharing-only UpdateLease requests; in python/packages/jumpstarter/jumpstarter/config/client_config_test.py:419-450, pass a non-empty shared_with list and assert it reaches ClientService.CreateLease; in python/packages/jumpstarter-cli/jumpstarter_cli/create_test.py:11-42, test --share alice,bob forwarding and malformed comma-separated input; and in python/packages/jumpstarter-cli/jumpstarter_cli/share.py:16-88, add tests for share add, share remove, and share list, including empty and missing lease results.Source: Coding guidelines
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
214-244: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winObserve mode still enables stdin when stdin is piped.
The validation covers
--inputand--no-outputonly. If the user runscat cmds.txt | j serial pipe --observe,input_flagisNoneandno_inputisFalse, soinput_enabledbecomesTrueat Line 241._pipe_serialthen starts_stdin_to_serialon the observer stream, and the firstsendraisesReadOnlyStreamError. Force read-only whenobserveis set.🐛 Proposed fix
# Determine if input should be enabled - if no_input: + if observe or no_input: input_enabled = False elif input_flag: input_enabled = True else: input_enabled = stdin_is_piped🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py` around lines 214 - 244, Update the input selection logic around observe, input_enabled, and stdin_is_piped so observe mode always forces input_enabled to False, regardless of piped stdin or --input. Preserve the existing no_input, input_flag, and auto-detection behavior for non-observe mode.
🧹 Nitpick comments (9)
python/packages/jumpstarter/jumpstarter/streams/fanout_test.py (1)
103-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused helpers.
No test calls
_make_memory_sourceor_memory_source_factory. Every test defines a localfactory. Delete both helpers, or use them to remove the repeated factory setup in each test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter/jumpstarter/streams/fanout_test.py` around lines 103 - 113, Remove the unused _make_memory_source and _memory_source_factory helpers from fanout_test.py, since tests already define local factory functions. Do not alter the existing test-local setup or behavior.python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.py (1)
54-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two stdin readers.
__stdin_exit_onlyand__stdin_to_serialdiffer only by the finalawait stream.send(data). Use one method that takes an optional stream and forwards bytes only when the stream is present. This keeps the Ctrl-B exit sequence in one place.♻️ Proposed refactor
- async def __stdin_exit_only(self): - stdin = FileReadStream(sys.stdin.buffer) - ctrl_b_count = 0 - while True: - data = await stdin.receive(max_bytes=1) - if not data: - continue - if data == b"\x02": - ctrl_b_count += 1 - if ctrl_b_count == 3: - raise ConsoleExit - else: - ctrl_b_count = 0 - - async def __stdin_to_serial(self, stream): + async def __stdin_to_serial(self, stream=None): stdin = FileReadStream(sys.stdin.buffer) ctrl_b_count = 0 while True: data = await stdin.receive(max_bytes=1) if not data: continue if data == b"\x02": # Ctrl-B ctrl_b_count += 1 if ctrl_b_count == 3: raise ConsoleExit else: ctrl_b_count = 0 - await stream.send(data) + if stream is not None: + await stream.send(data)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.py` around lines 54 - 66, Merge __stdin_exit_only and __stdin_to_serial into a single stdin-reading method that accepts an optional output stream. Keep the existing Ctrl-B counting and ConsoleExit behavior in that method, and forward each received byte only when the optional stream is present; update callers to use this unified method.controller/internal/service/controller_service.go (1)
830-834: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the log message for shared access.
The check now accepts shared clients, but the message still says "lease not held by client". Change it to state that the lease is not accessible by the client.
♻️ Proposed change
if !lease.IsAccessibleBy(client.Name) { err := fmt.Errorf("permission denied") - logger.Error(err, "lease not held by client") + logger.Error(err, "lease not accessible by client") return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/service/controller_service.go` around lines 830 - 834, Update the logger.Error message in the lease accessibility check around lease.IsAccessibleBy so it states that the lease is not accessible by the client, replacing the outdated “lease not held by client” wording while leaving the permission error and return behavior unchanged.controller/internal/service/client/v1/client_service_test.go (2)
338-342: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not discard the scheme registration error.
_ = jumpstarterdevv1alpha1.AddToScheme(s)hides a registration failure. The fake client then fails later with an unrelated "no kind is registered" message.Return the error to the caller through
t.Fatalf, or passtinto the helper and fail fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/service/client/v1/client_service_test.go` around lines 338 - 342, Update testScheme to handle the error returned by jumpstarterdevv1alpha1.AddToScheme instead of discarding it; pass the test handle into testScheme and call t.Fatalf on registration failure so the test stops with the actual error.
351-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the authorization gates in
UpdateLease.The subtests exercise
applySharedWithChangeswell. They do not cover the surrounding gates inUpdateLease:
- a non-owner shared client attempting
add_shared_withmust be rejected;- a request that combines a transfer with sharing changes;
- a name present in both
add_shared_withandremove_shared_with.Add these cases so the ownership rules stay enforced under refactoring.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/service/client/v1/client_service_test.go` around lines 351 - 533, Extend TestApplySharedWithChanges with UpdateLease-focused cases covering the authorization gates: reject a non-owner shared client issuing add_shared_with, reject requests combining a lease transfer with sharing changes, and reject a name appearing in both add_shared_with and remove_shared_with. Exercise the public UpdateLease path with appropriate lease/client fixtures and assert each request returns an error.controller/internal/service/client/v1/client_service.go (3)
613-615: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared-client limit into a named constant.
The literal
10duplicates the CRD constraint+kubebuilder:validation:MaxItems=10onLeaseSpec.SharedWithincontroller/api/v1alpha1/lease_types.go. If one value changes, the other silently diverges.Define one exported constant in the
v1alpha1package and reference it here and in the create path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/service/client/v1/client_service.go` around lines 613 - 615, Define an exported shared-client limit constant in the v1alpha1 package, use it for LeaseSpec.SharedWith validation and the CRD MaxItems constraint, and replace the literal 10 in the client service validation and create path with that constant.
327-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign create-time shared client validation with the update path.
CreateLeasevalidates only owner-exclusion and client existence.applySharedWithChangesadditionally deduplicates entries and enforces the maximum of 10. A create request with duplicates or more than 10 entries therefore fails later in the API server with a raw CRD validation error instead of anInvalidArgumentgRPC error.Extract the shared checks into one helper and call it from both paths.
Note also the loop variable
nameat Line 328 shadows the lease namenameat Line 308. Rename it tosharedNamefor clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/service/client/v1/client_service.go` around lines 327 - 338, Extract shared-client validation from CreateLease and applySharedWithChanges into a common helper that checks owner exclusion, client existence, duplicate entries, and the maximum of 10 entries, returning InvalidArgument errors consistently. Update both call sites to use the helper, and rename the CreateLease loop variable name to sharedName to avoid shadowing the lease name.
619-663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis policy matcher duplicates
clientAllowedByPolicy.
validateClientPolicyAccessperforms the same exporter-selector and client-selector matching asclientAllowedByPolicyincontroller/internal/controller/lease_controller.go(Lines 538-564). The two copies can diverge, and the service and the reconciler would then disagree on which shared clients are allowed.Move the matching logic into one exported helper in
controller/api/v1alpha1and call it from both sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/service/client/v1/client_service.go` around lines 619 - 663, The access-policy matching logic in validateClientPolicyAccess duplicates the clientAllowedByPolicy behavior and should be centralized. Move the exporter-selector and client-selector matching into a single exported helper under controller/api/v1alpha1, then update ClientService.validateClientPolicyAccess and the lease controller call site to reuse that helper instead of maintaining separate copies. Preserve the existing nil/invalid selector handling and the current allowed/denied outcome in both paths.controller/internal/controller/lease_controller.go (1)
112-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface shared-client pruning to the user.
The reconciler removes entries from
lease.Spec.SharedWithand only writes a log line. The user who ranjmp share addsees a success response, and the entry then disappears with no API-visible reason.Record a Kubernetes event or a lease condition when the reconciler prunes a shared client. That makes the removal traceable through
kubectl describe leaseand through the client-facing status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/controller/lease_controller.go` around lines 112 - 119, Update reconcileSharedWithPolicies and its call site in the lease reconciliation flow to surface each pruned lease.Spec.SharedWith entry through a Kubernetes event or lease condition, rather than only logging it. Ensure the notification identifies the removed shared client and remains visible via kubectl describe lease or client-facing lease status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/internal/controller/lease_controller_test.go`:
- Around line 2738-2741: Rename the test case describing clientAllowedByPolicy
with nil or empty policy lists so its title states that access is denied when no
policies exist. Keep both BeFalse assertions unchanged, since they reflect the
intended behavior.
In `@controller/internal/controller/lease_controller.go`:
- Around line 538-564: Update clientAllowedByPolicy to handle invalid exporter
and client selectors consistently with attachMatchingPolicies: do not silently
continue and return false. Propagate the selector-conversion error through the
reconciliation path, or log it at error level and prevent
reconcileSharedWithPolicies from pruning shared clients during that reconcile.
In `@controller/internal/service/client/v1/client_service.go`:
- Around line 507-509: Restrict destructive lease release to the lease owner by
replacing the IsAccessibleBy guard with IsOwnedBy in DeleteLease at
controller/internal/service/client/v1/client_service.go lines 507-509 and
ReleaseLease at controller/internal/service/controller_service.go lines
1141-1143, keeping both service surfaces consistent.
- Around line 456-489: Update transferLease to validate the target client
against the assigned exporter’s access policy after resolving newClient and
while Status.ExporterRef is set. Reuse validateClientPolicyAccess with the
transfer target and exporter reference, returning its error before updating
Spec.ClientRef; preserve the existing namespace, existence, and lease-state
checks.
- Around line 378-396: Require lease ownership before applying duration or
begin/end time changes in the update flow around updateLeaseTimeFields.
Distinguish requests that modify time fields from other accessible-client
updates, and reject time-field mutations from shared clients while preserving
existing ownership checks for transfer and sharing changes.
- Around line 398-407: Prevent inconsistent combined lease updates by handling
transfer and sharing changes in a mutually exclusive order. In the
request-processing flow around transferLease and the hasShareChanges block,
either reject requests containing both a client transfer and add/remove sharing
changes, or apply sharing changes before transferLease so the transfer’s cleared
Spec.SharedWith and new owner remain authoritative.
In `@protocol/proto/jumpstarter/client/v1/client.proto`:
- Around line 158-159: Update the comment for the shared_with field to describe
its values as client names, matching the contract used by LeaseFromProtobuf and
ClientService.CreateLease. Do not change the resource-name handling or lookup
behavior.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/create.py`:
- Around line 144-145: Update the shared_clients parsing in the create command
to reject empty client names produced by comma-separated --share input,
including leading, trailing, or consecutive commas. Raise click.UsageError
before sending the request, while preserving valid trimmed client names.
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py`:
- Around line 172-188: The set_dtr and set_rts methods should use the
already-open self._transport.serial when connected instead of opening a second
port via serial_for_url. Retain the temporary serial_for_url path only when no
active transport exists, and ensure temporary connections are still closed after
updating the control signal.
In
`@python/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py`:
- Around line 162-168: Regenerate the protobuf-generated metadata in
client_pb2.py from the canonical client.proto definition, including the
serialized start and end offsets for _LEASE_DEPRECATEDLABELSENTRY and
_LEASE_CONTEXTENTRY. Do not manually reorder offsets; use the repository’s
protobuf generation workflow or the matching schema source so non-C descriptor
parsing receives consistent metadata even if the .proto file is not checked in.
In `@python/packages/jumpstarter/jumpstarter/streams/fanout_test.py`:
- Around line 290-296: Update the exclusive-session test around
fanout.attach_exclusive to wrap the nested context manager in
pytest.raises(ExclusiveSessionActive), then assert holder_identity on the
captured exception. Remove the try/except structure so the test fails when no
exception is raised.
In `@python/packages/jumpstarter/jumpstarter/streams/fanout.py`:
- Around line 494-496: The fan-out remains active during driver teardown because
StreamFanOut.close() is async and the PySerial close command shadows the
lifecycle method. In
python/packages/jumpstarter/jumpstarter/streams/fanout.py:494-496, add an async
teardown hook or use the driver portal to await _fanout shutdown before
delegating to super().close(); in
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py:163-170,
rename the exported close command or make it invoke the mixin teardown so
_reader_loop cannot reopen the transport.
- Around line 339-358: Bound the source-readiness wait in both attach_exclusive
and attach_observer so a device that never opens cannot block indefinitely. Wrap
each _wait_source_ready() call in exception handling that detaches the
registered client and releases the write token on TimeoutError, then re-raises
the timeout.
- Around line 295-307: Update _broadcast_data to detect the closed state of each
ClientBuffer after attempting to push data, rather than relying on the
unreachable exception handler. Add closed buffers to disconnected and retain the
existing removal and write-token cleanup logic so closed clients no longer
affect status counts or retain ownership.
- Around line 260-284: Update the reader loop around the async for over source
so a clean end-of-stream follows the same reconnect behavior as handled
exceptions: clear the active reader/source state as appropriate, log the
disconnection and reconnect delay, broadcast the disconnected status, sleep for
the current backoff, and increase backoff before reopening. Preserve shutdown
handling and avoid applying this reconnect path when shutdown has been
requested.
- Around line 317-325: The task group in _ensure_started is entered by one task
but exited by other task paths in _stop_reader, causing invalid cancel-scope
ownership and potentially duplicate reader loops. Refactor task-group lifetime
so a single owner task enters and exits self._task_group, with _stop_reader and
related _detach/close paths signaling that owner to stop; do not swallow
task-group exit failures or clear _started until the original reader and task
group have fully terminated.
---
Outside diff comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py`:
- Around line 214-244: Update the input selection logic around observe,
input_enabled, and stdin_is_piped so observe mode always forces input_enabled to
False, regardless of piped stdin or --input. Preserve the existing no_input,
input_flag, and auto-detection behavior for non-observe mode.
In `@python/packages/jumpstarter/jumpstarter/client/grpc_test.py`:
- Around line 555-559: Expand lease-sharing test coverage: in
python/packages/jumpstarter/jumpstarter/client/grpc_test.py:555-559, cover
protobuf deserialization, Rich row rendering, CreateLease serialization, and
sharing-only UpdateLease requests; in
python/packages/jumpstarter/jumpstarter/config/client_config_test.py:419-450,
pass a non-empty shared_with list and assert it reaches
ClientService.CreateLease; in
python/packages/jumpstarter-cli/jumpstarter_cli/create_test.py:11-42, test
--share alice,bob forwarding and malformed comma-separated input; and in
python/packages/jumpstarter-cli/jumpstarter_cli/share.py:16-88, add tests for
share add, share remove, and share list, including empty and missing lease
results.
---
Nitpick comments:
In `@controller/internal/controller/lease_controller.go`:
- Around line 112-119: Update reconcileSharedWithPolicies and its call site in
the lease reconciliation flow to surface each pruned lease.Spec.SharedWith entry
through a Kubernetes event or lease condition, rather than only logging it.
Ensure the notification identifies the removed shared client and remains visible
via kubectl describe lease or client-facing lease status.
In `@controller/internal/service/client/v1/client_service_test.go`:
- Around line 338-342: Update testScheme to handle the error returned by
jumpstarterdevv1alpha1.AddToScheme instead of discarding it; pass the test
handle into testScheme and call t.Fatalf on registration failure so the test
stops with the actual error.
- Around line 351-533: Extend TestApplySharedWithChanges with
UpdateLease-focused cases covering the authorization gates: reject a non-owner
shared client issuing add_shared_with, reject requests combining a lease
transfer with sharing changes, and reject a name appearing in both
add_shared_with and remove_shared_with. Exercise the public UpdateLease path
with appropriate lease/client fixtures and assert each request returns an error.
In `@controller/internal/service/client/v1/client_service.go`:
- Around line 613-615: Define an exported shared-client limit constant in the
v1alpha1 package, use it for LeaseSpec.SharedWith validation and the CRD
MaxItems constraint, and replace the literal 10 in the client service validation
and create path with that constant.
- Around line 327-338: Extract shared-client validation from CreateLease and
applySharedWithChanges into a common helper that checks owner exclusion, client
existence, duplicate entries, and the maximum of 10 entries, returning
InvalidArgument errors consistently. Update both call sites to use the helper,
and rename the CreateLease loop variable name to sharedName to avoid shadowing
the lease name.
- Around line 619-663: The access-policy matching logic in
validateClientPolicyAccess duplicates the clientAllowedByPolicy behavior and
should be centralized. Move the exporter-selector and client-selector matching
into a single exported helper under controller/api/v1alpha1, then update
ClientService.validateClientPolicyAccess and the lease controller call site to
reuse that helper instead of maintaining separate copies. Preserve the existing
nil/invalid selector handling and the current allowed/denied outcome in both
paths.
In `@controller/internal/service/controller_service.go`:
- Around line 830-834: Update the logger.Error message in the lease
accessibility check around lease.IsAccessibleBy so it states that the lease is
not accessible by the client, replacing the outdated “lease not held by client”
wording while leaving the permission error and return behavior unchanged.
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.py`:
- Around line 54-66: Merge __stdin_exit_only and __stdin_to_serial into a single
stdin-reading method that accepts an optional output stream. Keep the existing
Ctrl-B counting and ConsoleExit behavior in that method, and forward each
received byte only when the optional stream is present; update callers to use
this unified method.
In `@python/packages/jumpstarter/jumpstarter/streams/fanout_test.py`:
- Around line 103-113: Remove the unused _make_memory_source and
_memory_source_factory helpers from fanout_test.py, since tests already define
local factory functions. Do not alter the existing test-local setup or behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d4af493-ca33-40e2-86f4-e3e03f31961f
⛔ Files ignored due to path filters (1)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (29)
controller/api/v1alpha1/lease_helpers.gocontroller/api/v1alpha1/lease_helpers_test.gocontroller/api/v1alpha1/lease_types.gocontroller/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlcontroller/internal/controller/lease_controller.gocontroller/internal/controller/lease_controller_test.gocontroller/internal/service/client/v1/client_service.gocontroller/internal/service/client/v1/client_service_test.gocontroller/internal/service/controller_service.goprotocol/proto/jumpstarter/client/v1/client.protopython/packages/jumpstarter-cli/jumpstarter_cli/create.pypython/packages/jumpstarter-cli/jumpstarter_cli/create_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/jmp.pypython/packages/jumpstarter-cli/jumpstarter_cli/share.pypython/packages/jumpstarter-cli/jumpstarter_cli/update.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver_test.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/grpc_test.pypython/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/config/client.pypython/packages/jumpstarter/jumpstarter/config/client_config_test.pypython/packages/jumpstarter/jumpstarter/streams/fanout.pypython/packages/jumpstarter/jumpstarter/streams/fanout_test.py
Comment on lines
+2738
to
+2741
| It("should allow when no policies exist", func() { | ||
| Expect(clientAllowedByPolicy(nil, exporter, client)).To(BeFalse()) | ||
| Expect(clientAllowedByPolicy([]jumpstarterdevv1alpha1.ExporterAccessPolicy{}, exporter, client)).To(BeFalse()) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test title contradicts its assertions.
The title says "should allow when no policies exist", but both assertions expect BeFalse. The assertions are correct: clientAllowedByPolicy denies when the policy list is empty, and reconcileSharedWithPolicies short-circuits before reaching it. Only the title is wrong, and it will mislead the next reader about the default-open behavior.
💚 Proposed fix
- It("should allow when no policies exist", func() {
+ It("should deny when no policies are supplied (callers short-circuit this case)", func() {📝 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.
Suggested change
| It("should allow when no policies exist", func() { | |
| Expect(clientAllowedByPolicy(nil, exporter, client)).To(BeFalse()) | |
| Expect(clientAllowedByPolicy([]jumpstarterdevv1alpha1.ExporterAccessPolicy{}, exporter, client)).To(BeFalse()) | |
| }) | |
| It("should deny when no policies are supplied (callers short-circuit this case)", func() { | |
| Expect(clientAllowedByPolicy(nil, exporter, client)).To(BeFalse()) | |
| Expect(clientAllowedByPolicy([]jumpstarterdevv1alpha1.ExporterAccessPolicy{}, exporter, client)).To(BeFalse()) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/internal/controller/lease_controller_test.go` around lines 2738 -
2741, Rename the test case describing clientAllowedByPolicy with nil or empty
policy lists so its title states that access is denied when no policies exist.
Keep both BeFalse assertions unchanged, since they reflect the intended
behavior.
Comment on lines
+538
to
+564
| func clientAllowedByPolicy( | ||
| policies []jumpstarterdevv1alpha1.ExporterAccessPolicy, | ||
| exporter *jumpstarterdevv1alpha1.Exporter, | ||
| jclient *jumpstarterdevv1alpha1.Client, | ||
| ) bool { | ||
| for _, policy := range policies { | ||
| exporterSelector, err := metav1.LabelSelectorAsSelector(&policy.Spec.ExporterSelector) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if !exporterSelector.Matches(labels.Set(exporter.Labels)) { | ||
| continue | ||
| } | ||
| for _, p := range policy.Spec.Policies { | ||
| for _, from := range p.From { | ||
| clientSelector, err := metav1.LabelSelectorAsSelector(&from.ClientSelector) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if clientSelector.Matches(labels.Set(jclient.Labels)) { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Malformed selectors silently cause shared clients to be removed.
Lines 545-546 and 554-555 discard the error from metav1.LabelSelectorAsSelector and continue. A policy with an invalid selector then matches nothing, clientAllowedByPolicy returns false, and reconcileSharedWithPolicies deletes the shared client from Spec.SharedWith. The removal is permanent, and the operator gets no signal about the broken policy.
attachMatchingPolicies (Lines 446-449 and 454-457) returns an error for the same condition. Align the two paths: propagate the error, or at minimum log it at error level and skip pruning for that reconcile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/internal/controller/lease_controller.go` around lines 538 - 564,
Update clientAllowedByPolicy to handle invalid exporter and client selectors
consistently with attachMatchingPolicies: do not silently continue and return
false. Propagate the selector-conversion error through the reconciliation path,
or log it at error level and prevent reconcileSharedWithPolicies from pruning
shared clients during that reconcile.
Comment on lines
+378
to
396
| hasShareChanges := len(req.AddSharedWith) > 0 || len(req.RemoveSharedWith) > 0 | ||
|
|
||
| if hasShareChanges { | ||
| if !jlease.IsOwnedBy(jclient.Name) { | ||
| return nil, fmt.Errorf("UpdateLease permission denied: only lease owner can modify sharing") | ||
| } | ||
| } else if !jlease.IsAccessibleBy(jclient.Name) { | ||
| return nil, fmt.Errorf("UpdateLease permission denied") | ||
| } | ||
|
|
||
| original := kclient.MergeFrom(jlease.DeepCopy()) | ||
|
|
||
| // Only parse time fields from protobuf if any are being updated | ||
| if req.Lease.BeginTime != nil || req.Lease.Duration != nil || req.Lease.EndTime != nil { | ||
| desired, err := jumpstarterdevv1alpha1.LeaseFromProtobuf(req.Lease, *key, | ||
| corev1.LocalObjectReference{ | ||
| Name: jclient.Name, | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := s.updateLeaseTimeFields(req.Lease, key, jclient.Name, &jlease); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // BeginTime can only be updated before lease starts; only if explicitly provided | ||
| if req.Lease.BeginTime != nil { | ||
| if jlease.Status.ExporterRef != nil { | ||
| if jlease.Spec.BeginTime == nil || !jlease.Spec.BeginTime.Equal(desired.Spec.BeginTime) { | ||
| return nil, fmt.Errorf("cannot update BeginTime: lease has already started") | ||
| } | ||
| } | ||
| jlease.Spec.BeginTime = desired.Spec.BeginTime | ||
| } | ||
| // Update Duration only if provided; preserve existing otherwise | ||
| if req.Lease.Duration != nil { | ||
| jlease.Spec.Duration = desired.Spec.Duration | ||
| } | ||
| // Update EndTime only if provided; preserve existing otherwise | ||
| if req.Lease.EndTime != nil { | ||
| jlease.Spec.EndTime = desired.Spec.EndTime | ||
| } | ||
| if err := s.transferLease(ctx, req.Lease, key, jclient.Name, &jlease); err != nil { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
A shared client can change the lease duration and times.
Line 384 accepts any client returned by IsAccessibleBy, which includes shared clients. updateLeaseTimeFields (Lines 422-454) applies Duration, BeginTime, and EndTime without an ownership check. A shared client can therefore extend or shorten a lease it does not own. transferLease and sharing changes both require ownership, so time fields are the only unguarded mutation.
Require ownership for time-field updates, or restrict which time fields a shared client may change.
🔒️ Proposed fix to require ownership for time updates
func (s *ClientService) updateLeaseTimeFields(
lease *cpb.Lease,
key *types.NamespacedName,
clientName string,
jlease *jumpstarterdevv1alpha1.Lease,
) error {
if lease.BeginTime == nil && lease.Duration == nil && lease.EndTime == nil {
return nil
}
+ if !jlease.IsOwnedBy(clientName) {
+ return fmt.Errorf("UpdateLease permission denied: only lease owner can change lease times")
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/internal/service/client/v1/client_service.go` around lines 378 -
396, Require lease ownership before applying duration or begin/end time changes
in the update flow around updateLeaseTimeFields. Distinguish requests that
modify time fields from other accessible-client updates, and reject time-field
mutations from shared clients while preserving existing ownership checks for
transfer and sharing changes.
Comment on lines
+398
to
407
| if hasShareChanges { | ||
| if jlease.Status.Ended { | ||
| return nil, fmt.Errorf("cannot transfer lease: lease has already ended") | ||
| return nil, fmt.Errorf("cannot modify sharing: lease has already ended") | ||
| } | ||
| newClientKey, err := utils.ParseClientIdentifier(*req.Lease.Client) | ||
| newShared, err := s.applySharedWithChanges(ctx, &jlease, key.Namespace, req.AddSharedWith, req.RemoveSharedWith) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if newClientKey.Namespace != key.Namespace { | ||
| return nil, fmt.Errorf("cannot transfer lease to client in different namespace") | ||
| } | ||
| var newClient jumpstarterdevv1alpha1.Client | ||
| if err := s.Get(ctx, *newClientKey, &newClient); err != nil { | ||
| return nil, fmt.Errorf("target client not found: %w", err) | ||
| } | ||
| jlease.Spec.ClientRef.Name = newClientKey.Name | ||
| jlease.Spec.SharedWith = newShared | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Combined transfer and share requests produce inconsistent state.
transferLease (Lines 456-489) changes Spec.ClientRef.Name and sets Spec.SharedWith to nil. This block then runs afterwards. If one request carries both lease.client and add_shared_with, the sharing list that transfer intentionally cleared is repopulated, and the owner-exclusion check inside applySharedWithChanges compares against the new owner rather than the caller.
Reject requests that combine a transfer with sharing changes, or apply sharing changes before the transfer.
🛡️ Proposed guard
hasShareChanges := len(req.AddSharedWith) > 0 || len(req.RemoveSharedWith) > 0
+
+ if hasShareChanges && req.Lease.Client != nil && *req.Lease.Client != "" {
+ return nil, status.Error(codes.InvalidArgument,
+ "cannot transfer a lease and modify sharing in the same request")
+ }📝 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.
Suggested change
| if hasShareChanges { | |
| if jlease.Status.Ended { | |
| return nil, fmt.Errorf("cannot transfer lease: lease has already ended") | |
| return nil, fmt.Errorf("cannot modify sharing: lease has already ended") | |
| } | |
| newClientKey, err := utils.ParseClientIdentifier(*req.Lease.Client) | |
| newShared, err := s.applySharedWithChanges(ctx, &jlease, key.Namespace, req.AddSharedWith, req.RemoveSharedWith) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if newClientKey.Namespace != key.Namespace { | |
| return nil, fmt.Errorf("cannot transfer lease to client in different namespace") | |
| } | |
| var newClient jumpstarterdevv1alpha1.Client | |
| if err := s.Get(ctx, *newClientKey, &newClient); err != nil { | |
| return nil, fmt.Errorf("target client not found: %w", err) | |
| } | |
| jlease.Spec.ClientRef.Name = newClientKey.Name | |
| jlease.Spec.SharedWith = newShared | |
| } | |
| hasShareChanges := len(req.AddSharedWith) > 0 || len(req.RemoveSharedWith) > 0 | |
| if hasShareChanges && req.Lease.Client != nil && *req.Lease.Client != "" { | |
| return nil, status.Error(codes.InvalidArgument, | |
| "cannot transfer a lease and modify sharing in the same request") | |
| } | |
| if hasShareChanges { | |
| if jlease.Status.Ended { | |
| return nil, fmt.Errorf("cannot modify sharing: lease has already ended") | |
| } | |
| newShared, err := s.applySharedWithChanges(ctx, &jlease, key.Namespace, req.AddSharedWith, req.RemoveSharedWith) | |
| if err != nil { | |
| return nil, err | |
| } | |
| jlease.Spec.SharedWith = newShared | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/internal/service/client/v1/client_service.go` around lines 398 -
407, Prevent inconsistent combined lease updates by handling transfer and
sharing changes in a mutually exclusive order. In the request-processing flow
around transferLease and the hasShareChanges block, either reject requests
containing both a client transfer and add/remove sharing changes, or apply
sharing changes before transferLease so the transfer’s cleared Spec.SharedWith
and new owner remain authoritative.
Comment on lines
+456
to
+489
| func (s *ClientService) transferLease( | ||
| ctx context.Context, | ||
| lease *cpb.Lease, | ||
| key *types.NamespacedName, | ||
| clientName string, | ||
| jlease *jumpstarterdevv1alpha1.Lease, | ||
| ) error { | ||
| if lease.Client == nil || *lease.Client == "" { | ||
| return nil | ||
| } | ||
| if !jlease.IsOwnedBy(clientName) { | ||
| return fmt.Errorf("UpdateLease permission denied: only lease owner can transfer") | ||
| } | ||
| if jlease.Status.ExporterRef == nil { | ||
| return fmt.Errorf("cannot transfer lease: lease has not started yet") | ||
| } | ||
| if jlease.Status.Ended { | ||
| return fmt.Errorf("cannot transfer lease: lease has already ended") | ||
| } | ||
| newClientKey, err := utils.ParseClientIdentifier(*lease.Client) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if newClientKey.Namespace != key.Namespace { | ||
| return fmt.Errorf("cannot transfer lease to client in different namespace") | ||
| } | ||
| var newClient jumpstarterdevv1alpha1.Client | ||
| if err := s.Get(ctx, *newClientKey, &newClient); err != nil { | ||
| return fmt.Errorf("target client not found: %w", err) | ||
| } | ||
| jlease.Spec.ClientRef.Name = newClientKey.Name | ||
| jlease.Spec.SharedWith = nil | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Lease transfer bypasses exporter access policy.
transferLease verifies only that the target client exists in the same namespace. It does not call validateClientPolicyAccess. Sharing an active lease is policy-checked (Lines 605-609), and the reconciler prunes policy-denied entries from SharedWith, but neither path validates Spec.ClientRef. An owner can therefore transfer an active lease to a client that no ExporterAccessPolicy allows for the assigned exporter, and that client gains Dial access.
Apply the same policy check to the transfer target when Status.ExporterRef is set.
🔒️ Proposed fix
var newClient jumpstarterdevv1alpha1.Client
if err := s.Get(ctx, *newClientKey, &newClient); err != nil {
return fmt.Errorf("target client not found: %w", err)
}
+ if err := s.validateClientPolicyAccess(ctx, &newClient, jlease); err != nil {
+ return fmt.Errorf("client %q not authorized for this exporter: %w", newClientKey.Name, err)
+ }
jlease.Spec.ClientRef.Name = newClientKey.Name📝 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.
Suggested change
| func (s *ClientService) transferLease( | |
| ctx context.Context, | |
| lease *cpb.Lease, | |
| key *types.NamespacedName, | |
| clientName string, | |
| jlease *jumpstarterdevv1alpha1.Lease, | |
| ) error { | |
| if lease.Client == nil || *lease.Client == "" { | |
| return nil | |
| } | |
| if !jlease.IsOwnedBy(clientName) { | |
| return fmt.Errorf("UpdateLease permission denied: only lease owner can transfer") | |
| } | |
| if jlease.Status.ExporterRef == nil { | |
| return fmt.Errorf("cannot transfer lease: lease has not started yet") | |
| } | |
| if jlease.Status.Ended { | |
| return fmt.Errorf("cannot transfer lease: lease has already ended") | |
| } | |
| newClientKey, err := utils.ParseClientIdentifier(*lease.Client) | |
| if err != nil { | |
| return err | |
| } | |
| if newClientKey.Namespace != key.Namespace { | |
| return fmt.Errorf("cannot transfer lease to client in different namespace") | |
| } | |
| var newClient jumpstarterdevv1alpha1.Client | |
| if err := s.Get(ctx, *newClientKey, &newClient); err != nil { | |
| return fmt.Errorf("target client not found: %w", err) | |
| } | |
| jlease.Spec.ClientRef.Name = newClientKey.Name | |
| jlease.Spec.SharedWith = nil | |
| return nil | |
| } | |
| func (s *ClientService) transferLease( | |
| ctx context.Context, | |
| lease *cpb.Lease, | |
| key *types.NamespacedName, | |
| clientName string, | |
| jlease *jumpstarterdevv1alpha1.Lease, | |
| ) error { | |
| if lease.Client == nil || *lease.Client == "" { | |
| return nil | |
| } | |
| if !jlease.IsOwnedBy(clientName) { | |
| return fmt.Errorf("UpdateLease permission denied: only lease owner can transfer") | |
| } | |
| if jlease.Status.ExporterRef == nil { | |
| return fmt.Errorf("cannot transfer lease: lease has not started yet") | |
| } | |
| if jlease.Status.Ended { | |
| return fmt.Errorf("cannot transfer lease: lease has already ended") | |
| } | |
| newClientKey, err := utils.ParseClientIdentifier(*lease.Client) | |
| if err != nil { | |
| return err | |
| } | |
| if newClientKey.Namespace != key.Namespace { | |
| return fmt.Errorf("cannot transfer lease to client in different namespace") | |
| } | |
| var newClient jumpstarterdevv1alpha1.Client | |
| if err := s.Get(ctx, *newClientKey, &newClient); err != nil { | |
| return fmt.Errorf("target client not found: %w", err) | |
| } | |
| if err := s.validateClientPolicyAccess(ctx, &newClient, jlease); err != nil { | |
| return fmt.Errorf("client %q not authorized for this exporter: %w", newClientKey.Name, err) | |
| } | |
| jlease.Spec.ClientRef.Name = newClientKey.Name | |
| jlease.Spec.SharedWith = nil | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/internal/service/client/v1/client_service.go` around lines 456 -
489, Update transferLease to validate the target client against the assigned
exporter’s access policy after resolving newClient and while Status.ExporterRef
is set. Reuse validateClientPolicyAccess with the transfer target and exporter
reference, returning its error before updating Spec.ClientRef; preserve the
existing namespace, existence, and lease-state checks.
Comment on lines
+260
to
+284
| while not self._shutdown: | ||
| try: | ||
| async with self._source_factory() as source: | ||
| self._source = source | ||
| backoff = 0.1 | ||
| if not first_connect: | ||
| self._broadcast_system(b"[reconnected]\r\n") | ||
| first_connect = False | ||
| self._reader_running = True | ||
| self._source_ready.set() | ||
|
|
||
| async for data in source: | ||
| if self._shutdown: | ||
| break | ||
| self._scrollback_append(data) | ||
| self._broadcast_data(data) | ||
| except (OSError, BrokenResourceError, ClosedResourceError) as e: | ||
| self._source = None | ||
| self._reader_running = False | ||
| if self._shutdown: | ||
| break | ||
| logger.warning("Source disconnected (%s: %s), reconnecting in %.1fs", type(e).__name__, e, backoff) | ||
| self._broadcast_system(f"[disconnected: {e}, reconnecting...]\r\n".encode()) | ||
| await anyio.sleep(backoff) | ||
| backoff = min(backoff * 2, max_backoff) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Apply backoff after a clean end of stream.
The async for loop exits without an exception when the source reaches end of stream. The outer while loop then reopens the source immediately. If the source ends right after each open, the reader loop spins with no delay and no log output. Apply the same backoff on the normal exit path.
🐛 Proposed fix
async for data in source:
if self._shutdown:
break
self._scrollback_append(data)
self._broadcast_data(data)
+ if self._shutdown:
+ break
+ self._source = None
+ self._reader_running = False
+ logger.warning("Source ended, reconnecting in %.1fs", backoff)
+ self._broadcast_system(b"[source ended, reconnecting...]\r\n")
+ await anyio.sleep(backoff)
+ backoff = min(backoff * 2, max_backoff)
except (OSError, BrokenResourceError, ClosedResourceError) as e:📝 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.
Suggested change
| while not self._shutdown: | |
| try: | |
| async with self._source_factory() as source: | |
| self._source = source | |
| backoff = 0.1 | |
| if not first_connect: | |
| self._broadcast_system(b"[reconnected]\r\n") | |
| first_connect = False | |
| self._reader_running = True | |
| self._source_ready.set() | |
| async for data in source: | |
| if self._shutdown: | |
| break | |
| self._scrollback_append(data) | |
| self._broadcast_data(data) | |
| except (OSError, BrokenResourceError, ClosedResourceError) as e: | |
| self._source = None | |
| self._reader_running = False | |
| if self._shutdown: | |
| break | |
| logger.warning("Source disconnected (%s: %s), reconnecting in %.1fs", type(e).__name__, e, backoff) | |
| self._broadcast_system(f"[disconnected: {e}, reconnecting...]\r\n".encode()) | |
| await anyio.sleep(backoff) | |
| backoff = min(backoff * 2, max_backoff) | |
| while not self._shutdown: | |
| try: | |
| async with self._source_factory() as source: | |
| self._source = source | |
| backoff = 0.1 | |
| if not first_connect: | |
| self._broadcast_system(b"[reconnected]\r\n") | |
| first_connect = False | |
| self._reader_running = True | |
| self._source_ready.set() | |
| async for data in source: | |
| if self._shutdown: | |
| break | |
| self._scrollback_append(data) | |
| self._broadcast_data(data) | |
| if self._shutdown: | |
| break | |
| self._source = None | |
| self._reader_running = False | |
| logger.warning("Source ended, reconnecting in %.1fs", backoff) | |
| self._broadcast_system(b"[source ended, reconnecting...]\r\n") | |
| await anyio.sleep(backoff) | |
| backoff = min(backoff * 2, max_backoff) | |
| except (OSError, BrokenResourceError, ClosedResourceError) as e: | |
| self._source = None | |
| self._reader_running = False | |
| if self._shutdown: | |
| break | |
| logger.warning("Source disconnected (%s: %s), reconnecting in %.1fs", type(e).__name__, e, backoff) | |
| self._broadcast_system(f"[disconnected: {e}, reconnecting...]\r\n".encode()) | |
| await anyio.sleep(backoff) | |
| backoff = min(backoff * 2, max_backoff) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter/jumpstarter/streams/fanout.py` around lines 260 -
284, Update the reader loop around the async for over source so a clean
end-of-stream follows the same reconnect behavior as handled exceptions: clear
the active reader/source state as appropriate, log the disconnection and
reconnect delay, broadcast the disconnected status, sleep for the current
backoff, and increase backoff before reopening. Preserve shutdown handling and
avoid applying this reconnect path when shutdown has been requested.
Comment on lines
+295
to
+307
| def _broadcast_data(self, data: bytes) -> None: | ||
| """Push data to all clients, removing any that error.""" | ||
| disconnected = [] | ||
| for client_id, buf in self._clients.items(): | ||
| try: | ||
| buf.push(data) | ||
| except Exception: | ||
| disconnected.append(client_id) | ||
| for client_id in disconnected: | ||
| self._clients.pop(client_id, None) | ||
| if client_id == self._write_token_holder: | ||
| self._write_token_holder = None | ||
| self._write_token_holder_identity = None |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prune clients whose buffer is closed.
ClientBuffer.push never raises; it returns early when the buffer is closed. The except Exception branch is therefore unreachable, and a buffer closed by the error overflow policy stays registered. It keeps inflating status() counts and keeps the write token if it held it.
🐛 Proposed fix
disconnected = []
for client_id, buf in self._clients.items():
- try:
- buf.push(data)
- except Exception:
- disconnected.append(client_id)
+ if buf.closed:
+ disconnected.append(client_id)
+ continue
+ buf.push(data)
for client_id in disconnected:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter/jumpstarter/streams/fanout.py` around lines 295 -
307, Update _broadcast_data to detect the closed state of each ClientBuffer
after attempting to push data, rather than relying on the unreachable exception
handler. Add closed buffers to disconnected and retain the existing removal and
write-token cleanup logic so closed clients no longer affect status counts or
retain ownership.
Comment on lines
+339
to
+358
| async with self._lock: | ||
| if self._write_token_holder is not None: | ||
| raise ExclusiveSessionActive(self._write_token_holder_identity) | ||
|
|
||
| await self._ensure_started() | ||
|
|
||
| client_id = _new_client_id() | ||
| buf = ClientBuffer(max_bytes=buffer_bytes, on_overflow=on_overflow) | ||
| # Atomic: snapshot scrollback + register, under lock | ||
| buf.prefill(self._scrollback_snapshot()) | ||
| self._clients[client_id] = buf | ||
| self._write_token_holder = client_id | ||
| self._write_token_holder_identity = identity | ||
|
|
||
| await self._wait_source_ready() | ||
| stream = ExclusiveStream(self, client_id, buf) | ||
| try: | ||
| yield stream | ||
| finally: | ||
| await self._detach(client_id, release_token=True) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the wait for the source.
attach_exclusive takes the write token and then waits on _source_ready with no timeout. If the device never opens, the reader loop retries forever and never sets the event. The caller then hangs and holds the write token, so no other client can attach exclusively. Apply the same bound in attach_observer.
🐛 Proposed fix
- async def _wait_source_ready(self) -> None:
+ async def _wait_source_ready(self, timeout: float = 30.0) -> None:
"""Wait for the reader loop to open the source. Call after releasing _lock."""
- await self._source_ready.wait()
+ with anyio.move_on_after(timeout) as scope:
+ await self._source_ready.wait()
+ if scope.cancelled_caught:
+ raise TimeoutError("source did not become ready")Release the client registration and the write token if the wait fails:
try:
await self._wait_source_ready()
except TimeoutError:
await self._detach(client_id, release_token=True)
raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter/jumpstarter/streams/fanout.py` around lines 339 -
358, Bound the source-readiness wait in both attach_exclusive and
attach_observer so a device that never opens cannot block indefinitely. Wrap
each _wait_source_ready() call in exception handling that detaches the
registered client and releases the write token on TimeoutError, then re-raises
the timeout.
Comment on lines
+494
to
+496
| def close(self): | ||
| if hasattr(super(), "close"): | ||
| super().close() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Nothing shuts the fan-out down on driver teardown. The mixin close() forwards to super().close() and never closes _fanout, and PySerial.close replaces that method in the MRO. With always_on=True, the reader loop keeps the serial port open and reopens it after the transport closes.
python/packages/jumpstarter/jumpstarter/streams/fanout.py#L494-L496: shut down the fan-out in the mixin teardown. BecauseStreamFanOut.close()is async, expose an async lifecycle hook or run the shutdown through the driver portal before callingsuper().close().python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py#L163-L170: rename the exported command so it no longer shadows the lifecycleclose, or call the mixin teardown from it. Closing onlyself._transportlets_reader_loopreopen the port after the backoff delay.
📍 Affects 2 files
python/packages/jumpstarter/jumpstarter/streams/fanout.py#L494-L496(this comment)python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py#L163-L170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter/jumpstarter/streams/fanout.py` around lines 494 -
496, The fan-out remains active during driver teardown because
StreamFanOut.close() is async and the PySerial close command shadows the
lifecycle method. In
python/packages/jumpstarter/jumpstarter/streams/fanout.py:494-496, add an async
teardown hook or use the driver portal to await _fanout shutdown before
delegating to super().close(); in
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py:163-170,
rename the exported close command or make it invoke the mixin teardown so
_reader_loop cannot reopen the transport.
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/lease_test.py (1)
377-377: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd positive-path coverage for lease sharing.
The new tests cover empty sharing values and non-owner rejection, but they do not verify successful shared access or non-empty CLI forwarding.
python/packages/jumpstarter/jumpstarter/client/lease_test.py#L377-L377: add a test whereshared_withcontains the requesting client and assert thatrequest_async()succeeds.python/packages/jumpstarter-cli/jumpstarter_cli/update_test.py#L24-L35: add non-emptyshare_addandshare_removevalues and assert list conversion.python/packages/jumpstarter-cli/jumpstarter_cli/update_test.py#L53-L64: cover non-empty sharing values with a duration update.python/packages/jumpstarter-cli/jumpstarter_cli/update_test.py#L81-L92: cover non-empty sharing values without a client transfer.python/packages/jumpstarter-cli/jumpstarter_cli/update_test.py#L105-L106: retain the empty-values validation case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py` at line 377, Add positive-path lease-sharing and CLI forwarding coverage: in python/packages/jumpstarter/jumpstarter/client/lease_test.py lines 377-377, make shared_with include the requesting client and assert request_async() succeeds; in python/packages/jumpstarter-cli/jumpstarter_cli/update_test.py lines 24-35, 53-64, and 81-92, use non-empty share_add/share_remove values and assert list conversion for updates with duration, and without client transfer; retain the empty-values validation case at lines 105-106.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py`:
- Line 377: Add positive-path lease-sharing and CLI forwarding coverage: in
python/packages/jumpstarter/jumpstarter/client/lease_test.py lines 377-377, make
shared_with include the requesting client and assert request_async() succeeds;
in python/packages/jumpstarter-cli/jumpstarter_cli/update_test.py lines 24-35,
53-64, and 81-92, use non-empty share_add/share_remove values and assert list
conversion for updates with duration, and without client transfer; retain the
empty-values validation case at lines 105-106.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e37210a5-c2b8-4dd7-8707-daf7471fa921
⛔ Files ignored due to path filters (1)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (31)
controller/api/v1alpha1/lease_helpers.gocontroller/api/v1alpha1/lease_helpers_test.gocontroller/api/v1alpha1/lease_types.gocontroller/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlcontroller/internal/controller/lease_controller.gocontroller/internal/controller/lease_controller_test.gocontroller/internal/service/client/v1/client_service.gocontroller/internal/service/client/v1/client_service_test.gocontroller/internal/service/controller_service.goprotocol/proto/jumpstarter/client/v1/client.protopython/packages/jumpstarter-cli/jumpstarter_cli/create.pypython/packages/jumpstarter-cli/jumpstarter_cli/create_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/jmp.pypython/packages/jumpstarter-cli/jumpstarter_cli/share.pypython/packages/jumpstarter-cli/jumpstarter_cli/update.pypython/packages/jumpstarter-cli/jumpstarter_cli/update_test.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver_test.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/grpc_test.pypython/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/client/lease_test.pypython/packages/jumpstarter/jumpstarter/config/client.pypython/packages/jumpstarter/jumpstarter/config/client_config_test.pypython/packages/jumpstarter/jumpstarter/streams/fanout.pypython/packages/jumpstarter/jumpstarter/streams/fanout_test.py
🚧 Files skipped from review as they are similar to previous changes (29)
- python/packages/jumpstarter/jumpstarter/client/grpc_test.py
- controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml
- python/packages/jumpstarter-cli/jumpstarter_cli/jmp.py
- python/packages/jumpstarter-cli/jumpstarter_cli/create.py
- python/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
- controller/internal/service/controller_service.go
- python/packages/jumpstarter/jumpstarter/config/client.py
- python/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyi
- python/packages/jumpstarter-cli/jumpstarter_cli/share.py
- python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.py
- python/packages/jumpstarter/jumpstarter/streams/fanout_test.py
- python/packages/jumpstarter-cli/jumpstarter_cli/update.py
- python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
- python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver_test.py
- python/packages/jumpstarter/jumpstarter/config/client_config_test.py
- controller/internal/service/client/v1/client_service.go
- controller/internal/service/client/v1/client_service_test.go
- controller/api/v1alpha1/lease_helpers.go
- controller/api/v1alpha1/lease_helpers_test.go
- python/packages/jumpstarter/jumpstarter/client/lease.py
- python/packages/jumpstarter/jumpstarter/streams/fanout.py
- python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py
- protocol/proto/jumpstarter/client/v1/client.proto
- controller/internal/controller/lease_controller_test.go
- controller/internal/controller/lease_controller.go
- controller/api/v1alpha1/lease_types.go
- controller/api/v1alpha1/zz_generated.deepcopy.go
- python/packages/jumpstarter-cli/jumpstarter_cli/create_test.py
- python/packages/jumpstarter/jumpstarter/client/grpc.py
bennyz
marked this pull request as ready for review
August 6, 2026 09:49
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Add the ability for a lease owner to share access with other clients in the same namespace. Shared clients can connect (Dial), extend, and release the lease just like the owner. Closes jumpstarter-dev#898 - Add shared_with field to Lease CRD spec and proto - Controller reconciler propagates sharing changes and validates that shared clients exist in the same namespace - gRPC service authorizes shared clients for Dial, Listen, ExtendLease, and ReleaseLease operations - CLI: `jmp create lease --share client1,client2` - CLI: `jmp update lease <id> --share-add/--share-remove` - CLI: `jmp share add/remove/list` dedicated subcommands - Client config round-trips shared_with through YAML - Fix: shared clients can connect via `jmp shell --lease` - Fix: observer console exits on Ctrl+B x3 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Allow multiple clients sharing a lease to access the serial console simultaneously. One client holds the exclusive write token, others observe the output read-only with scrollback replay. - Add StreamFanOut state machine and FanOutStreamMixin for drivers with exclusive physical streams (jumpstarter/streams/fanout.py) - CLI: `j serial start-console --observe` for read-only console - CLI: `j serial pipe --observe` for read-only pipe - CLI: `j serial release-console` to force-release write token - CLI: `j serial console-status` to show session info - Byte-bounded ClientBuffer with drop-oldest overflow policy - 64KB scrollback ring replayed atomically on observer attach Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
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
2 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.
No description provided.