Preserve lifecycle lease when node stop fails - #19
Conversation
WalkthroughThe change centralizes lifecycle heartbeat cleanup and updates REST and gRPC stop operations to propagate failures. New tests verify lease preservation, competing-start rejection, heartbeat handling, request parameters, and client cleanup. ChangesStop failure lifecycle handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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
🤖 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 `@PasarGuardNodeBridge/controller.py`:
- Around line 359-366: Update _stop_lifecycle_heartbeat so cleanup catches
non-cancellation exceptions raised by the completed heartbeat task, logs them,
and does not re-raise them. Continue suppressing asyncio.CancelledError,
ensuring the original REST or gRPC stop error remains the propagated failure.
🪄 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: 0cff039c-2c33-44ef-b03b-97c1ac577e6b
📒 Files selected for processing (4)
PasarGuardNodeBridge/controller.pyPasarGuardNodeBridge/grpclib.pyPasarGuardNodeBridge/rest.pytests/test_stop_lifecycle.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_stop_lifecycle.py`:
- Around line 73-86: Update test_failed_heartbeat_does_not_mask_stop_error to
configure the stop request used by node.stop() to fail with a NodeAPIError,
replace the pre-failed heartbeat Future with a pending task that raises when
cancelled, and invoke await node.stop() rather than _stop_lifecycle_heartbeat()
directly. Assert that node.stop() raises the original stop NodeAPIError and
retain the node.logger.exception assertion for the heartbeat failure.
🪄 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: 8dacff39-3a67-48de-9a63-e1856210caa8
📒 Files selected for processing (4)
PasarGuardNodeBridge/controller.pyPasarGuardNodeBridge/grpclib.pyPasarGuardNodeBridge/rest.pytests/test_stop_lifecycle.py
🚧 Files skipped from review as they are similar to previous changes (3)
- PasarGuardNodeBridge/controller.py
- PasarGuardNodeBridge/grpclib.py
- PasarGuardNodeBridge/rest.py
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 (1)
tests/test_stop_lifecycle.py (1)
49-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce the REST request signature in this test.
At Line 50,
AsyncMockaccepts any keyword set. The test can pass with the configuredNodeAPIErroreven whenRestNode.stop()makes an invalid call. The suppliedPasarGuardNodeBridge/rest.pysignature requiresproto_response_class, butRestNode.stop()omits it. Use an autospecced mock or a mocked transport behind the real_make_requestmethod, then make the production call and assertion match the required parameter.🤖 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 `@tests/test_stop_lifecycle.py` around lines 49 - 54, Update the _assert_failed_stop_keeps_lease test setup to enforce the real _make_request signature, using an autospecced mock or mocked transport instead of an unrestricted AsyncMock. Ensure RestNode.stop() supplies the required proto_response_class argument and update the awaited-call assertion to include that parameter while preserving the failed-stop lease behavior.
🧹 Nitpick comments (2)
tests/test_stop_lifecycle.py (2)
67-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the gRPC request payload.
At Lines 67-70, the test does not inspect
request["request"]. Assert that it is aservice.Emptyinstance so an incorrect or missing protobuf request cannot pass this failure-path test.🤖 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 `@tests/test_stop_lifecycle.py` around lines 67 - 70, Update the test around node._handle_grpc_request in the stop lifecycle failure-path case to assert that request["request"] is an instance of service.Empty. Keep the existing method and timeout assertions unchanged, ensuring the gRPC call includes the expected empty protobuf payload.
35-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify lease expiration after a failed stop.
At Lines 35-42, the test checks only immediate rejection of a competing
START. It does not verify that the lease expires and becomes available. A lease that remains stuck forever would pass this test. Use a short deterministic lease or an injectable clock, then assert rejection before expiry and successful acquisition after expiry.🤖 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 `@tests/test_stop_lifecycle.py` around lines 35 - 42, Extend the test around node._lifecycle_coordinator.try_acquire to use a short deterministic lease, asserting the competing START is rejected before expiration and can be acquired after expiration. Keep the existing _lifecycle_heartbeat_tasks assertion and use the coordinator’s established timing or clock-injection mechanism rather than relying on an unbounded wait.
🤖 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 `@tests/test_stop_lifecycle.py`:
- Around line 49-54: Update the _assert_failed_stop_keeps_lease test setup to
enforce the real _make_request signature, using an autospecced mock or mocked
transport instead of an unrestricted AsyncMock. Ensure RestNode.stop() supplies
the required proto_response_class argument and update the awaited-call assertion
to include that parameter while preserving the failed-stop lease behavior.
---
Nitpick comments:
In `@tests/test_stop_lifecycle.py`:
- Around line 67-70: Update the test around node._handle_grpc_request in the
stop lifecycle failure-path case to assert that request["request"] is an
instance of service.Empty. Keep the existing method and timeout assertions
unchanged, ensuring the gRPC call includes the expected empty protobuf payload.
- Around line 35-42: Extend the test around
node._lifecycle_coordinator.try_acquire to use a short deterministic lease,
asserting the competing START is rejected before expiration and can be acquired
after expiration. Keep the existing _lifecycle_heartbeat_tasks assertion and use
the coordinator’s established timing or clock-injection mechanism rather than
relying on an unbounded wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57555c30-d140-47ca-b3b5-1b3b1161890a
📒 Files selected for processing (1)
tests/test_stop_lifecycle.py
1 participant
Summary
Validation
.venv\\Scripts\\python.exe -m unittest discover -s tests -v— 14 passedgit diff --checkThe repository-wide Ruff-format check reports pre-existing README alignment outside this PR; the changed Python/test files are formatted.