fix: transition exporter to AVAILABLE after afterLease hook failure#871
fix: transition exporter to AVAILABLE after afterLease hook failure#871evakhoni wants to merge 1 commit into
Conversation
- Report AVAILABLE status in finally block before calling request_lease_release() — ensures exporter can accept new leases after on_failure=endLease or unexpected errors - Update comments to reflect that finally block handles the AVAILABLE transition, not just lease release - Add test for afterLease hook failure with on_failure=endLease - Add test for afterLease hook unexpected error Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughFixes a bug where an exporter permanently stuck in AfterLeaseHookFailed status after an afterLease hook failure (endLease or unexpected exception) by explicitly reporting AVAILABLE in the finally block before requesting lease release, with clarifying comments and two new regression tests. ChangesAfterLease hook failure recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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)
1537-1581: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
AFTER_LEASE_HOOK_FAILEDis reported beforeAVAILABLE.Both tests verify that both statuses appear but don't verify ordering. A future refactor that accidentally swaps the sequence would pass these tests while breaking the recovery contract.
♻️ Proposed addition for both tests
Add after the existing
failed_statuses/available_statusesassertions:+ # Verify FAILED is reported before AVAILABLE + status_sequence = [s for s, _ in status_calls] + failed_idx = status_sequence.index(ExporterStatus.AFTER_LEASE_HOOK_FAILED) + available_idx = status_sequence.index(ExporterStatus.AVAILABLE) + assert failed_idx < available_idx, ( + f"AFTER_LEASE_HOOK_FAILED must be reported before AVAILABLE, got: {status_calls}" + )Also applies to: 1583-1633
🤖 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 1537 - 1581, The after-lease hook tests in HookExecutor.run_after_lease_hook currently only assert that AFTER_LEASE_HOOK_FAILED and AVAILABLE are both reported, but they do not verify the required sequence. Update the relevant tests to assert ordering by checking the relative positions in status_calls so AFTER_LEASE_HOOK_FAILED is reported before AVAILABLE, covering both the endLease failure path and the other applicable test in hooks_test.py.
🤖 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 853-857: The cleanup path in the exporter hook should not let a
transient failure in report_status stop lease release. In the finally block
around request_lease_release, wrap the report_status(ExporterStatus.AVAILABLE,
...) call in its own try/except, log or ignore that failure as appropriate, and
then always continue to request_lease_release so the lease is still freed. Use
the existing report_status and request_lease_release symbols in this hook to
locate the change.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py`:
- Around line 1537-1581: The after-lease hook tests in
HookExecutor.run_after_lease_hook currently only assert that
AFTER_LEASE_HOOK_FAILED and AVAILABLE are both reported, but they do not verify
the required sequence. Update the relevant tests to assert ordering by checking
the relative positions in status_calls so AFTER_LEASE_HOOK_FAILED is reported
before AVAILABLE, covering both the endLease failure path and the other
applicable test in hooks_test.py.
🪄 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: 59ffe5a7-6596-4ca0-8f84-e88132b994a7
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/hooks.pypython/packages/jumpstarter/jumpstarter/exporter/hooks_test.py
| # Transition to AVAILABLE to clear AFTER_LEASE_HOOK_FAILED. | ||
| # Idempotent if already AVAILABLE from the happy/warn paths. | ||
| await report_status(ExporterStatus.AVAILABLE, "Available for new lease") | ||
| try: | ||
| await request_lease_release() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap report_status in try/except to guarantee request_lease_release runs.
If report_status raises (e.g., transient gRPC failure), execution exits the finally block before reaching request_lease_release(), leaving the lease unreleased and the exporter stuck — the same class of bug this PR fixes. The request_lease_release call below is already protected; report_status should be too.
🔒 Proposed fix: protect report_status call
if request_lease_release and not shutdown_called:
# Transition to AVAILABLE to clear AFTER_LEASE_HOOK_FAILED.
# Idempotent if already AVAILABLE from the happy/warn paths.
- await report_status(ExporterStatus.AVAILABLE, "Available for new lease")
+ try:
+ await report_status(ExporterStatus.AVAILABLE, "Available for new lease")
+ except Exception as e:
+ logger.error("Failed to report AVAILABLE status: %s", e, exc_info=True)
try:
await request_lease_release()
except Exception as e:
logger.error("Failed to request lease release: %s", e, exc_info=True)📝 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.
| # Transition to AVAILABLE to clear AFTER_LEASE_HOOK_FAILED. | |
| # Idempotent if already AVAILABLE from the happy/warn paths. | |
| await report_status(ExporterStatus.AVAILABLE, "Available for new lease") | |
| try: | |
| await request_lease_release() | |
| # Transition to AVAILABLE to clear AFTER_LEASE_HOOK_FAILED. | |
| # Idempotent if already AVAILABLE from the happy/warn paths. | |
| try: | |
| await report_status(ExporterStatus.AVAILABLE, "Available for new lease") | |
| except Exception as e: | |
| logger.error("Failed to report AVAILABLE status: %s", e, exc_info=True) | |
| try: | |
| await request_lease_release() |
🤖 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 853 -
857, The cleanup path in the exporter hook should not let a transient failure in
report_status stop lease release. In the finally block around
request_lease_release, wrap the report_status(ExporterStatus.AVAILABLE, ...)
call in its own try/except, log or ignore that failure as appropriate, and then
always continue to request_lease_release so the lease is still freed. Use the
existing report_status and request_lease_release symbols in this hook to locate
the change.
|
I am not sure if this was intended behavior if afterlease is failing, we may want the admin to look at it? |
|
@mangelajo I'm not sure we should really allow the |
May be this is something we can combine now with the "enabled" flag of the exporters, we could just flip the flag off and add a "disableExporter" mode or similar.. |
|
interesting points. so basically what you @kirkbrauer saying is for being a no-op in this case, this should not even exists as an option? it's a good point, however I'm not sure if we should put a hard restriction on it, or just document it for being a no-op? after al it works as described, even though it's functionally useless, so it's a question of should it be left for the user judgement for not to use it, or restricted it while introducing an inconsistency with @mangelajo , seems like you suggesting to repurpose it into a different functionality 🤔 I'm not sure do we even benefit from keeping an exporter online but disabled for an admin to interact with, while we already have onFailure exit for that? do the admin even need that? if yes, do we also needs the other mode when it actually exits the process, or we can just set onfailure exit to always set offline or just disabled but keeping the process alive for debug? or if we do decide that we need two different modes, then what about |
3 participants
Fixes #870
Problem
When an afterLease hook fails with
on_failure: endLeaseor encounters an unexpected error, the exporter gets permanently stuck inAfterLeaseHookFailedstatus and cannot accept new leases.Root cause: The
run_after_lease_hook()finally block callsrequest_lease_release(), which sendsAVAILABLEstatus to the controller via gRPC but doesn't update the exporter's internal_exporter_statusfield.Solution
Add
await report_status(ExporterStatus.AVAILABLE, ...)in the finally block before callingrequest_lease_release(). This ensures both the internal state and controller are updated.Testing
AfterLeaseHookFailed→Availableand accepts subsequent leases