fix(nodes): prevent cross-worker lifecycle races

[dr-hoseyn]

Summary

• let sibling workers replace their local node controller without stopping the remote node
• treat lifecycle lease contention as a no-op after attach fails, so a transient 409 cannot overwrite the lease owner's result
• add regression coverage for sibling upsert/connect handling, local-only controller replacement, and lifecycle contenders

Root cause

In multi-worker deployments, a worker receiving an upsert or connect synchronization event replaced its local controller and remotely stopped the node. That raced the origin worker's successful start. Concurrent contenders then persisted Node lifecycle operation already in progress over the owner's eventual result.

Verification

• ruff check .
• pytest tests/test_node_manager_sync.py tests/test_node_manager.py tests/test_connect_concurrency.py -q (13 passed)
• pytest --ignore=tests/api --ignore=tests/test_review_admins_unit.py -q (156 passed, 2 skipped)
• two-worker production canary: the node reached connected, lifecycle settled at healthy with no active lease, and no node-409 entries recurred after deployment

Production observation

About 23 minutes after the successful canary, the remote node VPS itself became unreachable on SSH and both node ports from the panel and an independent network path. The panel and the other nodes remained healthy, and the fixed lifecycle 409 did not recur. The later Request timed out state is therefore tracked as a separate infrastructure outage rather than evidence of the cross-worker race returning.

Test-suite note

The repository contribution guide names next, but that branch is not present on the upstream remote, so this PR targets dev. The full API suite is currently blocked by its local test database/auth seeding setup; the affected node lifecycle unit and integration-adjacent tests pass.

Summary by CodeRabbit

• Bug Fixes

  • Improved node synchronization across workers without triggering unintended remote shutdowns.
  • Prevented concurrent connection attempts from overwriting the result produced by the worker that owns the operation.
  • Improved replacement of locally managed node controllers while preserving their connection state.

• Tests

  • Added regression coverage for concurrent connections, controller replacement, and synchronized node updates.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 883c32db-9635-483f-b66e-2243f78bb8be

📥 Commits

Reviewing files that changed from the base of the PR and between 3331421 and 8852da7.

📒 Files selected for processing (6)

• app/node/__init__.py
• app/node/manager_sync.py
• app/operation/node.py
• tests/test_connect_concurrency.py
• tests/test_node_manager.py
• tests/test_node_manager_sync.py

Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review.

---

Walkthrough

Node replacement now supports disabling remote shutdown during synchronization. Concurrent connection contenders return None after failed lease attachment, preserving the lease owner’s result. Regression tests cover both behaviors.

Changes

Node lifecycle coordination

Layer / File(s)	Summary
Remote-stop propagation and synchronization app/node/__init__.py, app/node/manager_sync.py, tests/test_node_manager.py, tests/test_node_manager_sync.py	update_node accepts remote_stop. Synchronization paths pass remote_stop=False. Tests verify local replacement and remote connect and upsert handling.
Connection lease contention app/operation/node.py, tests/test_connect_concurrency.py	A contender returns None after a 409 lease conflict and unsuccessful attachment retry. The concurrency test verifies this result.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 8852d

This change narrows cross-worker node lifecycle behavior and handles lease contention without overwriting the lease owner’s result; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant manager_sync
  participant NodeManager
  participant OldController
  manager_sync->>NodeManager: update_node(remote_stop=False)
  NodeManager->>OldController: shutdown(remote_stop=False)
  NodeManager-->>manager_sync: replacement controller

Loading

sequenceDiagram
  participant connect_node
  participant NodeStartup
  participant AttachRetry
  connect_node->>NodeStartup: start node
  NodeStartup-->>connect_node: NodeAPIError(409, "already in progress")
  connect_node->>AttachRetry: attempt attachment
  AttachRetry-->>connect_node: unsuccessful
  connect_node-->>connect_node: return None

Loading

Possibly related PRs

• PasarGuard/panel#746: Updates the same node synchronization and connection coordination paths.

Suggested labels: Backend

Suggested reviewers: m03ed, x0sina

Poem

I’m a rabbit guarding nodes tonight,
No remote stop when sync feels right.
A contender hops aside with care,
While the lease owner keeps the share.
None marks the path as clear.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: preventing cross-worker node lifecycle races.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[dr-hoseyn]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.