Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesNode sync lifecycle
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant NodeOperation
participant NodeManager
participant UserSyncTask
NodeOperation->>NodeManager: start user sync
NodeManager->>UserSyncTask: track sync task
NodeManager->>UserSyncTask: cancel on connection change or removal
UserSyncTask-->>NodeManager: lifecycle cancellation
NodeManager-->>NodeOperation: raise UserSyncPreemptedError
NodeOperation-->>NodeOperation: return HTTP 409 without node status update
Suggested reviewers: Merge Risk: 🔵 Low · up to A sync started during a node change or removal can briefly defeat preemption. The PR is mergeable with awareness of this bounded concurrency risk. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 12.12% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: allowing node lifecycle operations to preempt in-flight user synchronization. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
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.
A rabbit checks the syncs in flight,
Node changes pause the batches’ light.
Locks remain while tasks unwind,
New nodes serve the work assigned.
A retry waits beyond the bend,
Then users sync from start to end.
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…on-sync-lock # Conflicts: # app/node/__init__.py
3aef26a to
47ddab7
Compare
Cancel in-flight user syncs from update_node only when the connection
signature changed (and from remove_node as before). A name or usage
coefficient edit now waits for the per-node lock instead: bulk deliveries
release it between batches and continue on the replacement object, so
pending user deltas are not dropped. Once metadata-only edits stop
restarting the node, nothing else would re-send cancelled deltas.
Run sync_full in its own tracked task. Preemption then cancels only the
sync, never the caller (in single-process mode that is the admin's
POST /node/{id}/sync request), and the caller gets UserSyncPreemptedError,
which the node operation maps to a 409 without marking the node as
errored. Cancelling the caller still cancels the sync.
Log deliveries cancelled by a lifecycle change at debug level in
_update_users instead of dropping the CancelledError results silently.
- removal cancels a stuck per-user fallback delivery - an unchanged health-check reconnect keeps a running full or bulk sync, while a connection change cancels it - a name or usage coefficient edit waits for the batch in flight and the remaining batches reach the replacement object - a metadata edit waits for a running full sync - preempting sync_full raises UserSyncPreemptedError in the caller and cancelling the caller cancels the sync - the sync endpoint maps preemption to 409 without an error status
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Node edits, disables, and removals could wait for many minutes when a bulk or full user sync held the per-node lock. A failed chunked sync made this especially visible because the bridge fallback retried users individually while lifecycle requests remained queued.
The PR changes only
app/node/__init__.py; it adds no documentation, test files, migrations, dependencies, or unrelated changes.Type of change
Checklist
Testing
python -m pytest tests/test_node_manager.py tests/test_node_manager_sync.py -q— 11 passed.python -m ruff check --no-fix app/node/__init__.py— passed.python -m ruff format --check app/node/__init__.py— passed.git diff --check— passed.Notes for reviewers
Cancellation is limited to user-sync tasks for the affected node and only happens for an actual configuration change or lifecycle removal. Automated reconnects with an unchanged connection signature and metadata continue to reuse the live node without disrupting synchronization.
Summary by CodeRabbit