refactor: replace _previous_leased with derived LeaseState - #948
Conversation
|
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:
📝 WalkthroughWalkthroughThe exporter now uses the public ChangesExporter lease lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Exporter
participant LeaseContext
participant BeforeLeaseHook
participant ConnectionStream
Exporter->>LeaseContext: create and register lease context
Exporter->>BeforeLeaseHook: start before-lease hook
Exporter->>ConnectionStream: process lease connections
ConnectionStream-->>Exporter: lease end or interruption
Exporter->>ConnectionStream: close listen stream
Exporter->>BeforeLeaseHook: run after-lease cleanup
Exporter->>LeaseContext: record completed lease and clear contexts
Exporter->>Exporter: replay deferred status
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
d21d21a to
9b72161
Compare
209efdd to
117bb70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (3)
1135-1153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that the rejected lease does not update client state.
The test proves the return value and the retained
lease_name. The rejection path also skips_on_lease_update. Add an assertion onclient_nameso a future change that moves_on_lease_updatebefore the overlap check fails this test.🧪 Proposed assertion
assert result is False assert exporter._lease_context.lease_name == "lease-A" + assert exporter._lease_context.client_name != "other-client"🤖 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/exporter/exporter_test.py` around lines 1135 - 1153, Extend test_overlap_rejection_returns_false to assert that the rejected lease does not alter the existing lease context’s client_name, alongside the current lease_name assertion. Use the original lease context value and keep the assertion after _apply_status completes.
1341-1349: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleeps with event-based synchronisation.
This test depends on two
anyio.sleep(0.1)calls to lethandle_leasereach itsfinallyblock. On a loaded CI runner those windows can expire before the cleanup runs, which makes the test flaky.test_handle_lease_processes_connectionsalready uses anEventplusfail_after. Apply the same pattern here, for example by wrapping_cleanup_after_leasein a side effect that sets an event and waiting on that event underfail_after.🤖 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/exporter/exporter_test.py` around lines 1341 - 1349, Replace the fixed anyio.sleep calls in the handle_lease test with event-based synchronization: have the _cleanup_after_lease mock side effect set an Event, then wait for that event within fail_after before cancelling the task group. Follow the pattern used by test_handle_lease_processes_connections while preserving the existing assertions.
1120-1133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing one exporter test factory.
_make_idle_exporterrepeats most of_make_serve_exporterat line 1366. Both build anExporterthrough__new__and set the same private fields. A shared helper with keyword arguments would keep the two fixtures in sync when newinit=Falsefields appear.🤖 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/exporter/exporter_test.py` around lines 1120 - 1133, Refactor the exporter test factories by introducing one shared helper for constructing the Exporter via __new__ and initializing the common private fields. Update _make_idle_exporter and _make_serve_exporter to call that helper with keyword arguments for their differing state, keeping both fixtures synchronized as new init=False fields are added.
🤖 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 `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 1232-1248: Add assertions to
test_leased_to_idle_calls_on_lease_released verifying the observable effects of
_on_lease_released, including that the lease context’s release signalling is
triggered and, when exit_on_lease_end is enabled, _stop_requested is set.
Configure the exporter flag as needed while preserving the existing
LEASED-to-IDLE setup.
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 1000-1009: In the finally cleanup guarded by self._lease_context
is lease_scope, keep the lease context set while awaiting the 0.2-second session
settle delay, then clear _lease_context and call clear_log_context afterward.
Ensure cancellation cannot bypass the required delay and subsequent context
cleanup, using the surrounding task framework’s shielded cancellation mechanism
if necessary.
- Around line 1051-1071: Update the lease reacquisition logic around
_apply_status and handle_lease so a completed lease remains a boundary after
_lease_context is cleared and _lease_state becomes IDLE. Track the
just-completed lease name/status or require a different non-empty lease status
before calling _on_lease_acquired, while preserving acquisition of genuinely new
leases and existing lease-update behavior.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 1135-1153: Extend test_overlap_rejection_returns_false to assert
that the rejected lease does not alter the existing lease context’s client_name,
alongside the current lease_name assertion. Use the original lease context value
and keep the assertion after _apply_status completes.
- Around line 1341-1349: Replace the fixed anyio.sleep calls in the handle_lease
test with event-based synchronization: have the _cleanup_after_lease mock side
effect set an Event, then wait for that event within fail_after before
cancelling the task group. Follow the pattern used by
test_handle_lease_processes_connections while preserving the existing
assertions.
- Around line 1120-1133: Refactor the exporter test factories by introducing one
shared helper for constructing the Exporter via __new__ and initializing the
common private fields. Update _make_idle_exporter and _make_serve_exporter to
call that helper with keyword arguments for their differing state, keeping both
fixtures synchronized as new init=False fields are added.
🪄 Autofix (Beta)
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: 9458b628-e270-4794-b975-726807f28e7d
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
A few notes, nothing major.
|
|
||
| if self._lease_context is None and status.lease_name != "" and current_leased: | ||
| self._on_lease_acquired(status, tg) | ||
|
|
||
| if current_leased: | ||
| if previous_state == LeaseState.IDLE and status.lease_name != "": | ||
| self._on_lease_acquired(status, tg) | ||
| elif ( | ||
| previous_state == LeaseState.LEASED | ||
| and self._lease_context | ||
| and self._lease_context.lease_name != status.lease_name | ||
| ): |
There was a problem hiding this comment.
Nice addition — prevents concurrent handle_lease tasks for different leases.
Worth noting there's a small timing window here: after _on_lease_released returns (having waited for after_lease_hook_done), handle_lease's outer finally may not have run yet (session teardown still in progress). During that gap _lease_context is still set, so a new lease arriving here would be rejected. It gets picked up on the next status tick once the finally clears _lease_context.
A brief comment would help future readers understand this is intentional:
# Note: this may briefly reject new leases while handle_lease
# finishes session teardown; the next status tick will acquire them.| """LEASED → IDLE transitions through _on_lease_released.""" | ||
| exporter = self._make_idle_exporter() | ||
| lease_ctx = make_lease_context(lease_name="ending-lease") | ||
| lease_ctx.after_lease_hook_done.set() | ||
| exporter._lease_context = lease_ctx | ||
| exporter._started = True | ||
|
|
||
| status = MagicMock() | ||
| status.leased = False | ||
| status.lease_name = "" | ||
| status.client_name = "" | ||
| status.context = {} | ||
|
|
||
| async with create_task_group() as tg: | ||
| await exporter._apply_status(status, tg) | ||
| tg.cancel_scope.cancel() |
There was a problem hiding this comment.
This test runs the LEASED→IDLE transition but has no assertions — it only checks that it doesn't raise. Consider verifying the side effects, e.g.:
assert lease_ctx.lease_ended.is_set()This would confirm the signal actually reached handle_lease (the core purpose of the transition).
|
|
||
| async def _on_lease_released(self, previous_leased: bool) -> None: | ||
| async def _on_lease_released(self, previous_state: LeaseState) -> None: | ||
| """Handle not-leased status: signal handle_lease on transition, clean up context.""" |
There was a problem hiding this comment.
nit: The docstring still says "clean up context" but this method no longer clears _lease_context, clear_log_context(), or does the sleep(0.2) — that responsibility moved to handle_lease's finally block. Consider updating to match the reduced scope:
"""Handle not-leased status: signal handle_lease on transition, check exit_on_lease_end."""
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
1001-1011: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCancellation during the settle delay skips the remaining cleanup.
The ordering fix is correct: the delay now runs before the context is cleared. One gap remains.
await sleep(0.2)is a cancellation checkpoint. If the surrounding task group is cancelled while thisfinallyruns, thesleepraises immediately and Lines 1008-1011 never execute._lease_contextthen stays set and_lease_statekeeps reportingLEASED. Shield the delay so the state cleanup always completes.🔧 Proposed fix
finally: if self._lease_context is lease_scope: session_was_created = lease_scope.session is not None - if session_was_created: - # Brief delay to ensure session is fully closed before next lease. - # Prevents SSL corruption from overlapping connections. - await sleep(0.2) - self._last_completed_lease = lease_scope.lease_name - self._lease_context = None - clear_log_context() - logger.debug("Ready for next lease") + with CancelScope(shield=True): + if session_was_created: + # Brief delay to ensure session is fully closed before next lease. + # Prevents SSL corruption from overlapping connections. + await sleep(0.2) + self._last_completed_lease = lease_scope.lease_name + self._lease_context = None + clear_log_context() + logger.debug("Ready for next lease")This was raised as a follow-up note on the previous ordering comment.
🤖 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/exporter/exporter.py` around lines 1001 - 1011, Shield the settle delay in the finally block of the lease cleanup flow so cancellation during await sleep(0.2) cannot skip the subsequent state reset. Ensure _last_completed_lease is assigned, _lease_context is cleared, clear_log_context() runs, and the “Ready for next lease” log remains executed even when the surrounding task group is cancelled.
🧹 Nitpick comments (5)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (3)
200-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
_last_completed_leaselike the neighboring state fields.Every other internal state field in this dataclass carries a docstring. The lifecycle of this field is non-obvious:
handle_leasesets it in thefinallyblock,_apply_statusclears it on aleased=falsetick, and the guard at Line 1061 uses it to drop trailing ticks.📝 Proposed docstring
_last_completed_lease: str | None = field(init=False, default=None) + """Name of the lease whose handle_lease task most recently completed. + + Set in handle_lease()'s finally block, cleared when the controller reports + leased=false. Used to ignore trailing leased=true status ticks that still + reference an already-finished lease. + """ _lease_context: LeaseContext | None = field(init=False, default=None)🤖 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/exporter/exporter.py` at line 200, Add a docstring for the `_last_completed_lease` dataclass field matching the neighboring internal state-field documentation, describing that `handle_lease` sets it on completion, `_apply_status` clears it when `leased=false`, and the trailing-tick guard uses it to discard late ticks.
893-918: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe stale-lease return at Line 918 leaves
listen_tx/listen_rxunclosed.
listen_txandlisten_rxare created at Line 896, but thereturnat Line 918 exits before the innerfinallyat Line 984 that callsawait listen_tx.aclose(). No producer or consumer task is attached yet, so nothing hangs; anyio can still emit aResourceWarningfor the unclosed streams. Create the streams after the second stale-lease check.♻️ Proposed reorder
logger.info("Listening for incoming connection requests on lease %s", lease_name) - # Buffer Listen responses to avoid blocking when responses arrive before - # process_connections starts iterating. This prevents a race condition where - # the client dials immediately after lease acquisition but before the session is ready. - listen_tx, listen_rx = create_memory_object_stream[jumpstarter_pb2.ListenResponse](max_buffer_size=10) - # Create session for the lease duration and populate lease_scopeThen create the streams immediately after the
_skip_stale_lease(..., "during session setup")check at Line 917.🤖 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/exporter/exporter.py` around lines 893 - 918, Move the listen_tx/listen_rx creation out of the pre-session setup and place it immediately after the _skip_stale_lease(..., "during session setup") check in the lease-serving flow. Preserve the existing stream configuration and all later uses, ensuring the stale-lease return occurs before either stream is allocated.
1053-1082: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicate
current_leasedbranching and skip the update on an empty lease name.Two points in this block:
- Line 1056 tests
not current_leased, and Line 1059 testscurrent_leasedagain. Move the_last_completed_lease = Nonereset into theelsebranch so the state has one branch per outcome.- If
current_leasedis true andstatus.lease_nameis""while the state isIDLE, neither inner branch runs and control reaches_on_lease_update(status)at Line 1079._lease_contextisNonethere, so the only effect is the log line"Currently leased by under ". Return early instead.♻️ Proposed restructure
previous_state = self._lease_state current_leased = status.leased - if not current_leased: - self._last_completed_lease = None - - if current_leased: - if previous_state == LeaseState.IDLE and status.lease_name != "": + if current_leased: + if previous_state == LeaseState.IDLE: + if status.lease_name == "": + logger.warning("Received leased status without a lease name; ignoring") + return False if status.lease_name == self._last_completed_lease: logger.debug("Ignoring trailing status for completed lease %s", status.lease_name) return False self._on_lease_acquired(status, tg) elif ( previous_state == LeaseState.LEASED and self._lease_context and self._lease_context.lease_name != status.lease_name ): # May briefly reject new leases while handle_lease finishes # session teardown; the next status tick will acquire them. logger.error( "Received lease %s while still handling %s; ignoring", status.lease_name, self._lease_context.lease_name, ) return False self._on_lease_update(status) else: + self._last_completed_lease = None await self._on_lease_released(previous_state)🤖 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/exporter/exporter.py` around lines 1053 - 1082, Refactor the current_leased handling in the status-processing method to use a single if/else: keep lease release handling in the false branch and move _last_completed_lease = None there before awaiting _on_lease_released(previous_state). In the true branch, return early when previous_state is LeaseState.IDLE and status.lease_name is empty, before _on_lease_update(status); preserve the existing acquisition, duplicate-completion, and conflicting-lease behavior.python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (2)
1370-1410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback path is not actually exercised by this test.
With
hook_executor=None,handle_lease's main "no hook" branch inside thetryblock runsawait self._report_status(...)thenlease_scope.before_lease_hook.set()before thefinallyfallback check (not lease_scope.before_lease_hook.is_set()) is reached. In this test,lease_ctx.lease_ended.set()happens only afterawait anyio.sleep(0.1), well after that primary branch already completes and sets the event.As a result, the assertion
assert lease_ctx.before_lease_hook.is_set()passes through the primary code path, not through thefinallyfallback. Deleting the fallback line inhandle_leasewould not make this test fail, so it does not protect against a regression of the deadlock-prevention fallback described in the comment above that code.To exercise the fallback, trigger the lease-end cancellation before the "no hook" branch reaches its checkpoint, for example by setting
lease_ctx.lease_endedbefore spawninghandle_lease, or by havingfake_retry_streamcalllease_ctx.lease_ended.set()synchronously as its first action.🧪 Proposed fix to exercise the fallback path
async def fake_retry_stream(name, factory, tx, **kwargs): + lease_ctx.lease_ended.set() await tx.aclose() exporter._retry_stream = fake_retry_stream exporter._listen_stream_factory = MagicMock(return_value=MagicMock()) async with create_task_group() as tg: tg.start_soon(exporter.handle_lease, "fallback-lease", tg, lease_ctx) - await anyio.sleep(0.1) - lease_ctx.lease_ended.set() await anyio.sleep(0.1) tg.cancel_scope.cancel()🤖 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/exporter/exporter_test.py` around lines 1370 - 1410, Update test_handle_lease_finally_sets_before_lease_hook_fallback so lease_ctx.lease_ended is triggered before the no-hook branch can set before_lease_hook, preferably by setting it before spawning handle_lease or as the first action in fake_retry_stream. Keep the assertion and cleanup verification, ensuring the test fails if the finally fallback in handle_lease is removed.
1191-1199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixed-duration sleeps used as test synchronization barriers.
Three tests use
await anyio.sleep(0.05)to wait for a background task to reach a certain point before asserting. Under CI load, a fixed sleep can complete before the background task finishes, causing intermittent, hard-to-reproduce test failures. The shared root cause is the absence of an explicit completion signal for the spawned background work.
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py#L1191-L1199: replaceawait anyio.sleep(0.05)with an explicit wait on an event set insidefake_handle_leaseonce it recordslease_name.python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py#L1226-L1229: replaceawait anyio.sleep(0.05)with an explicit wait on an event set byfake_handle_lease(or a second event set byfake_before_hook), sincefake_before_hookalready setslease_scope.before_lease_hook.python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py#L1359-L1365: replaceawait anyio.sleep(0.05)with an explicit wait on an event set by a fake_cleanup_after_leasewrapper before delegating to theAsyncMock.🤖 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/exporter/exporter_test.py` around lines 1191 - 1199, Replace the fixed-duration sleeps in exporter_test.py at lines 1191-1199, 1226-1229, and 1359-1365 with explicit AnyIO event synchronization. In the tests around fake_handle_lease, await an event set after lease_name is recorded; at 1226-1229, use fake_handle_lease’s event or an event from fake_before_hook after setting lease_scope.before_lease_hook; at 1359-1365, await an event set by a fake _cleanup_after_lease wrapper before it delegates to the AsyncMock.
🤖 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.
Duplicate comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 1001-1011: Shield the settle delay in the finally block of the
lease cleanup flow so cancellation during await sleep(0.2) cannot skip the
subsequent state reset. Ensure _last_completed_lease is assigned, _lease_context
is cleared, clear_log_context() runs, and the “Ready for next lease” log remains
executed even when the surrounding task group is cancelled.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 1370-1410: Update
test_handle_lease_finally_sets_before_lease_hook_fallback so
lease_ctx.lease_ended is triggered before the no-hook branch can set
before_lease_hook, preferably by setting it before spawning handle_lease or as
the first action in fake_retry_stream. Keep the assertion and cleanup
verification, ensuring the test fails if the finally fallback in handle_lease is
removed.
- Around line 1191-1199: Replace the fixed-duration sleeps in exporter_test.py
at lines 1191-1199, 1226-1229, and 1359-1365 with explicit AnyIO event
synchronization. In the tests around fake_handle_lease, await an event set after
lease_name is recorded; at 1226-1229, use fake_handle_lease’s event or an event
from fake_before_hook after setting lease_scope.before_lease_hook; at 1359-1365,
await an event set by a fake _cleanup_after_lease wrapper before it delegates to
the AsyncMock.
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Line 200: Add a docstring for the `_last_completed_lease` dataclass field
matching the neighboring internal state-field documentation, describing that
`handle_lease` sets it on completion, `_apply_status` clears it when
`leased=false`, and the trailing-tick guard uses it to discard late ticks.
- Around line 893-918: Move the listen_tx/listen_rx creation out of the
pre-session setup and place it immediately after the _skip_stale_lease(...,
"during session setup") check in the lease-serving flow. Preserve the existing
stream configuration and all later uses, ensuring the stale-lease return occurs
before either stream is allocated.
- Around line 1053-1082: Refactor the current_leased handling in the
status-processing method to use a single if/else: keep lease release handling in
the false branch and move _last_completed_lease = None there before awaiting
_on_lease_released(previous_state). In the true branch, return early when
previous_state is LeaseState.IDLE and status.lease_name is empty, before
_on_lease_update(status); preserve the existing acquisition,
duplicate-completion, and conflicting-lease behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f4fe91a-dff6-4a8f-89b3-13c5ef50a73c
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
1001-1011: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winShield the settle delay so the cleanup still runs on cancellation.
The delay ordering fix is correct. The remaining gap is cancellation.
await sleep(0.2)at Line 1007 is not shielded. When the surrounding task group is cancelled, thatawaitraises immediately and Lines 1008-1010 never run._lease_contextthen stays set, so_lease_statestaysLEASEDand a later lease is rejected by theLEASEDbranch in_apply_status. A prior review raised this same sub-point.🔧 Proposed fix
finally: if self._lease_context is lease_scope: session_was_created = lease_scope.session is not None - if session_was_created: - # Brief delay to ensure session is fully closed before next lease. - # Prevents SSL corruption from overlapping connections. - await sleep(0.2) - self._last_completed_lease = lease_scope.lease_name - self._lease_context = None - clear_log_context() - logger.debug("Ready for next lease") + with CancelScope(shield=True): + if session_was_created: + # Brief delay to ensure session is fully closed before next lease. + # Prevents SSL corruption from overlapping connections. + await sleep(0.2) + self._last_completed_lease = lease_scope.lease_name + self._lease_context = None + clear_log_context() + logger.debug("Ready for next lease")🤖 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/exporter/exporter.py` around lines 1001 - 1011, Shield the settle delay in the cleanup block of the lease-management method so task cancellation cannot skip the remaining finalization. Wrap the await of sleep(0.2) in the project’s supported cancellation-shield mechanism, while preserving the existing session_was_created condition and ensuring _last_completed_lease, _lease_context, clear_log_context(), and the debug log always execute.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
200-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
_last_completed_lease.Every other internal state field in this class has a docstring. The purpose of this field is not obvious from its name. Add a short docstring that states it suppresses trailing
leased=trueticks for a lease thathandle_leasealready finished, and that_apply_statusclears it on the firstleased=falsetick.♻️ Proposed docstring
_last_completed_lease: str | None = field(init=False, default=None) + """Name of the last lease whose handle_lease task completed. + + Set in handle_lease's finally block and cleared on the first not-leased + status. Used by _apply_status to ignore trailing leased=true ticks that + refer to an already-finished lease. + """ _lease_context: LeaseContext | None = field(init=False, default=None)🤖 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/exporter/exporter.py` around lines 200 - 201, Add a concise docstring to the `_last_completed_lease` field documenting that it suppresses trailing `leased=true` ticks for leases already completed by `handle_lease`, and that `_apply_status` clears it on the first `leased=false` tick. Leave the field behavior unchanged.
🤖 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 `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Line 896: Ensure the Listen memory streams created by
create_memory_object_stream in the exporter method are closed on the stale-lease
return path. Wrap the remaining listen-processing flow, including the
session_for_lease block and stale-lease check, in an async context manager for
listen_tx and listen_rx, while preserving the existing inner cleanup behavior.
- Around line 1070-1073: Update the comment in handle_lease to replace the
nonexistent _finalize_lease_context reference with the handle_lease finally
block that clears _lease_context.
---
Duplicate comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 1001-1011: Shield the settle delay in the cleanup block of the
lease-management method so task cancellation cannot skip the remaining
finalization. Wrap the await of sleep(0.2) in the project’s supported
cancellation-shield mechanism, while preserving the existing session_was_created
condition and ensuring _last_completed_lease, _lease_context,
clear_log_context(), and the debug log always execute.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 200-201: Add a concise docstring to the `_last_completed_lease`
field documenting that it suppresses trailing `leased=true` ticks for leases
already completed by `handle_lease`, and that `_apply_status` clears it on the
first `leased=false` tick. Leave the field behavior unchanged.
🪄 Autofix (Beta)
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: 0cd97282-f41d-41d5-8dc2-afd7f1360899
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
| # Controller reassigned the exporter to a different lease. | ||
| # Signal the old lease to tear down; the next status tick | ||
| # will acquire the new lease once _finalize_lease_context | ||
| # clears _lease_context. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale method name in the comment.
The comment refers to _finalize_lease_context. No such method exists in this file. handle_lease's finally block at Lines 1001-1011 clears _lease_context. Update the comment so readers can find the code that performs the clear.
🔧 Proposed fix
# Controller reassigned the exporter to a different lease.
# Signal the old lease to tear down; the next status tick
- # will acquire the new lease once _finalize_lease_context
- # clears _lease_context.
+ # will acquire the new lease once handle_lease's finally
+ # block clears _lease_context.📝 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.
| # Controller reassigned the exporter to a different lease. | |
| # Signal the old lease to tear down; the next status tick | |
| # will acquire the new lease once _finalize_lease_context | |
| # clears _lease_context. | |
| # Controller reassigned the exporter to a different lease. | |
| # Signal the old lease to tear down; the next status tick | |
| # will acquire the new lease once handle_lease's finally | |
| # block clears _lease_context. |
🤖 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/exporter/exporter.py` around lines
1070 - 1073, Update the comment in handle_lease to replace the nonexistent
_finalize_lease_context reference with the handle_lease finally block that
clears _lease_context.
- serve() → serve / _run_control_plane / _apply_status - Transition handlers: _on_lease_acquired, _on_lease_update, _on_lease_released, _check_stop_requested - Removes the C901 suppression - Status transitions are now unit-testable without task-group scaffolding" Next: jumpstarter-dev#948 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (2)
1486-1498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert the completed-lease guard.
The
finallyblock inhandle_leasesets_last_completed_leasetogether with clearing_lease_context._apply_statususes_last_completed_leaseto suppress trailing ticks. Asserting it here covers both halves of the finalization contract.🧪 Suggested assertion
assert exporter._lease_context is None + assert exporter._last_completed_lease == "cleanup-lease"🤖 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/exporter/exporter_test.py` around lines 1486 - 1498, Extend test_handle_lease_finally_clears_lease_context to also assert that exporter._last_completed_lease is set to the completed lease context after handle_lease finishes, covering the finalization guard alongside the existing _lease_context assertion.
1444-1484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider bounding this test with
fail_after.This test relies on fixed
anyio.sleepcalls to advancehandle_lease. If the fallback logic regresses and blocks, the test hangs instead of failing fast. The neighbouring test at Line 1435 already usesfail_after(5). Wrapping the task group infail_aftermakes the failure mode consistent.♻️ Suggested change
- async with create_task_group() as tg: - tg.start_soon(exporter.handle_lease, "fallback-lease", tg, lease_ctx) - await anyio.sleep(0.1) - lease_ctx.lease_ended.set() - await anyio.sleep(0.1) - tg.cancel_scope.cancel() + with fail_after(5): + async with create_task_group() as tg: + tg.start_soon(exporter.handle_lease, "fallback-lease", tg, lease_ctx) + await anyio.sleep(0.1) + lease_ctx.lease_ended.set() + await anyio.sleep(0.1) + tg.cancel_scope.cancel()🤖 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/exporter/exporter_test.py` around lines 1444 - 1484, Wrap the task-group execution in test_handle_lease_finally_sets_before_lease_hook_fallback with anyio.fail_after(5), matching the neighboring test’s timeout pattern. Keep the existing sleeps, lease-ended signaling, assertions, and cancellation behavior unchanged.
🤖 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/exporter/exporter_test.py`:
- Around line 1486-1498: Extend test_handle_lease_finally_clears_lease_context
to also assert that exporter._last_completed_lease is set to the completed lease
context after handle_lease finishes, covering the finalization guard alongside
the existing _lease_context assertion.
- Around line 1444-1484: Wrap the task-group execution in
test_handle_lease_finally_sets_before_lease_hook_fallback with
anyio.fail_after(5), matching the neighboring test’s timeout pattern. Keep the
existing sleeps, lease-ended signaling, assertions, and cancellation behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 637bd7f9-c115-4c23-a7af-6bd85e283800
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter/jumpstarter/exporter/exporter.py
| pending = self._pending_lease_status | ||
| if pending is not None: | ||
| self._pending_lease_status = None | ||
| if self._status_replay_tx is not None: | ||
| await self._status_replay_tx.send(pending) |
There was a problem hiding this comment.
If the task group is being cancelled during shutdown, _status_replay_tx may already be closed. send() on a closed stream would raise ClosedResourceError. Replaying a stashed status during shutdown is pointless anyway, so this should be guarded:
pending = self._pending_lease_status
if pending is not None:
self._pending_lease_status = None
if self._status_replay_tx is not None:
try:
await self._status_replay_tx.send(pending)
except (ClosedResourceError, EndOfStream):
logger.debug("Status channel closed, skipping replay for %s", pending.lease_name)| _last_completed_lease: str | None = field(init=False, default=None) | ||
| _pending_lease_status: jumpstarter_pb2.StatusResponse | None = field(init=False, default=None) | ||
| _status_replay_tx: MemoryObjectSendStream | None = field(init=False, default=None) |
There was a problem hiding this comment.
nit: I would document those attributes to make them more evident (also other fields have docstrings), also docstrings need maintenance so it's a double side sword... since a nit, I leave it up to your criteria :D
_last_completed_lease: str | None = field(init=False, default=None)
"""Name of the most recently completed lease, used to filter trailing status ticks
after handle_lease's finally has cleaned up. Cleared on the next leased=False tick."""
_pending_lease_status: jumpstarter_pb2.StatusResponse | None = field(init=False, default=None)
"""Stashed status for a reassigned lease. When the controller reassigns the exporter
to a new lease while the old one is still tearing down, the new status is held here
and replayed by handle_lease's finally block after cleanup completes."""
_status_replay_tx: MemoryObjectSendStream | None = field(init=False, default=None)
"""Reference to the status channel TX end, set by _run_control_plane. Used by
handle_lease's finally to replay stashed reassignment statuses into the status loop."""| @@ -847,7 +853,7 @@ async def _skip_stale_lease(self, lease_name: str, lease_scope: LeaseContext, co | |||
| lease_scope.after_lease_hook_done.set() | |||
| return True | |||
|
|
|||
There was a problem hiding this comment.
temporarily, needed the lint to pass in this PR 😅
|
There is a failure in E2E related to lease status: https://github.com/jumpstarter-dev/jumpstarter/actions/runs/30887610848/job/91922876687?pr=948#step:10:1773 I am trying to figure out if it could be related. |
| finally: | ||
| if self._lease_context is lease_scope: | ||
| session_was_created = lease_scope.session is not None | ||
| if session_was_created: | ||
| # Brief delay to ensure session is fully closed before next lease. | ||
| # Prevents SSL corruption from overlapping connections. | ||
| await sleep(0.2) | ||
| self._last_completed_lease = lease_scope.lease_name | ||
| self._lease_context = None | ||
| clear_log_context() | ||
| logger.debug("Ready for next lease") |
There was a problem hiding this comment.
_lease_state derives from _lease_context, but this finally block clears _lease_context = None concurrently with _apply_status. When handle_lease completes before the controller's leased=False tick arrives, _apply_status sees previous_state == IDLE and skips the exit_on_lease_end check in _on_lease_released. The exporter keeps running instead of shutting down.
|
This is what claude determined, when I saw it was a status transition I thought it could be related, I am looking at the code now. E2E Failure Analysis:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (2)
1445-1485: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test does not exercise the
finallyfallback.
handle_leasesetsbefore_lease_hookin the no-hook branch, right after startingconn_tg, before thefinallyblock runs.exporter.hook_executorisNonehere, so the event is already set by that normal path. The assertion at line 1484 passes even if thefinallyfallback is removed.To cover the fallback, prevent the no-hook branch from reaching
before_lease_hook.set(), for example by making_report_statusraise or by cancellingconn_tgbefore that line, and then assert the event is still set afterhandle_leasereturns.🤖 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/exporter/exporter_test.py` around lines 1445 - 1485, Update test_handle_lease_finally_sets_before_lease_hook_fallback so execution cannot reach the normal no-hook before_lease_hook.set() path, such as by making _report_status raise or cancelling conn_tg before that point. Ensure handle_lease completes and assert before_lease_hook is set only because of the finally fallback, while retaining the cleanup assertion.
1353-1377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet the production code perform the replay send.
The test assigns
_status_replay_tx, but nothing in production code reads it here. Lines 1368-1374 hand-roll the cleanup and the replay send thathandle_lease'sfinallyblock performs. The test therefore proves only that_apply_statusaccepts a replayed status; it does not prove that the pending status reaches_status_replay_tx.Drive the real finalization path so the send is exercised, for example by calling
exporter._finalize_lease_context(lease_ctx_a)(or completing a stubbedhandle_lease) and then receiving fromstatus_rxwithout sending manually.🤖 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/exporter/exporter_test.py` around lines 1353 - 1377, Update the test to exercise the production replay path instead of manually clearing lease state and sending the pending status. After applying status_b, invoke exporter._finalize_lease_context(lease_ctx_a) or complete a stubbed handle_lease so its finalization logic sends through _status_replay_tx, then receive the replayed status from status_rx and pass it to _apply_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 `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 1636-1656: The test test_finalize_skips_when_already_cleared calls
the removed Exporter._finalize_lease_context method and will raise
AttributeError. Remove or relocate this stale direct call and assert the
equivalent no-op behavior through the current _apply_status and lease-release
flow, unless the method is intentionally restored on Exporter.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 1445-1485: Update
test_handle_lease_finally_sets_before_lease_hook_fallback so execution cannot
reach the normal no-hook before_lease_hook.set() path, such as by making
_report_status raise or cancelling conn_tg before that point. Ensure
handle_lease completes and assert before_lease_hook is set only because of the
finally fallback, while retaining the cleanup assertion.
- Around line 1353-1377: Update the test to exercise the production replay path
instead of manually clearing lease state and sending the pending status. After
applying status_b, invoke exporter._finalize_lease_context(lease_ctx_a) or
complete a stubbed handle_lease so its finalization logic sends through
_status_replay_tx, then receive the replayed status from status_rx and pass it
to _apply_status.
🪄 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: fa8818e5-0cf2-4a00-ab6a-227ff7013404
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter/jumpstarter/exporter/exporter.py
e6db6cd to
970d7bf
Compare
bkhizgiy
left a comment
bkhizgiy
left a comment
There was a problem hiding this comment.
I don't see any issues, good from my side.
Introduce LeaseState enum and _lease_state property derived from _lease_context, eliminating a class of state-synchronization bugs where _previous_leased could drift from _lease_context. Key changes: - Wrap handle_lease body in try/finally so early returns (stale lease, session setup race) always clean up _lease_context and log context - Reject overlapping leases in _apply_status instead of silently replacing _lease_context, preventing concurrent handle_lease tasks - Move before-lease hook spawn into _on_lease_acquired for cleaner ownership of lease startup - Remove stale _previous_leased from test fixtures Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assited-by: claude-opus-4.6
4 participants
Introduce LeaseState enum and _lease_state property derived from
_lease_context, eliminating a class of state-synchronization bugs
where _previous_leased could drift from _lease_context.
Key changes:
session setup race) always clean up _lease_context and log context
replacing _lease_context, preventing concurrent handle_lease tasks
ownership of lease startup
Depends on #947
Next: #949