Harden bridge transport and compatibility - #18
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
WalkthroughThe change adds factory compatibility options, bounded and revocation-aware user-sync storage, hardened HTTP and gRPC lifecycle handling, sanitized logging, and controller worker recovery. Tests cover storage limits, leases, revocation, redirects, transport failures, cancellation, and reconnect behavior. ChangesNode bridge synchronization
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant UserSyncStore
participant NodeTransport
participant RevocationClient
RevocationClient->>Controller: begin_user_revocation
Controller->>UserSyncStore: begin revocation and fence users
Controller->>UserSyncStore: acquire user-sync lease
Controller->>NodeTransport: synchronize users with revocation_id
NodeTransport-->>Controller: return completion or failures
Controller->>UserSyncStore: acknowledge or requeue claims
RevocationClient->>Controller: finalize_user_revocation
Controller->>UserSyncStore: finalize revocation
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)
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: 6
🧹 Nitpick comments (4)
tests/test_security_hardening.py (3)
337-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the
disconnect()await so a regression fails instead of hanging.
await first.disconnect()has no time limit. This test depends ondisconnect()cancelling the running_sync_workertask. If that cancellation regresses, the test blocks until the suite-level timeout rather than reporting a failure.🧪 Proposed change
- await first.disconnect() + await asyncio.wait_for(first.disconnect(), timeout=1.0) claimed = await second._claim_pending_users()🤖 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_security_hardening.py` around lines 337 - 340, Bound the await of first.disconnect() in the test around _claim_pending_users so cancellation regressions fail promptly instead of hanging; use the test suite’s existing timeout utility or convention and preserve the subsequent claimed-user assertions.
158-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the hand-built worker fixture.
This test assigns about eighteen attributes to a
GrpcNodecreated with__new__.SharedStoreDisconnectTests._controllerperforms a similar setup. When_sync_workerstarts reading a new attribute, these tests fail withAttributeErrorinstead of a meaningful assertion, and each fixture must be updated separately.Extract a shared module-level builder that both test classes call.
🤖 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_security_hardening.py` around lines 158 - 183, Extract the repeated hand-built GrpcNode setup from test_stream_open_timeout_increments_worker_failure_and_requeues and SharedStoreDisconnectTests._controller into a shared module-level builder. Have both tests call the builder, while preserving their scenario-specific overrides and mocks, so newly required _sync_worker attributes are initialized in one place.
261-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this test out of
LoggingSafetyTests.
test_connect_restarts_worker_to_discover_stored_pending_workverifies worker restart behavior onconnect. It does not verify logging safety. Place it in a class that describes worker lifecycle so the suite stays navigable.🤖 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_security_hardening.py` around lines 261 - 281, Move test_connect_restarts_worker_to_discover_stored_pending_work out of LoggingSafetyTests and into the existing test class covering worker lifecycle or connect behavior. Keep the test setup, assertions, and mocking unchanged; only relocate it to the semantically appropriate class.tests/test_storage.py (1)
73-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the claimed-user accounting and the constructor validation.
The new test covers pending-only accounting. Two behaviors added in this PR remain untested:
enqueue_userscounts claimed users toward the bound, and the constructor rejects a non-positivemax_pending_users_per_node. Both are cheap to add.🧪 Suggested additional tests
async def test_claimed_users_count_toward_bound(self): store = InMemoryUserSyncStore(max_pending_users_per_node=1) await store.enqueue_users("node-1", [User(email="a@example.com")]) await store.claim_users("node-1", "worker-1", limit=10, lease_seconds=30) with self.assertRaises(UserSyncStoreFullError): await store.enqueue_users("node-1", [User(email="b@example.com")]) def test_non_positive_bound_is_rejected(self): with self.assertRaises(ValueError): InMemoryUserSyncStore(max_pending_users_per_node=0)🤖 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_storage.py` around lines 73 - 82, Add tests covering the remaining constructor and accounting behavior in the storage test suite: add an async test that claims the node’s only user and verifies enqueue_users rejects another user because claimed users count toward max_pending_users_per_node, and add a constructor test verifying InMemoryUserSyncStore rejects a zero or otherwise non-positive bound with ValueError.
🤖 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/aiohttp_compat.py`:
- Around line 31-33: Update Node.stream_logs in rest.py to invoke
raise_for_status for 3xx responses as well as 4xx/5xx responses, aligning its
pre-stream status check with BufferedStatusError’s 300-and-above policy.
Preserve normal streaming for 2xx responses so redirects produce NodeAPIError
instead of an empty log queue.
In `@PasarGuardNodeBridge/controller.py`:
- Around line 41-62: Update _sanitize_log_text to escape Unicode line separators
U+2028 and U+2029 in addition to the existing control characters. Modify
_SanitizingLoggerAdapter so the final rendered message is sanitized after
positional argument interpolation, preserving exception formatting and
truncation behavior; add tests covering positional arguments containing CR/LF
and U+2028.
- Around line 646-648: Extend the outer failure handling around the worker flow
to requeue any remaining claimed_users for all non-cancellation exceptions,
including failures from _ack_claimed_users(), _requeue_claimed_users(),
sync_users_chunked(), _sync_batch_users(), and _claim_pending_users(). Ensure
partial acknowledgment or requeue failures trigger explicit retry/requeue
handling so every still-claimed user is recovered before the worker exits.
In `@PasarGuardNodeBridge/grpclib.py`:
- Around line 435-446: Update the user-send loop in the SyncUser stream flow to
stop iterating after the first send_message failure. Keep the failed user in
failed, mark all remaining users as failed without retrying send_message, and
preserve the existing warning for the initial stream error.
- Around line 142-153: Update _open_grpc_stream so cancellation or timeout
during method.open’s context entry still closes the partially established gRPC
stream; do not rely solely on AsyncExitStack.enter_async_context registering
__aexit__ after __aenter__ completes. Explicitly retain and clean up the
stream/context manager using the appropriate grpclib lifecycle methods, while
preserving the bounded establishment and cleanup timeouts.
In `@PasarGuardNodeBridge/storage.py`:
- Around line 141-144: The new store-capacity failure must have a consistent
public error contract. Update Controller.update_user and update_users to catch
the relevant initialization/enqueue exceptions and convert them to NodeAPIError,
or explicitly document that these methods propagate the RuntimeError subclasses;
preserve the chosen behavior consistently for both methods.
---
Nitpick comments:
In `@tests/test_security_hardening.py`:
- Around line 337-340: Bound the await of first.disconnect() in the test around
_claim_pending_users so cancellation regressions fail promptly instead of
hanging; use the test suite’s existing timeout utility or convention and
preserve the subsequent claimed-user assertions.
- Around line 158-183: Extract the repeated hand-built GrpcNode setup from
test_stream_open_timeout_increments_worker_failure_and_requeues and
SharedStoreDisconnectTests._controller into a shared module-level builder. Have
both tests call the builder, while preserving their scenario-specific overrides
and mocks, so newly required _sync_worker attributes are initialized in one
place.
- Around line 261-281: Move
test_connect_restarts_worker_to_discover_stored_pending_work out of
LoggingSafetyTests and into the existing test class covering worker lifecycle or
connect behavior. Keep the test setup, assertions, and mocking unchanged; only
relocate it to the semantically appropriate class.
In `@tests/test_storage.py`:
- Around line 73-82: Add tests covering the remaining constructor and accounting
behavior in the storage test suite: add an async test that claims the node’s
only user and verifies enqueue_users rejects another user because claimed users
count toward max_pending_users_per_node, and add a constructor test verifying
InMemoryUserSyncStore rejects a zero or otherwise non-positive bound with
ValueError.
🪄 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: 42487186-7352-4846-af7d-600c610fb7fb
📒 Files selected for processing (10)
PasarGuardNodeBridge/__init__.pyPasarGuardNodeBridge/aiohttp_compat.pyPasarGuardNodeBridge/controller.pyPasarGuardNodeBridge/grpclib.pyPasarGuardNodeBridge/rest.pyPasarGuardNodeBridge/storage.pyREADME.mdtests/test_constructor_compatibility.pytests/test_security_hardening.pytests/test_storage.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_security_hardening.py`:
- Around line 443-467: Update test_outer_worker_failure_retries_failed_requeue
so the worker terminates after the recovery retry: either set the controller
shutdown event during the mocked backoff or run _sync_worker as a cancellable
task and cancel it before awaiting completion. Preserve the existing assertions
that the requeue is attempted twice and the user is recoverable.
🪄 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: bcc47fa7-c057-49c3-8d57-ae61339f43ec
📒 Files selected for processing (6)
PasarGuardNodeBridge/controller.pyPasarGuardNodeBridge/grpclib.pyPasarGuardNodeBridge/rest.pyREADME.mdtests/test_security_hardening.pytests/test_storage.py
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- PasarGuardNodeBridge/rest.py
- PasarGuardNodeBridge/grpclib.py
- PasarGuardNodeBridge/controller.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
PasarGuardNodeBridge/controller.py (1)
748-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
!sconversion flag for the exception text.Ruff reports RUF010 on Line 751. The surrounding code already uses
{e!s}in other log statements, for example in_cleanup_sync_workerat Line 532.♻️ Proposed fix
- f"[{self.name}] Unexpected error in sync worker | Error: {error_type} - {str(e)}", exc_info=True + f"[{self.name}] Unexpected error in sync worker | Error: {error_type} - {e!s}", exc_info=True🤖 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 `@PasarGuardNodeBridge/controller.py` around lines 748 - 753, Update the unexpected-error log in the sync worker’s exception handler to use the `!s` conversion flag when formatting the exception text, while preserving the existing error type, message context, and `exc_info=True` behavior.Source: Linters/SAST tools
tests/test_security_hardening.py (1)
526-574: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce timing sensitivity in the lease-expiry test.
The test depends on wall-clock margins that are small.
first._sync_lease_secondsis 0.08, and Line 561 sleeps 0.02 before asserting that the second worker has not processed the claim. On a loaded CI runner, the second worker can claim the expired lease before that assertion runs, which makes the test flaky.Increase the lease duration and the observation window so the margin between "lease still held" and "lease expired" is larger.
♻️ Proposed timing adjustment
- first._sync_lease_seconds = 0.08 + first._sync_lease_seconds = 0.5 ... - await asyncio.sleep(0.02) + await asyncio.sleep(0.1) self.assertFalse(second_processed.is_set()) self.assertFalse(second_worker.done()) - await asyncio.wait_for(second_processed.wait(), timeout=0.5) + await asyncio.wait_for(second_processed.wait(), timeout=2.0)🤖 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_security_hardening.py` around lines 526 - 574, Adjust the timing constants in test_second_worker_wakes_after_failed_requeue_lease_expires to increase the lease duration and lengthen the pre-expiry observation delay, preserving the assertion that the second worker has not processed the claim before expiration and the existing post-expiry wait behavior.tests/test_storage.py (1)
119-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the negative boundary too.
The test name says “non-positive,” but it only checks
0. Add-1so regressions in the validation condition are detected.Suggested test adjustment
def test_non_positive_per_node_bound_is_rejected(self): - with self.assertRaises(ValueError): - InMemoryUserSyncStore(max_pending_users_per_node=0) + for limit in (0, -1): + with self.subTest(limit=limit): + with self.assertRaises(ValueError): + InMemoryUserSyncStore(max_pending_users_per_node=limit)🤖 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_storage.py` around lines 119 - 122, Update test_non_positive_per_node_bound_is_rejected to also construct InMemoryUserSyncStore with max_pending_users_per_node=-1 inside the ValueError assertion, covering both zero and negative non-positive bounds.
🤖 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 565-573: Update _wait_for_claim_recheck so wait_delay is bounded
below by _sync_poll_interval, while still preventing negative delays. Keep the
existing event wait, timeout handling, and wake-up behavior unchanged.
In `@tests/test_storage.py`:
- Around line 87-99: Update
test_next_claim_delay_distinguishes_empty_pending_and_leased_work to avoid
timing flakiness by using a substantially longer lease_seconds value, or by
controlling time.monotonic() deterministically; preserve the assertions that
empty work returns None, pending work returns 0.0, and leased work returns a
positive delay no greater than the lease.
---
Nitpick comments:
In `@PasarGuardNodeBridge/controller.py`:
- Around line 748-753: Update the unexpected-error log in the sync worker’s
exception handler to use the `!s` conversion flag when formatting the exception
text, while preserving the existing error type, message context, and
`exc_info=True` behavior.
In `@tests/test_security_hardening.py`:
- Around line 526-574: Adjust the timing constants in
test_second_worker_wakes_after_failed_requeue_lease_expires to increase the
lease duration and lengthen the pre-expiry observation delay, preserving the
assertion that the second worker has not processed the claim before expiration
and the existing post-expiry wait behavior.
In `@tests/test_storage.py`:
- Around line 119-122: Update test_non_positive_per_node_bound_is_rejected to
also construct InMemoryUserSyncStore with max_pending_users_per_node=-1 inside
the ValueError assertion, covering both zero and negative non-positive bounds.
🪄 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: 30391eeb-0cbb-4b2e-9ee1-58f4949609e2
📒 Files selected for processing (6)
PasarGuardNodeBridge/controller.pyPasarGuardNodeBridge/rest.pyPasarGuardNodeBridge/storage.pyREADME.mdtests/test_security_hardening.pytests/test_storage.py
🚧 Files skipped from review as they are similar to previous changes (1)
- PasarGuardNodeBridge/storage.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
PasarGuardNodeBridge/controller.py (3)
761-772: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winA very small positive delay still bypasses the backoff floor.
wait_delayapplies the floor only whendelayis not greater than zero. A store that reports a small positive deadline, for example0.0005, produces a tight claim loop with no effective pause. Apply the floor to every delay.🐛 Proposed fix
- wait_delay = delay if delay > 0 else max(self._sync_poll_interval, MIN_CLAIM_RECHECK_DELAY) + wait_delay = max(delay, MIN_CLAIM_RECHECK_DELAY) if delay > 0 else max( + self._sync_poll_interval, MIN_CLAIM_RECHECK_DELAY + )🤖 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 `@PasarGuardNodeBridge/controller.py` around lines 761 - 772, Update _wait_for_claim_recheck so wait_delay always applies the minimum backoff floor, including when delay is a small positive value; retain the configured _sync_poll_interval as the other floor input and preserve the existing event-wait behavior.
684-701: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the sync-worker cleanup above the recovery timeout.
_cleanup_sync_workeruses a 2.0 second maximum, while_recover_claimed_usersusesCLAIM_RECOVERY_TIMEOUT = 1.0. If the worker task can still execute recovery during cleanup, raise this cleanup bound enough above the fixed recovery timeout, or derive it fromCLAIM_RECOVERY_TIMEOUT, so cleanup does not time out while the worker task is still scheduled.🤖 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 `@PasarGuardNodeBridge/controller.py` around lines 684 - 701, The timeout in _cleanup_sync_worker must exceed the fixed CLAIM_RECOVERY_TIMEOUT used by _recover_claimed_users. Update the cleanup wait bound to derive from CLAIM_RECOVERY_TIMEOUT or otherwise provide sufficient margin, ensuring the worker can finish recovery before cleanup times out.
968-1016: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDon’t retain the whole execution lease for known failed users.
_sync_batch_usersreturns only the failed users, and those keys are put back infailed_claims. Then_abandon_user_sync_leasestores the lease for alluser_keys, so any concurrentbegin_user_revocation(["X","Y"], ...)must wait for or fail against the same fail-closed lease, even though onlyYhas an unknown outcome. Release or split the lease for the acknowledged/failed users and keep it only for keys whose remote outcome is genuinely unknown.🤖 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 `@PasarGuardNodeBridge/controller.py` around lines 968 - 1016, Update the partial-failure path around _sync_batch_users so _abandon_user_sync_lease does not retain the lease for every user key. Release or split the lease after deriving failed_claims, removing acknowledged and known-failed users; retain it only for keys whose remote outcome is genuinely unknown. Keep the existing acknowledgment and requeue behavior intact, and ensure the exception path still abandons the lease for genuinely unresolved outcomes.
🧹 Nitpick comments (2)
tests/test_user_revocation.py (1)
20-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the hand-built controller factory between test modules.
This
_controllerhelper builds aControllerwithobject.__new__and sets 21 private attributes by hand.tests/test_security_hardening.pydefines a near-identical helper inSharedStoreDisconnectTests._controller. The two copies already differ: this one omits_tasks,_task_lock, and_version_lock.When
Controller.__init__or_sync_workerstarts using a new attribute, both copies must be updated, and a missed update surfaces as anAttributeErrorinside the worker rather than a clear failure. Move the factory into a shared test helper module.🤖 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_user_revocation.py` around lines 20 - 43, Move the hand-built Controller factory from this test module into a shared test helper, then update both this module and SharedStoreDisconnectTests._controller to import and reuse it. Preserve the existing setup while consolidating all required private attributes, including _tasks, _task_lock, and _version_lock, so future Controller changes require updates in only one factory.tests/test_security_hardening.py (1)
437-443: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
_wait_untilspins the event loop instead of yielding time.
await asyncio.sleep(0)yields control but schedules an immediate callback. The loop therefore runs at full CPU for up totimeout.test_idle_retirement_boundary_100x_never_strands_enqueued_workcalls this helper 100 times, so the cost accumulates.Use a small positive sleep so the loop can idle between checks.
♻️ Proposed change
`@staticmethod` async def _wait_until(predicate, timeout=0.2): async def poll(): while not predicate(): - await asyncio.sleep(0) + await asyncio.sleep(0.001) await asyncio.wait_for(poll(), timeout=timeout)🤖 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_security_hardening.py` around lines 437 - 443, Update the _wait_until helper’s poll loop to await a small positive sleep interval instead of asyncio.sleep(0), allowing the event loop to idle between predicate checks while preserving the existing timeout and polling 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 `@PasarGuardNodeBridge/storage.py`:
- Around line 435-479: Update abort_user_revocation and finalize_user_revocation
to restore each affected state's closing flag and ownership/finalization fields
when _wait_for_user_sync_leases raises UserSyncLeaseLostError or cancellation,
then re-raise the exception. Add regression tests covering failed lease drains
for both methods and verify the owning revocation_id can acquire a user-sync
lease afterward.
In `@tests/test_security_hardening.py`:
- Around line 608-660: Increase the scheduling margins in
test_idle_retirement_boundary_100x_never_strands_enqueued_work and
test_zero_deadline_worker_cancels_without_hot_loop_or_task_leak so loaded CI
does not fail nondeterministically. Raise the short sleeps, per-operation
timeouts, and claim-count allowance as needed, or reduce the retirement test
iteration count while preserving its boundary and task-cleanup assertions.
---
Outside diff comments:
In `@PasarGuardNodeBridge/controller.py`:
- Around line 761-772: Update _wait_for_claim_recheck so wait_delay always
applies the minimum backoff floor, including when delay is a small positive
value; retain the configured _sync_poll_interval as the other floor input and
preserve the existing event-wait behavior.
- Around line 684-701: The timeout in _cleanup_sync_worker must exceed the fixed
CLAIM_RECOVERY_TIMEOUT used by _recover_claimed_users. Update the cleanup wait
bound to derive from CLAIM_RECOVERY_TIMEOUT or otherwise provide sufficient
margin, ensuring the worker can finish recovery before cleanup times out.
- Around line 968-1016: Update the partial-failure path around _sync_batch_users
so _abandon_user_sync_lease does not retain the lease for every user key.
Release or split the lease after deriving failed_claims, removing acknowledged
and known-failed users; retain it only for keys whose remote outcome is
genuinely unknown. Keep the existing acknowledgment and requeue behavior intact,
and ensure the exception path still abandons the lease for genuinely unresolved
outcomes.
---
Nitpick comments:
In `@tests/test_security_hardening.py`:
- Around line 437-443: Update the _wait_until helper’s poll loop to await a
small positive sleep interval instead of asyncio.sleep(0), allowing the event
loop to idle between predicate checks while preserving the existing timeout and
polling behavior.
In `@tests/test_user_revocation.py`:
- Around line 20-43: Move the hand-built Controller factory from this test
module into a shared test helper, then update both this module and
SharedStoreDisconnectTests._controller to import and reuse it. Preserve the
existing setup while consolidating all required private attributes, including
_tasks, _task_lock, and _version_lock, so future Controller changes require
updates in only one factory.
🪄 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: 7ee711d6-dd3f-4563-bdb2-85e6e2ba1cc8
📒 Files selected for processing (10)
PasarGuardNodeBridge/__init__.pyPasarGuardNodeBridge/abstract_node.pyPasarGuardNodeBridge/controller.pyPasarGuardNodeBridge/grpclib.pyPasarGuardNodeBridge/rest.pyPasarGuardNodeBridge/storage.pyREADME.mdtests/test_security_hardening.pytests/test_storage.pytests/test_user_revocation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_storage.py
| async def abort_user_revocation(self, node_id: str, user_keys: list[str], revocation_id: str) -> None: | ||
| if not revocation_id: | ||
| raise ValueError("revocation_id must not be empty") | ||
| unique_keys = self._unique_user_keys(user_keys) | ||
| async with self._lease_changed: | ||
| affected_keys = { | ||
| user_key | ||
| for user_key in unique_keys | ||
| if (state := self._revocations.get(node_id, {}).get(user_key)) is not None | ||
| and not state.finalized | ||
| and state.active_owner == revocation_id | ||
| } | ||
| for user_key in affected_keys: | ||
| self._revocation_state(node_id, user_key).closing = True | ||
| await self._wait_for_user_sync_leases(node_id, affected_keys) | ||
| for user_key in affected_keys: | ||
| state = self._revocation_state(node_id, user_key) | ||
| state.active_owner = None | ||
| state.closing = False | ||
| if affected_keys: | ||
| self._lease_changed.notify_all() | ||
|
|
||
| async def finalize_user_revocation(self, node_id: str, user_keys: list[str], revocation_id: str) -> None: | ||
| if not revocation_id: | ||
| raise ValueError("revocation_id must not be empty") | ||
| unique_keys = self._unique_user_keys(user_keys) | ||
| async with self._lease_changed: | ||
| affected_keys = { | ||
| user_key | ||
| for user_key in unique_keys | ||
| if (state := self._revocations.get(node_id, {}).get(user_key)) is not None | ||
| and not state.finalized | ||
| and state.active_owner == revocation_id | ||
| } | ||
| for user_key in affected_keys: | ||
| state = self._revocation_state(node_id, user_key) | ||
| state.closing = True | ||
| await self._wait_for_user_sync_leases(node_id, affected_keys) | ||
| for user_key in affected_keys: | ||
| state = self._revocation_state(node_id, user_key) | ||
| state.finalized = True | ||
| state.active_owner = None | ||
| state.closing = False | ||
| if affected_keys: | ||
| self._lease_changed.notify_all() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for tests that exercise abort/finalize interrupted by an expired or cancelled lease drain.
set -euo pipefail
rg -n -C 6 'abort_user_revocation|finalize_user_revocation' --glob 'tests/**/*.py'
rg -n -C 4 'closing' PasarGuardNodeBridge/storage.pyRepository: PasarGuard/node_bridge_py
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)PasarGuardNodeBridge/storage\.py$|tests/' || true
echo
echo "storage.py outline around revocation symbols:"
rg -n 'class |def .*user_revocation|def .*user_sync_lease|async def |closing|revocation_id|UserSyncLeaseLostError' PasarGuardNodeBridge/storage.py || true
echo
echo "Relevant lines 340-520:"
sed -n '340,520p' PasarGuardNodeBridge/storage.py | nl -ba -v340
echo
echo "Tests containing revoke/drain/lease-related terms:"
rg -n -C 4 'revocation|leasename|lease|drain|cancel|cancelled|closing|finalize_user_revocation|abort_user_revocation|begin_user_revocation|UserSyncLeaseLostError' --glob 'tests/**/*.py' --glob '**/*test*.py' || trueRepository: PasarGuard/node_bridge_py
Length of output: 6443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant storage.py lines 390-510:"
sed -n '390,510p' PasarGuardNodeBridge/storage.py | awk '{printf "%5d\t%s\n", NR+389, $0}'
echo
echo "user revocation tests:"
sed -n '1,320p' tests/test_user_revocation.py | awk '{printf "%5d\t%s\n", NR, $0}'
echo
echo "tests containing cancel/drain/closing/lease lost:"
rg -n -C 5 'cancel|cancelled|closing|drain|lease_lost|UserSyncLeaseLostError|abort_user_revocation|finalize_user_revocation|begin_user_revocation|acquire_user_sync_lease|release_user_sync_lease' tests || trueRepository: PasarGuard/node_bridge_py
Length of output: 46396
Add tests for failed abort/finalize lease drains.
abort_user_revocation and finalize_user_revocation set state.closing = True before _wait_for_user_sync_leases. If that call raises UserSyncLeaseLostError or the task is cancelled, closing stays True, active_owner is not cleared, and later acquire_user_sync_lease denies the owning revocation_id. The current revocation tests cover cancelled/expired begin_user_revocation, but not the same path for abort_user_revocation/finalize_user_revocation.
🤖 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 `@PasarGuardNodeBridge/storage.py` around lines 435 - 479, Update
abort_user_revocation and finalize_user_revocation to restore each affected
state's closing flag and ownership/finalization fields when
_wait_for_user_sync_leases raises UserSyncLeaseLostError or cancellation, then
re-raise the exception. Add regression tests covering failed lease drains for
both methods and verify the owning revocation_id can acquire a user-sync lease
afterward.
1 participant
Summary
api_port,max_message_size, andController.extra.Validation
uv run python -m unittest discover -s tests -v(30 passed)uv run python -m compileall -q PasarGuardNodeBridge testsuv buildgit diff --checkRisk / rollout notes
Summary by CodeRabbit
New Features
Bug Fixes
Security
Documentation