fix(hooks): release lease when beforeLease hook fails with onFailure=exit#827
fix(hooks): release lease when beforeLease hook fails with onFailure=exit#827mangelajo wants to merge 1 commit into
Conversation
…exit When a beforeLease hook fails with on_failure='exit', the exporter called shutdown(wait_for_lease_exit=True) which defers the actual exit until the lease ends. However, nobody released the lease, creating a deadlock: - serve() waits for the controller to send leased=False - The controller never sends leased=False because the lease was never released - handle_lease blocks on lease_ended.wait() which is never signaled - The exporter hangs forever in a zombie-like state The fix releases the lease before deferring shutdown by calling request_lease_release(), which: 1. Tells the controller to release the lease (release_lease=True) 2. Sets lease_ended as a fallback to unblock handle_lease This allows the serve() loop to receive leased=False from the controller, check _stop_requested, and proceed with the deferred shutdown. Also extracts _safe_release_lease() helper to keep run_before_lease_hook within the ruff C901 complexity limit. Fixes: #640
📝 WalkthroughWalkthroughThis change updates before-lease hook failure handling for ChangesBefore-lease exit flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py (1)
494-509: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winModel the release callback’s status side effect in this test.
A bare
AsyncMockonly proves the callback was invoked; it does not catch the production callback reportingAVAILABLEafter the exporter was markedOFFLINE. Add an assertion/fake callback that preserves the expected status sequence for the shutdown 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 `@python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py` around lines 494 - 509, The mock_report_status in the test is a bare AsyncMock that does not verify the status callback behavior during the shutdown path. Configure mock_report_status to model or assert the expected status sequence, specifically that it is called with the appropriate status values (such as OFFLINE and then AVAILABLE) during the lease hook execution. Add assertions or side effects to the mock_report_status mock to ensure the production behavior of reporting status changes during the shutdown flow is properly tested.
🤖 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/hooks.py`:
- Around line 681-688: The shutdown path in Exporter._safe_release_lease /
Exporter._request_lease_release is briefly reporting the exporter as AVAILABLE
after it has already been marked OFFLINE, and _stop_requested is being set too
late. Update the shutdown-specific lease release flow so it preserves OFFLINE
status instead of using the normal release path, and set the deferred stop
intent before unblocking lease cleanup. Make the change in the shutdown callback
path that calls shutdown(..., wait_for_lease_exit=True, should_unregister=True)
so release_lease=True does not advertise AVAILABLE during shutdown.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py`:
- Around line 494-509: The mock_report_status in the test is a bare AsyncMock
that does not verify the status callback behavior during the shutdown path.
Configure mock_report_status to model or assert the expected status sequence,
specifically that it is called with the appropriate status values (such as
OFFLINE and then AVAILABLE) during the lease hook execution. Add assertions or
side effects to the mock_report_status mock to ensure the production behavior of
reporting status changes during the shutdown flow is properly tested.
🪄 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
Run ID: d1db67e8-9ffa-4d71-948d-99939edd8811
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/hooks.pypython/packages/jumpstarter/jumpstarter/exporter/hooks_test.py
| # Release the lease so the controller sends leased=False, | ||
| # which unblocks the serve() loop to check _stop_requested. | ||
| # Without this, the exporter hangs waiting for a lease end | ||
| # that never comes. | ||
| await anyio.sleep(1.0) | ||
| await self._safe_release_lease(request_lease_release) | ||
| # Defer shutdown: sets _stop_requested=True, actual stop after lease cleanup | ||
| shutdown(exit_code=1, wait_for_lease_exit=True, should_unregister=True) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve OFFLINE and set stop intent before releasing the lease.
The runtime callback passed here is Exporter._request_lease_release, which reports ExporterStatus.AVAILABLE with release_lease=True before setting lease_ended; this runs after Lines 677-680 report OFFLINE and before Line 688 sets _stop_requested. That can briefly advertise a shutting-down exporter as available and can let lease cleanup observe the lease end before the deferred stop flag is set.
Use a shutdown-specific release path that keeps the exporter OFFLINE, and set the deferred stop intent before unblocking lease cleanup.
Suggested direction
- await anyio.sleep(1.0)
- await self._safe_release_lease(request_lease_release)
# Defer shutdown: sets _stop_requested=True, actual stop after lease cleanup
shutdown(exit_code=1, wait_for_lease_exit=True, should_unregister=True)
+ await anyio.sleep(1.0)
+ await self._safe_release_lease(request_lease_release)Also adjust the concrete release callback used for this path so release_lease=True does not report AVAILABLE after the exporter has been marked OFFLINE.
🤖 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/hooks.py` around lines 681 -
688, The shutdown path in Exporter._safe_release_lease /
Exporter._request_lease_release is briefly reporting the exporter as AVAILABLE
after it has already been marked OFFLINE, and _stop_requested is being set too
late. Update the shutdown-specific lease release flow so it preserves OFFLINE
status instead of using the normal release path, and set the deferred stop
intent before unblocking lease cleanup. Make the change in the shutdown callback
path that calls shutdown(..., wait_for_lease_exit=True, should_unregister=True)
so release_lease=True does not advertise AVAILABLE during shutdown.
|
Not a bug, see issue comments |
1 participant
Superseded by #823, which now fixes both #639 (endLease) and #640 (exit) in a single PR.
The
exitfix was incorporated into #823 with the correct semantics:shutdown(wait_for_lease_exit=False)for immediate exit without releasing the lease (the lease stays active so the client can observe the failure status)."