fix: retry transient gRPC failures in status reports#911
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe exporter adds bounded retries for status and lease-release RPCs, supports compatibility fallback for unsupported release APIs, and signals lease completion across outcomes. The controller now authenticates exporter-initiated release requests and handles release operations idempotently. ChangesExporter lease release and retry flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Exporter
participant Controller
participant Backoff
Exporter->>Controller: ReleaseLease request
Controller-->>Exporter: Success, unsupported, or transient error
Exporter->>Backoff: Retry transient failure
Backoff-->>Exporter: Retry delay completed
Exporter->>Controller: ReportStatus release fallback or AVAILABLE
Controller-->>Exporter: Status response
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 3
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (1)
541-558: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the promised exponential backoff schedule.
Both tests patch
anyio.sleepbut do not verify its arguments, so1,1,1or another incorrect delay sequence would pass. Assert[1.0, 2.0, 4.0]for exhaustion and[1.0, 2.0]for recovery.
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py#L541-L558: assert all three retry delays.python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py#L590-L595: assert the two delays before recovery.As per coding guidelines, “Provide comprehensive package test coverage.”
🤖 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 541 - 558, Update the _report_status retry tests in python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py at lines 541-558 and 590-595 to inspect the patched anyio.sleep calls: assert [1.0, 2.0, 4.0] for exhaustion and [1.0, 2.0] for recovery, preserving the existing warning and error assertions.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.
Inline comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 443-446: Update the lease-release result flow around the success
handling in the exporter to distinguish an unsupported RPC (UNIMPLEMENTED) from
an actual delivery failure. Propagate a distinct unsupported outcome from the
retry/request logic, and only emit the “Failed to request lease release after
retries” error for genuine failed attempts; preserve the existing warning for
unsupported controllers and success logging for delivered requests.
- Around line 436-442: Update the release flow around _send_report_status_rpc
and ReportStatusRequest to include the lease ID being released as an
expected-lease precondition. Propagate this identity through the protocol and
controller release operation, and atomically reject requests whose expected ID
does not match the currently assigned lease. Ensure retries after UNAVAILABLE
still target only the original lease and cannot release a newly assigned one.
- Around line 365-384: The ReportStatus retry loop lacks a per-attempt deadline,
allowing a call to hang indefinitely. Update the ReportStatus invocation inside
the retry loop to pass a short timeout, then handle
grpc.StatusCode.DEADLINE_EXCEEDED explicitly according to the existing retry
policy—retry it as transient while attempts remain, or return False immediately
if deadline failures should not retry.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 541-558: Update the _report_status retry tests in
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py at lines
541-558 and 590-595 to inspect the patched anyio.sleep calls: assert [1.0, 2.0,
4.0] for exhaustion and [1.0, 2.0] for recovery, preserving the existing warning
and error assertions.
🪄 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: 6a90d77c-0769-4675-990a-011d049d8e31
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
Extract ReportStatus gRPC call into _send_report_status_rpc with retry logic for transient errors (UNAVAILABLE, DEADLINE_EXCEEDED): - Max 3 retries (4 total attempts) - Exponential backoff: 1s, 2s, 4s - UNIMPLEMENTED handled specially (no retry, warning log) - Non-transient errors fail fast Both _report_status and _request_lease_release now use this shared method, making all status transitions resilient to transient failures. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add tests proving retry logic works: - test_report_status_retries_on_transient_failure: transient error resolves on 3rd attempt, status delivered - test_report_status_does_not_retry_non_transient_grpc_error: PERMISSION_DENIED fails fast (no retry) - test_report_status_does_not_retry_non_grpc_exception: ConnectionError fails fast (no retry) - test_request_lease_release_retries_on_transient_failure: _request_lease_release also retries via shared logic Update existing test for retry behavior: - test_other_grpc_error_logs_error: patch anyio.sleep, verify 3 retry warnings logged before final error Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
da12fa5 to
fd51b13
Compare
Retrying release_lease=true is unsafe: if the controller processed it but the response was lost, a retry could release a newly-assigned lease (no lease identity in the request). Split _request_lease_release into phases: 1. Send release_lease=true once (no retry) 2. On failure, retry just status=AVAILABLE via _report_status (idempotent) The controller already ended the lease before afterLease runs, so release_lease=true is a courtesy - the critical part is AVAILABLE status. Documentation: - Fix _send_report_status_rpc docstring (UNIMPLEMENTED returns False) - Add comments clarifying UNIMPLEMENTED is legacy support for controllers before Nov 2025 (b76f6c8), safe to remove in future versions Tests: update existing + add two new tests for success and UNIMPLEMENTED. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace two boolean flags (released, unimplemented) with one (should_retry) for clearer intent and simpler future removal of UNIMPLEMENTED legacy support. Changes: - Use should_retry flag instead of double-negative condition (if should_retry vs if not released and not unimplemented) - Simplify Phase 2 comment - legacy context is already documented at the UNIMPLEMENTED check itself (line 455-458) - Remove unreachable return False at end of _send_report_status_rpc loop (all paths explicitly return inside the loop) No functional changes - all tests pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Extract test fixture to deduplicate mock setup: - Add _setup_mock_controller_stub helper to eliminate 40+ lines of repeated boilerplate across 8 tests - Makes test assertions more visible and easier to maintain Remove single-use success variable: - Inline _send_report_status_rpc return check in _report_status - Add comment clarifying error logging is handled by the helper Simplify duplicate UNIMPLEMENTED comment: - Replace verbose duplicate in _request_lease_release with cross-reference to _send_report_status_rpc where full context is documented Add missing call_count assertion: - Validate test_other_grpc_error_logs_error makes exactly 4 RPC attempts - Catches potential off-by-one errors the backoff schedule alone might miss Add retry exhaustion test: - test_request_lease_release_exhausts_retries verifies critical safety: even when all retries fail, lease_ended is still set so handle_lease can exit (prevents permanent deadlock) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (3)
519-533: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that
UNIMPLEMENTEDis not retried.This test checks logging only, so a retry regression could still pass. Add
assert mock_controller.ReportStatus.call_count == 1to enforce the fail-fast contract.As per coding guidelines, Python tests should provide comprehensive package coverage; the PR objective requires
UNIMPLEMENTEDto 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 `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py` around lines 519 - 533, Update the UNIMPLEMENTED handling test around _report_status to assert mock_controller.ReportStatus.call_count equals 1 after the call, ensuring the status report is attempted once and not retried while preserving the existing logging assertions.Source: Coding guidelines
575-610: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover
DEADLINE_EXCEEDEDas well.Parameterize or duplicate this scenario for both
UNAVAILABLEandDEADLINE_EXCEEDED; currently the retry classification and backoff behavior for the second transient code are untested.As per coding guidelines, Python tests should provide comprehensive package coverage; the PR objective explicitly requires retries for both transient gRPC codes.
🤖 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 575 - 610, Extend test_report_status_retries_on_transient_failure to exercise both grpc.StatusCode.UNAVAILABLE and grpc.StatusCode.DEADLINE_EXCEEDED, preferably by parameterizing the test. Preserve the existing fail-twice-then-succeed assertions, including three calls and backoff values [1.0, 2.0], for each transient status code.Source: Coding guidelines
644-686: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent lease-release fallback for terminal errors.
This test covers only
UNAVAILABLE. In the supplied_request_lease_releaseimplementation, every non-UNIMPLEMENTEDgRPC error and every non-gRPC exception setsshould_retry, soPERMISSION_DENIEDandConnectionErrorstill trigger the Phase 2 status call. Add fail-fast cases for both and restrict fallback toUNAVAILABLE/DEADLINE_EXCEEDEDonly.As per coding guidelines, Python tests should provide comprehensive package coverage; the PR objective states that other gRPC errors and non-gRPC exceptions are not retried.
🤖 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 644 - 686, Extend the _request_lease_release tests with fail-fast cases for gRPC PERMISSION_DENIED and non-gRPC ConnectionError, asserting Phase 2 _report_status is not called. Update _request_lease_release so fallback retry occurs only for UNAVAILABLE and DEADLINE_EXCEEDED; propagate or handle all other gRPC statuses and non-gRPC exceptions without issuing a status-only request.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.
Outside diff comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 519-533: Update the UNIMPLEMENTED handling test around
_report_status to assert mock_controller.ReportStatus.call_count equals 1 after
the call, ensuring the status report is attempted once and not retried while
preserving the existing logging assertions.
- Around line 575-610: Extend test_report_status_retries_on_transient_failure to
exercise both grpc.StatusCode.UNAVAILABLE and grpc.StatusCode.DEADLINE_EXCEEDED,
preferably by parameterizing the test. Preserve the existing
fail-twice-then-succeed assertions, including three calls and backoff values
[1.0, 2.0], for each transient status code.
- Around line 644-686: Extend the _request_lease_release tests with fail-fast
cases for gRPC PERMISSION_DENIED and non-gRPC ConnectionError, asserting Phase 2
_report_status is not called. Update _request_lease_release so fallback retry
occurs only for UNAVAILABLE and DEADLINE_EXCEEDED; propagate or handle all other
gRPC statuses and non-gRPC exceptions without issuing a status-only request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a92aadf3-e336-4fa1-885a-5cdddd40e53a
📒 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
Add assertion to verify UNIMPLEMENTED fails fast without retry, addressing CodeRabbit review finding. Also document the conservative retry approach for non-transient errors (retries all errors except UNIMPLEMENTED) with a note about the alternative stricter approach. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace the two-phase ReportStatus(release_lease=true) approach with: - Primary path: ReleaseLease RPC (semantically correct, retry-safe) - Fallback: ReportStatus with release_lease=true for old controllers Changes: - Add _send_release_lease_rpc() with enum return type for clarity - Detect auth rejection (PERMISSION_DENIED/UNAUTHENTICATED/UNIMPLEMENTED) and cache in _release_lease_unsupported to skip on subsequent leases - Fallback uses non-AVAILABLE status to prevent race condition where retry could release a newly-assigned lease (controller identifies lease by exporter.Status.LeaseRef, not request) - Mark fallback as deprecated with TODO for removal - Update all tests for new flow Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Extend ReleaseLease to support both client and exporter authentication. Try authenticateExporter first, then authenticateClient, allowing both entity types to release their leases via the same RPC. Add releaseLeaseAsExporter() method with ExporterRef ownership check and idempotency guards (already-ended/already-releasing). Add same idempotency guards to client path. Include 4 integration tests for exporter-auth scenarios. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (1)
647-973: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared mock/stub helper for the seven
_request_lease_releasetests.Each of
test_request_lease_release_succeeds_on_first_attempt,test_request_lease_release_retries_on_transient_failure,test_request_lease_release_exhausts_retries,test_request_lease_release_falls_back_on_unsupported,test_request_lease_release_skips_release_lease_when_cached_unsupported,test_request_lease_release_preserves_failure_status_in_fallback, andtest_request_lease_release_unimplementedrepeats the same ~10-linemock_controller = AsyncMock()/stub_ctx.__aenter__/__aexit__boilerplate. The existing_setup_mock_controller_stubhelper (used for theReportStatus-only tests above) doesn't cover the dualReleaseLease+ReportStatusside-effect case these tests need. Extracting a small helper (e.g._setup_mock_controller_stub(release_lease_effect=None, report_status_effect=None)) would cut this duplication and make future lease-release tests cheaper to add.Test logic/assertions themselves are correct and consistent with the
_send_release_lease_rpc/_request_lease_releaseretry, fallback, and caching contract.🤖 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 647 - 973, Extract the repeated AsyncMock controller and async stub-context setup from the seven _request_lease_release tests into a shared helper, extending or adding _setup_mock_controller_stub to accept separate release_lease_effect and report_status_effect arguments. Update each test to use the helper while preserving its existing side effects, assertions, and ReleaseLease/ReportStatus behavior.
🤖 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/service/controller_service.go`:
- Around line 1049-1070: Update releaseLeaseAsExporter to return explicit gRPC
status errors for each failure path: map lease retrieval and patch failures to
an appropriate retryable/transient status while preserving the underlying error,
and map the exporter ownership denial to codes.PermissionDenied with the
existing message. Keep successful and idempotent release behavior unchanged so
_send_release_lease_rpc can classify these cases correctly.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 647-973: Extract the repeated AsyncMock controller and async
stub-context setup from the seven _request_lease_release tests into a shared
helper, extending or adding _setup_mock_controller_stub to accept separate
release_lease_effect and report_status_effect arguments. Update each test to use
the helper while preserving its existing side effects, assertions, and
ReleaseLease/ReportStatus behavior.
🪄 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: e7d74f45-9766-4dc2-8b69-d5630b218d13
📒 Files selected for processing (4)
controller/internal/service/controller_service.gocontroller/internal/service/controller_service_integration_test.gopython/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
| req *pb.ReleaseLeaseRequest, | ||
| ) (*pb.ReleaseLeaseResponse, error) { | ||
| // Try exporter auth first, then client auth | ||
| if exporter, err := s.authenticateExporter(ctx); err == nil { |
There was a problem hiding this comment.
hmm this might spam the logs with errors in the common flow where the client does the releasing?
There was a problem hiding this comment.
a check like this might work, not super pretty because it returns []string:
func (s *Auth) IsExporter(ctx context.Context) bool {
md, ok := metadata.FromIncomingContext(ctx)
if !ok { return false }
kinds := md.Get("jumpstarter-kind")
return len(kinds) == 1 && kinds[0] == "Exporter"
}There was a problem hiding this comment.
good idea. I actually already asked claude about that one before, and it convinced me that no error is expected 🤦
anyway, implemented in 834fde2
Address code review feedback on PR jumpstarter-dev#911: 1. Route ReleaseLease calls based on jumpstarter-kind metadata instead of try-exporter-then-client to eliminate spurious "exporter authentication failed" log entries when clients release leases. Add Auth.IsExporter() helper and comprehensive tests. 2. Return semantic gRPC status codes from releaseLeaseAsExporter instead of fmt.Errorf (which becomes codes.Unknown): - NotFound when lease doesn't exist - FailedPrecondition when exporter doesn't own the lease - Internal for k8s API errors These codes improve observability (can distinguish ownership failures from API errors in metrics) while remaining safe: none trigger the Python exporter's UNSUPPORTED fallback (PERMISSION_DENIED, INVALID_ARGUMENT, UNAUTHENTICATED, UNIMPLEMENTED), so all map to FAILURE as intended. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
3 participants
Summary
Fixes #890 by adding retry logic with exponential backoff to handle transient gRPC failures during status reporting, ensuring exporters recover from network blips instead of becoming permanently stuck.
Problem
When the afterLease hook completes, the exporter calls
_report_status(AVAILABLE)to tell the controller it's ready for new leases. If that gRPC call fails transiently (network blip, controller momentarily unreachable), the error is silently swallowed. The controller permanently seesAFTER_LEASE_HOOK, and the exporter is never assigned new leases — even though it locally believes it'sAVAILABLE. Only a pod restart recovers.Solution
Retry mechanism (
_send_report_status_rpc)Extracts the
ReportStatusgRPC call into a new private method with bounded retry:UNAVAILABLE,DEADLINE_EXCEEDEDPERMISSION_DENIED,INVALID_ARGUMENT, and other non-transient codes (logs error, returns immediately)UNIMPLEMENTEDlogs warning, returns immediately (controllers before Nov 2025)Two-phase lease release (race condition fix)
To avoid retrying
release_lease=true(which could release a newly-assigned lease if the controller processed the request but response was lost),_request_lease_releaseuses a two-phase approach:release_lease=trueonce (non-idempotent, not safe to retry)UNIMPLEMENTED, call_report_status(AVAILABLE)which invokes the retry mechanism aboveTotal attempts by error type:
Phase 2 is always triggered for failures except UNIMPLEMENTED (conservative approach), giving non-transient errors one more chance with a fresh gRPC stub, but only transient errors enter the full retry loop.
Testing
Test plan