fix(core): align force-delete topology lock order to avoid observed deadlock in CI/CD

[pbreton]

Summary

We have observed multiple random CI/CD deadlock failures identical to https://github.com/NVIDIA/infra-controller/actions/runs/29973650808/job/89101241767?pr=3905. This PR tries to address it.

• Lock each machine topology before deleting its explored endpoint during AdminForceDeleteMachine.
• Keep topology and endpoint locks in Site Explorer's ascending BMC-IP order.
• Add a deterministic regression test for the topology/endpoint deadlock.

Why

Site Explorer updates machine_topologies before explored_endpoints, while force-delete previously acquired those rows in the opposite order. When the two paths overlapped, PostgreSQL detected the cycle and aborted cleanup_machine_by_id with 40P01.

Testing

• cargo check --package carbide-api-db
• cargo clippy --package carbide-api-db -- -D warnings
• cargo fmt --all

The focused test was also attempted with:

cargo test --package carbide-api-core --no-default-features test_admin_force_delete_orders_topology_before_endpoint -- --nocapture

carbide-test-harness, a dev-dependency, depends back on carbide-api-core with default features and re-enables linux-build. The resulting tss-esapi-sys build rejects the local aarch64-darwin target, so this test still requires Linux CI.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[coderabbitai]

Walkthrough

This PR clarifies force-delete lock-ordering comments in admin_force_delete_machine, adds a lock_by_machine_id helper in machine_topology.rs to lock topology rows via SELECT ... FOR UPDATE, and introduces a regression test with a generalized wait_until_blocked_on_any lock-wait utility.

Changes

Machine force-delete lock ordering

Layer / File(s)	Summary
Topology locking contract and documentation crates/api-db/src/machine_topology.rs, crates/api-core/src/handlers/machine.rs	Adds lock_by_machine_id to lock machine_topologies rows for a given machine within a transaction, and updates comments in admin_force_delete_machine explaining lock ordering for explored_managed_hosts, machine_topologies, and explored_endpoints to avoid deadlocks.
Force-delete concurrency regression test crates/api-core/src/tests/machine_admin_force_delete.rs	Adds test_admin_force_delete_orders_topology_before_endpoint, which simulates concurrent exploration and force-delete transactions contending for topology locks, and replaces the single-relation wait_until_blocked_on with a generalized wait_until_blocked_on_any helper.

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

Sequence Diagram(s)

sequenceDiagram
  participant SiteExplorer as Exploration Transaction
  participant Postgres
  participant ForceDelete as admin_force_delete_machine
  participant Test as Regression Test

  SiteExplorer->>Postgres: Lock machine_topologies row (FOR UPDATE)
  Test->>ForceDelete: Trigger concurrent force-delete
  ForceDelete->>Postgres: lock_by_machine_id (blocks on machine_topologies)
  Test->>Postgres: wait_until_blocked_on_any(machine_topologies, cleanup_machine_by_id)
  Test->>Postgres: Update explored_endpoints while delete blocked
  SiteExplorer->>Postgres: Commit exploration transaction
  Postgres-->>ForceDelete: Lock released
  ForceDelete->>Postgres: Complete cleanup_machine_by_id
  ForceDelete-->>Test: Managed host deleted

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check	✅ Passed	The title clearly summarizes the main change: fixing force-delete lock ordering to avoid deadlocks.
Description check	✅ Passed	The description matches the implemented lock-order fix, regression test, and validation notes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[pbreton]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

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 `@crates/api-db/src/machine_topology.rs`:
- Around line 181-186: Replace the static query execution in the
machine-topology operation with a checked sqlx::query! or query_scalar! macro
while preserving its machine_id binding, transaction usage, and error mapping.
Apply the same checked-macro conversion to the three static query sites in
crates/api-core/src/tests/machine_admin_force_delete.rs at lines 471-475,
493-497, and 522-527; retain each query’s existing behavior and result handling.

🪄 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: a90b31b1-74a8-406d-b93c-5ee38bfe80e3

📥 Commits

Reviewing files that changed from the base of the PR and between 1bac3d0 and 7bdeb67.

📒 Files selected for processing (3)

• crates/api-core/src/handlers/machine.rs
• crates/api-core/src/tests/machine_admin_force_delete.rs
• crates/api-db/src/machine_topology.rs

[kensimon]

I know this is a draft, but before you go to far...

[kensimon]

Don't use the sqlx::query! macro, regardless of what @coderabbitai says. It's an interesting idea but we should either move everything to it or don't do it at all, it's out of scope to completely change the way we do queries (and require SQLX_OFFLINE and throw a bunch of static files in .sqlx) on a whim like this.

Delete all the .sqlx/* files and rollback the Dockerfiles and ci.yaml too.

[coderabbitai]

@kensimon, agreed. Introducing sqlx::query! and its offline-metadata/tooling requirements is out of scope for this targeted lock-ordering fix.

• Replace sqlx::query! in lock_by_machine_id with the existing runtime-query pattern (sqlx::query(QUERY).bind(machine_id)).
• Remove every .sqlx/* file added by this PR.
• Revert this PR’s Dockerfile and ci.yaml changes that enabled or supported SQLx offline metadata.
• Keep the topology lock behavior and regression test intact.

---

✏️ Learnings added

Learnt from: kensimon
Repo: NVIDIA/infra-controller PR: 4015
File: crates/api-db/src/machine_topology.rs:183-183
Timestamp: 2026-07-24T19:04:24.910Z
Learning: For NVIDIA/infra-controller Rust changes, do not introduce `sqlx::query!` macros, SQLX_OFFLINE, or generated `.sqlx` metadata as part of an isolated change. Continue using the repository’s existing runtime SQLx query APIs unless the project deliberately adopts compile-time SQLx queries through a separately scoped migration.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

• 📌 Commit the changes to this branch

[pbreton]

Will do! Thx!

[coderabbitai]

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

[pbreton]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.