fix: AdminForceDeleteMachine can deadlock#3364
Conversation
Summary by CodeRabbit
WalkthroughThe changes centralize advisory locking for network segments, apply admin-segment locks before interface and reconciliation operations, reorder force-delete explored-data cleanup, and add concurrency coverage for exploration and deletion lock ordering. ChangesLock ordering and transaction cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ForceDelete
participant Exploration
participant PostgreSQL
ForceDelete->>PostgreSQL: Lock admin segments
ForceDelete->>PostgreSQL: Delete explored rows
Exploration->>PostgreSQL: Lock explored_managed_hosts
ForceDelete-->>Exploration: Wait for explored-row lock
Exploration->>PostgreSQL: Update machine_interfaces
Exploration->>PostgreSQL: Commit
PostgreSQL-->>ForceDelete: Complete deletion
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ﾉ ✅ Action performedFull review finished. |
chet
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/api-core/src/handlers/machine.rs (1)
640-640: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit cleanup identifiers as structured tracing fields.
These interpolated values will not be independently searchable as logfmt attributes.
Proposed structured logs
- tracing::info!("Cleaning up explored endpoint at {addr} {}", machine.id); + tracing::info!(%addr, machine_id = %machine.id, "Cleaning up explored endpoint"); ... - tracing::info!("Cleaning up explored endpoint at {addr} {}", dpu_machine.id); + tracing::info!(%addr, machine_id = %dpu_machine.id, "Cleaning up explored endpoint");As per coding guidelines, common log data must be passed as tracing attributes rather than string interpolation.
Also applies to: 652-652
🤖 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 `@crates/api-core/src/handlers/machine.rs` at line 640, Update the cleanup logs in the explored-endpoint cleanup handler, including the messages around the referenced `tracing::info!` calls, to emit `addr` and `machine.id` as structured tracing fields rather than interpolating them into the message; keep the message static and pass both identifiers as named tracing attributes.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 `@crates/api-core/src/handlers/machine_discovery.rs`:
- Around line 292-296: Move the lock_all_admin_segments call earlier in the
transaction, before associate_interface_with_dpu_machine performs its
interface-row write. Ensure every execution path that reaches that write
acquires the segment advisory locks first, and remove the later redundant lock
call while preserving allocator lock ordering through reconciliation.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 1895-1900: Stabilize the Admin segment set before reconciliation
by serializing Admin-segment create, update, and delete operations with the same
catalog-level advisory lock used by load_and_lock_all_admin_segments, or
otherwise use a transaction-wide stable snapshot. Update the relevant Admin
segment mutation paths and load_and_lock_all_admin_segments so later reconcile
reads cannot discover newly committed segments after interface row locks are
acquired.
---
Nitpick comments:
In `@crates/api-core/src/handlers/machine.rs`:
- Line 640: Update the cleanup logs in the explored-endpoint cleanup handler,
including the messages around the referenced `tracing::info!` calls, to emit
`addr` and `machine.id` as structured tracing fields rather than interpolating
them into the message; keep the message static and pass both identifiers as
named tracing attributes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 262ca78a-0652-40e3-9696-f2735b3142c2
📒 Files selected for processing (7)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-db/src/machine_interface.rscrates/site-explorer/src/machine_creator.rs
chet
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/api-core/src/handlers/machine_discovery.rs (1)
291-297: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftSegment lock is acquired after an earlier interface-row write, re-opening the deadlock window.
The comment asserts the lock precedes "the interface-row writes below", but when
machine_discovery_info.create_machineis true,associate_interface_with_dpu_machineat Lines 202-207 already writes (and row-locks) the interface inside this same transaction — well before Line 296. A concurrent reconcile/allocator transaction holds the admin-segment advisory locks first and then contends for interface rows; this path holds an interface row and then waits on the segment locks, which is the exact inverted ordering the PR set out to eliminate.Acquire the admin-segment locks once, immediately after
txn_begin()(Line 118), so every interface write in the DPU path is ordered behind them.🔒 Proposed lock placement
let mut txn = api.txn_begin().await?; + if hardware_info.is_dpu() { + // Segment advisory locks first, before any interface-row write in this + // transaction (e.g. associate_interface_with_dpu_machine below), so the + // allocator lock order holds through the reconcile pass. + db::machine_interface::lock_all_admin_segments(&mut txn).await?; + } tracing::debug!( ?remote_ip, ?interface_id, "discover_machine loading interface" );Then drop the later acquisition at Lines 292-296.
As per path instructions: prefer findings about behavior, concurrency, resource lifetimes over style-only comments.
#!/bin/bash # Confirm associate_interface_with_dpu_machine performs an interface-row write/lock. rg -nP --type=rust -C8 'async fn associate_interface_with_dpu_machine' crates/api-db/src/machine_interface.rs🤖 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 `@crates/api-core/src/handlers/machine_discovery.rs` around lines 291 - 297, Move the DPU admin-segment lock acquisition from the later hardware-info block to immediately after txn_begin(), before any interface-row operations such as associate_interface_with_dpu_machine. Remove the later lock_all_admin_segments call and its outdated comment, ensuring all DPU-path writes follow the segment-then-interface lock order.Source: Path instructions
🤖 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.
Duplicate comments:
In `@crates/api-core/src/handlers/machine_discovery.rs`:
- Around line 291-297: Move the DPU admin-segment lock acquisition from the
later hardware-info block to immediately after txn_begin(), before any
interface-row operations such as associate_interface_with_dpu_machine. Remove
the later lock_all_admin_segments call and its outdated comment, ensuring all
DPU-path writes follow the segment-then-interface lock order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cdb9628-069d-4125-8d46-827322e72057
📒 Files selected for processing (7)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-db/src/machine_interface.rscrates/site-explorer/src/machine_creator.rs
Machine force-deletion, DPU discovery, exploration, and the DHCP SLAAC-replacement path now hold their locks in one global order -- segment advisory locks first, then machine interface/address rows, with the explored-table cleanup ahead of any interface work -- so the 40P01 deadlocks between `AdminForceDeleteMachine` and its concurrent writers are gone. Those surfaced as gRPC `Internal` and took down PR NVIDIA#3293's CI twice in one day (run 29035250694, victims at `explored_managed_host.rs:132` and `machine_interface.rs:2554`/`:1868`). - `admin_force_delete_machine` opens its transaction by advisory-locking every admin segment (the new `db::machine_interface::lock_all_admin_segments`) and moves the `explored_managed_hosts`/`explored_endpoints` cleanup ahead of all interface deletes, matching site-explorer's write order -- the two captured cycles. - The flows that wrote interface rows before their reconcile pass took the segment locks -- DPU discovery (`machine_discovery.rs`), site-explorer machine creation (`machine_creator.rs`), and `set_primary_interface` (`managed_host.rs`) -- hoist the admin-segment locks to their transaction start, and the DHCP SLAAC replacement locks its segment before dropping the address row, so no writer holds interface rows while reaching for a segment lock another transaction owns. - `lock_network_segments_exclusive` is now the one home for the segment lock key and ordering; `lock_network_segment_exclusive` and `load_and_lock_all_admin_segments` funnel through it. - The regression test drives the real force-delete RPC against an exploration-order transaction, synchronized on `pg_stat_activity` lock waits scoped to the test's database; it reproduces the exact CI deadlock against the previous ordering and completes cleanly now. Tests added! This supports NVIDIA#3339 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ﾉ ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/handlers/machine_discovery.rs (1)
118-127: 🚀 Performance & Scalability | 🔵 TrivialLock placement is correct; flag the serialization cost on this hot path.
The advisory lock now sits immediately after
txn_begin(), ahead of every interface-row write on both the DPU and non-DPU paths, correctly aligningdiscover_machinewith the allocator lock order. This properly resolves the deadlock window.One consideration worth confirming:
lock_all_admin_segmentsacquires exclusive advisory locks on every admin segment and holds them for the full transaction (throughreconcile_admin_addresses_for_hostand up tocommitat Line 446). Since it is now unconditional, all concurrentdiscover_machinecalls are fully serialized against one another — and against the allocator, reconcile, and force-delete flows — regardless of which segment a given discovery actually touches. On the DPU path this merely widens the pre-existing reconcile hold-window; on the non-DPU path it introduces serialization that did not previously exist. This is a defensible deadlock-safety tradeoff and consistent with the rest of the cohort, but discovery is a frequently-exercised RPC, so it is worth verifying the expected steady-state concurrency and monitoring advisory-lock wait time on this path.Please confirm the anticipated discovery throughput tolerates full serialization across all admin segments. It may be worth adding a metric on lock-acquisition latency for this transaction to catch contention regressions early.
🤖 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 `@crates/api-core/src/handlers/machine_discovery.rs` around lines 118 - 127, Add a metric around lock acquisition in discover_machine, specifically timing db::machine_interface::lock_all_admin_segments(&mut txn).await, and record the wait duration with appropriate success/error labeling. Confirm via benchmarks or capacity analysis that serializing all discovery transactions across admin segments meets expected throughput, and document or test the expected contention 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.
Nitpick comments:
In `@crates/api-core/src/handlers/machine_discovery.rs`:
- Around line 118-127: Add a metric around lock acquisition in discover_machine,
specifically timing db::machine_interface::lock_all_admin_segments(&mut
txn).await, and record the wait duration with appropriate success/error
labeling. Confirm via benchmarks or capacity analysis that serializing all
discovery transactions across admin segments meets expected throughput, and
document or test the expected contention behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ec97c36-eb1a-460e-8add-9c18210a0e89
📒 Files selected for processing (7)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-db/src/machine_interface.rscrates/site-explorer/src/machine_creator.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/site-explorer/src/machine_creator.rs
- crates/api-core/src/dhcp/discover.rs
- crates/api-core/src/handlers/machine.rs
- crates/api-core/src/handlers/managed_host.rs
- crates/api-db/src/machine_interface.rs
- crates/api-core/src/tests/machine_admin_force_delete.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/handlers/machine_discovery.rs (1)
118-127: 🚀 Performance & Scalability | 🔵 TrivialLock placement is correct; note the global serialization it introduces on the discovery hot path.
Acquiring all admin-segment advisory locks unconditionally at transaction start is the right, conservative choice for lock-order safety. Be aware, however, that this now fully serializes every
discover_machinecall — DPU and non-DPU alike — against one another and against the allocator/reconcile/force-delete transactions. Under high concurrent discovery load this becomes a single contention point. Recommend adding lock-wait/duration observability around this path so the serialization cost is measurable before it manifests as latency in production.🤖 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 `@crates/api-core/src/handlers/machine_discovery.rs` around lines 118 - 127, Add lock-wait and acquisition-duration observability around lock_all_admin_segments in discover_machine, measuring from transaction start through successful lock acquisition and logging the elapsed time with appropriate context. Preserve the existing unconditional lock placement and error propagation while making contention visible for discovery, allocator, reconcile, and force-delete interactions.
🤖 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.
Nitpick comments:
In `@crates/api-core/src/handlers/machine_discovery.rs`:
- Around line 118-127: Add lock-wait and acquisition-duration observability
around lock_all_admin_segments in discover_machine, measuring from transaction
start through successful lock acquisition and logging the elapsed time with
appropriate context. Preserve the existing unconditional lock placement and
error propagation while making contention visible for discovery, allocator,
reconcile, and force-delete interactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 309df0ca-af8b-4b35-85dc-129d941b8bb3
📒 Files selected for processing (7)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-db/src/machine_interface.rscrates/site-explorer/src/machine_creator.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
Caught this during PR #3293 CICD --
test-release-container-servicesfailed twice in one day, both times with Postgres killing a deadlock insideAdminForceDeleteMachineor the discovery flow racing it (run was 29035250694):The root cause is lock ordering: force-delete took machine-interface rows before the explored tables and segment locks, while every writer it races -- site-explorer's persistence pass, DPU discovery, the allocator/reconcile paths -- acquires them the other way around. This PR gives all of those flows one global lock order -- segment advisory locks first, then interface/address rows, with the explored-table cleanup ahead of any interface work -- so the deadlock class is gone, not just the captured instance.
Core changes
admin_force_delete_machineopens its transaction by advisory-locking every admin segment (the newdb::machine_interface::lock_all_admin_segments) and moves theexplored_managed_hosts/explored_endpointscleanup ahead of all interface deletes, matching site-explorer's write order.machine_discovery.rs), site-explorer machine creation (machine_creator.rs), andset_primary_interface(managed_host.rs).lock_network_segments_exclusiveis now the one home for the segment lock key and ordering;lock_network_segment_exclusiveandload_and_lock_all_admin_segmentsfunnel through it.Regression test
pg_stat_activitylock waits scoped to the test's database.Tests added!
This supports #3339
Closes #3339