fix: skip processing for already-ended leases#862
Open
evakhoni wants to merge 2 commits into
Open
Conversation
Check lease_ended before executing hooks and creating sessions so stale leases are drained without spawning subprocesses or allocating sockets. Fix race in handle_lease finally block where unconditional before_lease_hook.set() could override skip_after_lease_hook. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExporter lease handling now detects stale leases earlier, skips hook/session work when a lease has already ended, and tightens cleanup around lease completion. Hook readiness waiting was also split out so before-lease execution can stop cleanly on lease-end or timeout, with tests updated for both paths. ChangesStale lease handling in exporter and hooks
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Serve
participant HandleLease
participant SkipStaleLease
participant SessionForLease
participant ReportStatus
Serve->>HandleLease: start lease handling
HandleLease->>HandleLease: await sleep(0)
HandleLease->>SkipStaleLease: check lease_ended before session
alt lease already ended
SkipStaleLease->>ReportStatus: report AVAILABLE
SkipStaleLease-->>HandleLease: return early
else lease active
HandleLease->>SessionForLease: enter session context
HandleLease->>SkipStaleLease: re-check lease_ended during setup
alt lease ended during setup
SkipStaleLease->>ReportStatus: report AVAILABLE
SkipStaleLease-->>HandleLease: return early
else lease still active
HandleLease->>HandleLease: continue connection handling
end
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_test.py`:
- Around line 1627-1669: Add a regression test around
HookExecutor.run_before_lease_hook for the case where config.before_lease is
unset and only after_lease is configured, with lease_scope.lease_ended already
set while lease_scope.is_ready() is still true. The current test covers the
_wait_for_lease_ready bail-out path, but not this ordering case; create a
focused test that verifies the executor exits immediately without waiting, does
not report status, and still leaves the lease_scope state consistent for
downstream hooks.
In `@python/packages/jumpstarter/jumpstarter/exporter/hooks.py`:
- Around line 666-684: The lease-ended check in the before-lease flow should run
before the no-hook fast path in the hook handling logic. Update the
`before_lease` path around `_wait_for_lease_ready()` so
`lease_scope.lease_ended.is_set()` is evaluated first, then skip the hook and
set `skip_after_lease_hook` if needed, before any
`self.config.before_lease`/`LEASE_READY` handling. This ensures stale leases do
not report ready when only `after_lease` is configured and `hook_executor` is
present.
🪄 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: 56bbc204-5696-496b-a500-10a214ef940d
📒 Files selected for processing (4)
python/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.pypython/packages/jumpstarter/jumpstarter/exporter/hooks.pypython/packages/jumpstarter/jumpstarter/exporter/hooks_test.py
New tests: handle_lease skips session for stale leases, suppresses AVAILABLE on shutdown, bails after session creation when lease ends during setup. Hook bails during is_ready wait when lease_ended is set. _wait_for_lease_ready times out and reports BEFORE_LEASE_HOOK_FAILED when session is never populated. Updated tests: assert hook subprocess is never executed (not just LEASE_READY skipped) when lease is already ended, matching the new pre-execution lease_ended check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
evakhoni
force-pushed
the
fix/skip-stale-lease-processing
branch
from
d67bc41 to
e4e307b
Compare
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
1 participant
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.
Summary
When the exporter falls behind processing leases (e.g. temporary unavailability, resource starvation, or slow hooks), the client retry loop creates new leases faster than the exporter can process them. Each stale lease triggered full session creation, hook execution, and teardown — a cascading backlog that prevented recovery.
Three layers of stale lease detection:
hooks.py: check
lease_endedbefore executing hook subprocess, and during theis_readywait loop. Extracted_wait_for_lease_readyhelper to keep complexity under C901 limit. Both hooks (before and after lease) are skipped for stale leases.exporter.py
handle_lease: yield (sleep(0)) then checklease_endedbefore session creation; check again after session creation but before Listen stream and connection handling. Extracted_skip_stale_leasehelper for both check sites.exporter.py
serve: skip 0.2s inter-lease sleep when no session was created (no connections to overlap).Also fixes a race condition in
handle_lease's finally block wherebefore_lease_hook.set()was called unconditionally, racing withrun_before_lease_hook'sskip_after_lease_hookflag update. Now conditioned onnot self.hook_executor.Test plan
make pkg-test-jumpstarter)make lint-fix)make pkg-ty-jumpstarter)--lease) — no stale backlog, normal lifecycle unaffected