fix: prevent duplicate machine interface addresses - #4435
Conversation
Summary by CodeRabbit
WalkthroughThe change enforces globally unique host-form machine-interface addresses. It adds conflict-safe allocation and static replacement, updates admin-network migration logic, and adds concurrency, rollback, migration, and DHCP ownership-contract tests. ChangesMachine-interface address ownership
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MachineInterfaceAllocator
participant MachineInterfaceAddress
participant PostgreSQL
MachineInterfaceAllocator->>MachineInterfaceAddress: Insert allocated host address
MachineInterfaceAddress->>PostgreSQL: Claim globally unique address
PostgreSQL-->>MachineInterfaceAddress: Success or ownership conflict
MachineInterfaceAddress-->>MachineInterfaceAllocator: Return insert result
MachineInterfaceAllocator->>PostgreSQL: Roll back and retry allocation
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/dhcp/v6.rs (1)
98-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe two conflict paths return different error kinds to the caller.
The sequential pre-check at lines 107-118 returns
CarbideError::FailedPreconditionwith a descriptive message. The race path returns whatevermachine_interface_address::insertproduces, which isDatabaseError::AddressAlreadyInUseconverted through?. A DHCPv6 client therefore sees two different statuses for the same logical condition, depending on timing. Consider mappingDatabaseError::AddressAlreadyInUseto the sameFailedPreconditionso the observable behaviour is timing-independent.The comment also records a TODO for same-interface family replacement. I can open a tracking issue for that shared transaction policy if you want.
🤖 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/dhcp/v6.rs` around lines 98 - 126, Update the SLAAC insertion path in the function containing the find_by_address pre-check so db::machine_interface_address::insert maps DatabaseError::AddressAlreadyInUse to the same CarbideError::FailedPrecondition and descriptive duplicate-allocation message used by the sequential conflict path. Preserve propagation of all other insertion errors and leave the same-interface family replacement TODO unchanged.
🧹 Nitpick comments (10)
crates/api-db/src/machine_interface.rs (3)
2396-2405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
query_scalarfor this single-column result.The query selects one column.
query_asforces a one-element tuple and an extra destructuring pattern in thematch.query_scalarexpresses the intent directly.♻️ Proposed refactor
- let interface_id: Option<(MachineInterfaceId,)> = sqlx::query_as(query) + let interface_id: Option<MachineInterfaceId> = sqlx::query_scalar(query) .bind(remote_ip) .fetch_optional(&mut *txn) .await .map_err(|e| DatabaseError::query(query, e))?; match interface_id { - Some((interface_id,)) => find_one(txn, interface_id).await.map(Some), + Some(interface_id) => find_one(txn, interface_id).await.map(Some), None => Ok(None), }Based on learnings: "when using sqlx to fetch exactly one scalar value from a query ... prefer
sqlx::query_scalar(query).bind(...).fetch_one(txn)oversqlx::query_as(...)".🤖 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-db/src/machine_interface.rs` around lines 2396 - 2405, Update the single-column query in the interface lookup to use sqlx::query_scalar with the existing MachineInterfaceId option type, removing the one-element tuple annotation and destructuring in the match while preserving the current fetch_optional and error handling behavior.Source: Learnings
3549-3565: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the extra allocation when ordering the insert.
allocated_addresses.clone()exists only to produce a sorted insertion order while the unsorted vector is returned. An iterator-based sort expresses that intent without cloning the vector.♻️ Proposed refactor
let allocated_addresses = allocate_addresses_from_segment(txn, family_segment).await?; - let mut address_insert_order = allocated_addresses.clone(); - address_insert_order.sort_unstable(); - for address in address_insert_order { + for address in allocated_addresses.iter().copied().sorted() { crate::machine_interface_address::insert( txn.as_mut(), interface_id, address, AllocationType::Dhcp, ) .await?; }This requires
itertools::Itertoolsin scope. The vectors are tiny, so the benefit is readability rather than throughput.🤖 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-db/src/machine_interface.rs` around lines 3549 - 3565, Update try_allocate_addresses_for_family to avoid cloning allocated_addresses solely for sorting: use an iterator-based ordering approach, such as itertools, to iterate addresses in sorted order for insertion while preserving the original vector for the return value. Add the required Itertools import and keep the existing insertion behavior unchanged.
55-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReconcile the two retry bounds for address-ownership conflicts.
ALLOCATION_CONFLICT_MAX_RETRIESis 8 and documents that each attempt rebuilds the candidate set. The fast path at lines 1743-1748 now also retries onDatabaseError::AddressAlreadyInUse, but it usesFAST_PATH_MAX_RETRIES(128). One conflict class therefore has two different bounds depending on the code path. Under sustained static-writer contention the fast path can rebuild its candidate batch 128 times while holding a segment lock. Consider bounding the new conflict class withALLOCATION_CONFLICT_MAX_RETRIESin the fast path as well, or document why 128 is correct there.🤖 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-db/src/machine_interface.rs` around lines 55 - 59, The fast-path handling of DatabaseError::AddressAlreadyInUse uses a different retry bound than the allocation-wide conflict policy. Update the fast-path retry logic near its AddressAlreadyInUse handling to use ALLOCATION_CONFLICT_MAX_RETRIES, or explicitly establish and document why FAST_PATH_MAX_RETRIES is required for this conflict class; keep unrelated fast-path retries unchanged.crates/api-db/src/machine_interface/tests.rs (2)
727-753: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated MAC literal from the trigger body.
The trigger function hardcodes
'02:00:00:20:03:12'::macaddr, whileowner_macis declared at line 702 with the same value. If either literal changes, the trigger stops matching, the owner never blocks on the advisory lock, and the test fails later with the opaque timeout message at line 838 instead of pointing at the mismatch. Derive the SQL from the Rust value so the two cannot drift.♻️ Proposed refactor
- sqlx::raw_sql( + sqlx::raw_sql(&format!( "CREATE FUNCTION test_block_fixed_address_insert() RETURNS trigger LANGUAGE plpgsql AS $$ BEGIN IF EXISTS ( SELECT 1 FROM machine_interfaces WHERE id = NEW.interface_id - AND mac_address = '02:00:00:20:03:12'::macaddr + AND mac_address = '{owner_mac}'::macaddr ) THENThe remainder of the statement is unchanged. Note that
$$in aformat!string needs no escaping, but the literal braces do not appear here, so the substitution is safe.🤖 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-db/src/machine_interface/tests.rs` around lines 727 - 753, Update the trigger SQL setup around test_block_fixed_address_insert to derive the embedded macaddr value from the existing owner_mac Rust variable instead of duplicating the hardcoded MAC literal. Format the SQL string while preserving the trigger body and advisory-lock behavior, ensuring owner_mac remains the single source of truth.
867-887: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the actual result when the ownership conflict assertion fails.
assert!(matches!(...))with a guard prints only the pattern. The other race helpers in this file use amatchwithother => panic!("... got {other:?}"), which names the observed error. Aligning this assertion with that style shortens diagnosis when the ordering under test regresses.🤖 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-db/src/machine_interface/tests.rs` around lines 867 - 887, The ownership conflict assertion around owner_result should report the actual unexpected error instead of relying on assert!(matches!). Replace it with an explicit match that preserves the existing field validations for DatabaseError::AddressAlreadyInUse and panics on other results with the observed value using the style of the nearby race helpers.crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql (1)
21-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify that the reported total counts duplicate groups, not duplicate rows.
duplicate_address_countcounts rows returned by the grouped subquery, so it is the number of duplicated host addresses. The message text "(% total; showing at most 20)" reads as a row total. An operator reading "1 total" for two conflicting owners may under-estimate the work. Consider naming the unit explicitly.📝 Proposed message change
IF duplicate_addresses IS NOT NULL THEN RAISE EXCEPTION - 'machine_interface_addresses contains duplicate host addresses (% total; showing at most 20): %', + 'machine_interface_addresses contains duplicate host addresses (% duplicated addresses; showing at most 20): %', duplicate_address_count,Note: this change requires updating the assertion
message.contains("1 total")incrates/api-db/src/machine_interface_address.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-db/migrations/20260730222152_machine_interface_addresses_unique.sql` around lines 21 - 61, Update the migration exception raised by the duplicate-address check to explicitly label duplicate_address_count as the number of duplicate host-address groups, not rows. Adjust the related assertion in the machine-interface address tests to match the new message wording while preserving the existing count and detail output.crates/api-db/src/machine_interface_address.rs (3)
218-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe bounded retry is sound. Add coverage for the exhausted path.
The ownerless-conflict retry and the terminal
DatabaseError::internalat lines 247-249 have no test. That path is reachable only when a competing writer inserts and deletes the same address three times in a row, so a unit-level test is impractical, but the loop bound is a behavioural contract. Consider extracting the loop body so a test can drive the ownerless branch directly, or accept the gap explicitly.🤖 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-db/src/machine_interface_address.rs` around lines 218 - 249, Add test coverage for the retry loop’s exhausted ownerless-conflict path, validating that repeated insert/delete conflicts reach the terminal DatabaseError::internal result after the configured bound. If direct integration setup is impractical, extract the loop body or a suitable seam around the address-assignment logic so the test can drive the ownerless branch deterministically without changing normal behavior.
610-672: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the constraint drops so the migration test does not depend on prior migration state.
Lines 616-623 drop
machine_interface_addresses_address_keyandmachine_interface_addresses_host_address_checkwithoutIF EXISTS. The same pattern repeats at lines 680-687. If the fixture ever runs against a schema built before this migration, both tests fail during setup rather than reporting the behaviour under test. AddingIF EXISTSkeeps the intent and removes the ordering dependency.♻️ Proposed change (apply to both tests)
sqlx::raw_sql( "ALTER TABLE machine_interface_addresses - DROP CONSTRAINT machine_interface_addresses_address_key; + DROP CONSTRAINT IF EXISTS machine_interface_addresses_address_key; ALTER TABLE machine_interface_addresses - DROP CONSTRAINT machine_interface_addresses_host_address_check;", + DROP CONSTRAINT IF EXISTS machine_interface_addresses_host_address_check;", )🤖 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-db/src/machine_interface_address.rs` around lines 610 - 672, Update the setup SQL in both migration tests, including migration_rejects_duplicate_hosts_with_different_masks and the second test’s corresponding constraint-drop block, to use IF EXISTS when dropping machine_interface_addresses_address_key and machine_interface_addresses_host_address_check. Preserve the existing test behavior while allowing setup to succeed regardless of prior migration state.
196-217: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDrop only the redundant unique constraint
machine_interface_addresses_interface_id_address_keyis redundant aftermachine_interface_addresses_address_keyenforces global address uniqueness. Drop it in this migration. Keepunique_address_family_on_interface; it enforces the separate one-address-per-family invariant.🤖 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-db/src/machine_interface_address.rs` around lines 196 - 217, Update the migration associated with the machine_interface_addresses schema to drop only the redundant machine_interface_addresses_interface_id_address_key constraint. Preserve machine_interface_addresses_address_key for global address uniqueness and retain unique_address_family_on_interface for the per-interface address-family invariant.crates/api-core/src/tests/machine_discovery.rs (1)
891-902: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the specific error variant, not just that an error occurred.
The test currently uses
assert!(missing.is_err()), which passes for any failure, including connection or serialization errors unrelated to the missing mapping. Verify that the function returns the expectedNotFoundErrorvariant to ensure the test validates the intended behavior.Proposed change
let missing = db::machine_interface::find_for_update_by_ip(&mut txn, "203.0.113.253".parse().unwrap()) .await; - assert!(missing.is_err()); + assert!( + matches!(missing, Err(DatabaseError::NotFoundError { .. })), + "expected a not-found error, got {missing:?}" + ); Ok(())🤖 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/tests/machine_discovery.rs` around lines 891 - 902, Update test_discovery_ip_lookup_rejects_missing_mapping to assert that find_for_update_by_ip returns the expected NotFoundError variant for the unmapped address, rather than only checking is_err(). Preserve the existing transaction setup and ensure unrelated database or serialization errors do not satisfy the assertion.
🤖 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.
Outside diff comments:
In `@crates/api-core/src/dhcp/v6.rs`:
- Around line 98-126: Update the SLAAC insertion path in the function containing
the find_by_address pre-check so db::machine_interface_address::insert maps
DatabaseError::AddressAlreadyInUse to the same CarbideError::FailedPrecondition
and descriptive duplicate-allocation message used by the sequential conflict
path. Preserve propagation of all other insertion errors and leave the
same-interface family replacement TODO unchanged.
---
Nitpick comments:
In `@crates/api-core/src/tests/machine_discovery.rs`:
- Around line 891-902: Update test_discovery_ip_lookup_rejects_missing_mapping
to assert that find_for_update_by_ip returns the expected NotFoundError variant
for the unmapped address, rather than only checking is_err(). Preserve the
existing transaction setup and ensure unrelated database or serialization errors
do not satisfy the assertion.
In
`@crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql`:
- Around line 21-61: Update the migration exception raised by the
duplicate-address check to explicitly label duplicate_address_count as the
number of duplicate host-address groups, not rows. Adjust the related assertion
in the machine-interface address tests to match the new message wording while
preserving the existing count and detail output.
In `@crates/api-db/src/machine_interface_address.rs`:
- Around line 218-249: Add test coverage for the retry loop’s exhausted
ownerless-conflict path, validating that repeated insert/delete conflicts reach
the terminal DatabaseError::internal result after the configured bound. If
direct integration setup is impractical, extract the loop body or a suitable
seam around the address-assignment logic so the test can drive the ownerless
branch deterministically without changing normal behavior.
- Around line 610-672: Update the setup SQL in both migration tests, including
migration_rejects_duplicate_hosts_with_different_masks and the second test’s
corresponding constraint-drop block, to use IF EXISTS when dropping
machine_interface_addresses_address_key and
machine_interface_addresses_host_address_check. Preserve the existing test
behavior while allowing setup to succeed regardless of prior migration state.
- Around line 196-217: Update the migration associated with the
machine_interface_addresses schema to drop only the redundant
machine_interface_addresses_interface_id_address_key constraint. Preserve
machine_interface_addresses_address_key for global address uniqueness and retain
unique_address_family_on_interface for the per-interface address-family
invariant.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 2396-2405: Update the single-column query in the interface lookup
to use sqlx::query_scalar with the existing MachineInterfaceId option type,
removing the one-element tuple annotation and destructuring in the match while
preserving the current fetch_optional and error handling behavior.
- Around line 3549-3565: Update try_allocate_addresses_for_family to avoid
cloning allocated_addresses solely for sorting: use an iterator-based ordering
approach, such as itertools, to iterate addresses in sorted order for insertion
while preserving the original vector for the return value. Add the required
Itertools import and keep the existing insertion behavior unchanged.
- Around line 55-59: The fast-path handling of
DatabaseError::AddressAlreadyInUse uses a different retry bound than the
allocation-wide conflict policy. Update the fast-path retry logic near its
AddressAlreadyInUse handling to use ALLOCATION_CONFLICT_MAX_RETRIES, or
explicitly establish and document why FAST_PATH_MAX_RETRIES is required for this
conflict class; keep unrelated fast-path retries unchanged.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 727-753: Update the trigger SQL setup around
test_block_fixed_address_insert to derive the embedded macaddr value from the
existing owner_mac Rust variable instead of duplicating the hardcoded MAC
literal. Format the SQL string while preserving the trigger body and
advisory-lock behavior, ensuring owner_mac remains the single source of truth.
- Around line 867-887: The ownership conflict assertion around owner_result
should report the actual unexpected error instead of relying on
assert!(matches!). Replace it with an explicit match that preserves the existing
field validations for DatabaseError::AddressAlreadyInUse and panics on other
results with the observed value using the style of the nearby race helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 379cc84c-7cb4-4a93-9fad-175e8bfbb7c7
📒 Files selected for processing (8)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/tests/machine_discovery.rscrates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sqlcrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
crates/api-db/src/machine_interface.rs (3)
3660-3685: 🚀 Performance & Scalability | 🔵 TrivialConfirm the range join uses the new unique index on
address.The predicate compares
mia.addressagainsthost(network(...))::inetandhost(broadcast(...))::inet. This is index-eligible on the btree that the newUNIQUE (address)constraint creates, and the migration comment at Lines 79-80 states that intent. Two points deserve a check on a populated database:
network(np.prefix::inet)andbroadcast(np.prefix::inet)are stable per row, so the planner should choose an index range scan per prefix rather than a sequential scan ofmachine_interface_addresses. Confirm withEXPLAINon a segment that has several prefixes.- This resolver now also feeds
update_num_free_ips_into_prefix_listincrates/api-db/src/network_segment.rsLines 748-751, which runs per segment on list operations. A sequential-scan plan there would scale with total site addresses rather than with segment size.🤖 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-db/src/machine_interface.rs` around lines 3660 - 3685, Validate the range join in UsedMachineInterfaceIpResolver::used_ips against a populated database using EXPLAIN for a segment with multiple prefixes, confirming the UNIQUE(address) index on machine_interface_addresses is used for per-prefix range scans rather than a sequential scan. Also verify the plan remains efficient when used by update_num_free_ips_into_prefix_list, scaling with the segment’s prefixes rather than all site addresses.
3549-3567: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop the clone by sorting the allocated addresses in place.
allocated_addresses.clone()exists only to keep the returned vector in allocator order while inserting in sorted order. No caller depends on allocator order: the tests atcrates/api-db/src/machine_interface/tests.rsLines 519-528 assert membership and family only. If that holds, sort once and return the sorted vector.♻️ Proposed refactor
- let allocated_addresses = allocate_addresses_from_segment(txn, family_segment).await?; - let mut address_insert_order = allocated_addresses.clone(); - address_insert_order.sort_unstable(); - for address in address_insert_order { + let mut allocated_addresses = allocate_addresses_from_segment(txn, family_segment).await?; + allocated_addresses.sort_unstable(); + for address in &allocated_addresses { crate::machine_interface_address::insert( txn.as_mut(), interface_id, - address, + *address, AllocationType::Dhcp, ) .await?; }As per coding guidelines: "Avoid needless
.clone()calls; prefer borrowing, moving withinto_iter...".🤖 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-db/src/machine_interface.rs` around lines 3549 - 3567, Update try_allocate_addresses_for_family to sort allocated_addresses in place, remove the cloned address_insert_order vector, insert addresses by consuming or iterating over the sorted allocation, and return the sorted allocated_addresses.Source: Coding guidelines
2396-2405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
query_scalarfor this single-column projection.The query selects one column.
query_scalarremoves the one-element tuple and its destructuring.♻️ Proposed refactor
- let interface_id: Option<(MachineInterfaceId,)> = sqlx::query_as(query) + let interface_id: Option<MachineInterfaceId> = sqlx::query_scalar(query) .bind(remote_ip) .fetch_optional(&mut *txn) .await .map_err(|e| DatabaseError::query(query, e))?; match interface_id { - Some((interface_id,)) => find_one(txn, interface_id).await.map(Some), + Some(interface_id) => find_one(txn, interface_id).await.map(Some), None => Ok(None), }Based on learnings: "when using sqlx to fetch exactly one scalar value from a query ... prefer
sqlx::query_scalar(query).bind(...).fetch_one(txn)oversqlx::query_as(query)".🤖 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-db/src/machine_interface.rs` around lines 2396 - 2405, Replace the query_as call with query_scalar since the query selects a single column. Update the type annotation for interface_id from Option<(MachineInterfaceId,)> to Option<MachineInterfaceId> to remove the tuple wrapper. Simplify the match statement so that the Some branch receives interface_id directly without tuple destructuring, and update the None branch accordingly.Source: Learnings
crates/api-core/src/dhcp/v6.rs (1)
98-106: 🩺 Stability & Availability | 🔵 TrivialTrack the remaining same-interface family race.
The comment documents a real residual gap: a competing static or DHCPv6 write on the same interface family still surfaces a raw database error to the losing caller. The scoping is correct for this PR, because the global address constraint is the concern here.
Do you want me to open an issue to track a shared transaction policy for same-interface family replacement across the SLAAC, DHCP, and static writers?
🤖 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/dhcp/v6.rs` around lines 98 - 106, Track the remaining same-interface family replacement race as a follow-up issue covering the SLAAC, DHCPv6, and static writers, including the need for a shared transaction policy and graceful handling of the losing request’s database error. Keep the current scope and implementation unchanged.crates/api-db/src/machine_interface_address.rs (1)
179-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a log record when the ownerless-conflict retry loop repeats.
The loop hides a rare but interesting state: the conflicting row disappeared between the
INSERTand the owner lookup. When this path triggers, the only visible outcome is either success or a generic internal error after three attempts. A singletracing::warn!on the retry keeps that race diagnosable in production without changing behavior.♻️ Proposed refactor
// The winner disappeared between conflict detection and lookup. Try // the insert again rather than returning an ownerless conflict. + tracing::warn!( + %address, + %interface_id, + "retrying machine interface address insert after an ownerless conflict" + ); }🤖 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-db/src/machine_interface_address.rs` around lines 179 - 250, Add a single tracing::warn! call in the ownerless-conflict branch of insert, immediately before the loop retries after the owner lookup finds no row. Include relevant address and interface context, while preserving the existing retry and error behavior.crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql (1)
90-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer an explicit temp-table lifecycle over
IF NOT EXISTSplusTRUNCATE.The procedure can run twice inside one transaction.
ON COMMIT DROPonly fires at commit, so the second call reuses the first call's table and depends onTRUNCATEfor correctness. The current form is correct, but aDROP TABLE IF EXISTSfollowed byCREATE TEMP TABLEstates the intent directly and removes the dependency on the column definitions staying identical between calls.♻️ Proposed refactor
- CREATE TEMP TABLE IF NOT EXISTS tmp_admin_network_address_mapping ( + DROP TABLE IF EXISTS tmp_admin_network_address_mapping; + CREATE TEMP TABLE tmp_admin_network_address_mapping ( address_id uuid PRIMARY KEY, interface_id uuid NOT NULL, address inet NOT NULL, allocation_type text NOT NULL, domain_id uuid, hostname varchar(63) NOT NULL ) ON COMMIT DROP; - TRUNCATE tmp_admin_network_address_mapping;🤖 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-db/migrations/20260730222152_machine_interface_addresses_unique.sql` around lines 90 - 159, In the update_admin_network procedure, replace the `CREATE TEMP TABLE IF NOT EXISTS tmp_admin_network_address_mapping ... ON COMMIT DROP` pattern paired with the subsequent `TRUNCATE` statement with an explicit lifecycle: first drop the table unconditionally using `DROP TABLE IF EXISTS tmp_admin_network_address_mapping`, then immediately create the temp table using `CREATE TEMP TABLE tmp_admin_network_address_mapping` (without IF NOT EXISTS, keeping the ON COMMIT DROP clause). This removes the dependency on TRUNCATE for correctness when the procedure runs twice within a single transaction.crates/api-db/src/machine_interface/tests.rs (1)
212-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDo not retain the four inline suppressions without an explicit policy exception. The custom lint has no test-only or path exclusion, and
#[cfg(test)]does not exempt anallow. Either restructure the synchronization helpers to avoid holding the transaction across unrelated awaits, or document and approve a narrowly scoped test exception inSTYLE_GUIDE.md.🤖 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-db/src/machine_interface/tests.rs` around lines 212 - 217, Remove the inline txn_held_across_await suppressions from the synchronization tests, including the one above the fixed/fixed race test. Prefer restructuring the synchronization helpers so transactions are not held across unrelated awaits; if that cannot be done, document and obtain approval for a narrowly scoped exception in STYLE_GUIDE.md rather than retaining undocumented allows.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-db/src/machine_interface/tests.rs`:
- Around line 727-753: Update the trigger SQL in the test setup around owner_mac
so it reuses the already-bound owner_mac value instead of duplicating the
hard-coded MAC literal in the trigger body. Interpolate or otherwise bind that
value when constructing the SQL, while preserving the existing trigger behavior
and ensuring the owner address remains the one gated by the advisory lock.
---
Nitpick comments:
In `@crates/api-core/src/dhcp/v6.rs`:
- Around line 98-106: Track the remaining same-interface family replacement race
as a follow-up issue covering the SLAAC, DHCPv6, and static writers, including
the need for a shared transaction policy and graceful handling of the losing
request’s database error. Keep the current scope and implementation unchanged.
In
`@crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql`:
- Around line 90-159: In the update_admin_network procedure, replace the `CREATE
TEMP TABLE IF NOT EXISTS tmp_admin_network_address_mapping ... ON COMMIT DROP`
pattern paired with the subsequent `TRUNCATE` statement with an explicit
lifecycle: first drop the table unconditionally using `DROP TABLE IF EXISTS
tmp_admin_network_address_mapping`, then immediately create the temp table using
`CREATE TEMP TABLE tmp_admin_network_address_mapping` (without IF NOT EXISTS,
keeping the ON COMMIT DROP clause). This removes the dependency on TRUNCATE for
correctness when the procedure runs twice within a single transaction.
In `@crates/api-db/src/machine_interface_address.rs`:
- Around line 179-250: Add a single tracing::warn! call in the
ownerless-conflict branch of insert, immediately before the loop retries after
the owner lookup finds no row. Include relevant address and interface context,
while preserving the existing retry and error behavior.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 3660-3685: Validate the range join in
UsedMachineInterfaceIpResolver::used_ips against a populated database using
EXPLAIN for a segment with multiple prefixes, confirming the UNIQUE(address)
index on machine_interface_addresses is used for per-prefix range scans rather
than a sequential scan. Also verify the plan remains efficient when used by
update_num_free_ips_into_prefix_list, scaling with the segment’s prefixes rather
than all site addresses.
- Around line 3549-3567: Update try_allocate_addresses_for_family to sort
allocated_addresses in place, remove the cloned address_insert_order vector,
insert addresses by consuming or iterating over the sorted allocation, and
return the sorted allocated_addresses.
- Around line 2396-2405: Replace the query_as call with query_scalar since the
query selects a single column. Update the type annotation for interface_id from
Option<(MachineInterfaceId,)> to Option<MachineInterfaceId> to remove the tuple
wrapper. Simplify the match statement so that the Some branch receives
interface_id directly without tuple destructuring, and update the None branch
accordingly.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 212-217: Remove the inline txn_held_across_await suppressions from
the synchronization tests, including the one above the fixed/fixed race test.
Prefer restructuring the synchronization helpers so transactions are not held
across unrelated awaits; if that cannot be done, document and obtain approval
for a narrowly scoped exception in STYLE_GUIDE.md rather than retaining
undocumented allows.
🪄 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: c0f9b194-aff9-4570-90fa-603f46f9a79c
📒 Files selected for processing (8)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/tests/machine_discovery.rscrates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sqlcrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql (1)
55-61: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGate the abort on the count, not on the formatted list.
duplicate_address_countis derived frommachine_interface_addressesalone.duplicate_addressesis derived from an inner join tomachine_interfaces. If a duplicate address row has no matching interface row,string_aggreturns NULL, the guard passes, andADD CONSTRAINT machine_interface_addresses_address_keythen fails with a generic unique-violation message instead of the curated audit report. Gate on the count and keep the list as diagnostic detail.♻️ Proposed hardening of the audit guard
- IF duplicate_addresses IS NOT NULL THEN + IF duplicate_address_count > 0 THEN RAISE EXCEPTION 'machine_interface_addresses contains duplicate host addresses (% total; showing at most 20): %', duplicate_address_count, - duplicate_addresses + COALESCE(duplicate_addresses, '(owner details unavailable; rows without a matching interface)') USING HINT = 'Resolve each duplicate owner before retrying this migration.'; END IF;🤖 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-db/migrations/20260730222152_machine_interface_addresses_unique.sql` around lines 55 - 61, Update the audit guard around duplicate_address_count and duplicate_addresses to raise the curated exception whenever duplicate_address_count indicates duplicates, even if duplicate_addresses is NULL. Keep duplicate_addresses as optional diagnostic detail in the exception and preserve the existing migration constraint flow when the count is zero.
🤖 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/migrations/20260730222152_machine_interface_addresses_unique.sql`:
- Around line 216-231: Update the staging query producing named_addresses so
each interface contributes one deterministic hostname when multiple
remapped-family addresses exist, selecting the lowest new address per interface
via a DISTINCT ON projection or equivalent ordering. Apply the same
per-interface deterministic selection to the domain_id restore used by the later
machine_interfaces update, while preserving the existing address mapping rows.
---
Nitpick comments:
In
`@crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql`:
- Around line 55-61: Update the audit guard around duplicate_address_count and
duplicate_addresses to raise the curated exception whenever
duplicate_address_count indicates duplicates, even if duplicate_addresses is
NULL. Keep duplicate_addresses as optional diagnostic detail in the exception
and preserve the existing migration constraint flow when the count is zero.
🪄 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: 0d6de81a-0f18-45ef-b439-cd67f323730d
📒 Files selected for processing (8)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/tests/machine_discovery.rscrates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sqlcrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/api-core/src/tests/machine_discovery.rs
- crates/api-db/src/network_segment.rs
- crates/api-core/src/dhcp/v6.rs
- crates/api-db/src/machine_interface_address.rs
- crates/api-db/src/machine_interface.rs
- crates/api-db/src/machine_interface/tests.rs
- crates/api-core/src/dhcp/expire.rs
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4435.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/api-core/src/dhcp/v6.rs (1)
98-106: 🩺 Stability & Availability | 🔵 TrivialTrack the remaining family-replacement gap.
The comment accurately describes the new boundary. The TODO still records a user-visible defect: a competing static or DHCPv6 write on the same interface family returns a generic database error instead of a typed conflict, because that path has no
ON CONFLICThandling.Do you want me to open an issue that captures this shared transaction policy for SLAAC, DHCP, and static writers?
🤖 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/dhcp/v6.rs` around lines 98 - 106, Track the remaining same-interface family-replacement gap by opening an issue covering a shared transaction policy for the SLAAC, DHCPv6, and static writers. Reference the per-interface family constraint and missing ON CONFLICT handling, and specify that competing writes must return a typed conflict instead of a generic database error.crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql (1)
75-84: 🩺 Stability & Availability | 🔵 TrivialConfirm the maintenance window covers the constraint build.
The migration takes
ACCESS EXCLUSIVEat Line 14 and then builds two constraints. TheUNIQUE (address)build at Line 84 scans the whole table while that lock is held, so all reads and writes onmachine_interface_addresseswait for the duration.The Squawk suggestions (
NOT VALID,CONCURRENTLY) do not apply here.sqlx::migrate!()runs migrations in a transaction, andUNIQUEconstraints cannot be addedNOT VALID. The remaining action is operational: size the expected build time against the largest deployment and schedule accordingly.🤖 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-db/migrations/20260730222152_machine_interface_addresses_unique.sql` around lines 75 - 84, The migration’s UNIQUE constraint build on machine_interface_addresses requires an ACCESS EXCLUSIVE lock and may block reads and writes while scanning the table. Confirm the expected constraint-build duration against the largest deployment and schedule this migration within an adequate maintenance window; no SQL change is required.Sources: Learnings, Linters/SAST tools
crates/api-db/src/machine_interface_address.rs (1)
621-628: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the constraint-drop setup into a helper.
Both migration tests execute the same two
ALTER TABLE ... DROP CONSTRAINTstatements. A future constraint rename must be applied in two places.Add one helper next to
create_test_segmentand call it from both tests.♻️ Proposed helper
/// Restore the pre-migration schema so the migration can be replayed. async fn drop_address_uniqueness_constraints(pool: &sqlx::PgPool) -> Result<(), sqlx::Error> { sqlx::raw_sql( "ALTER TABLE machine_interface_addresses DROP CONSTRAINT machine_interface_addresses_address_key; ALTER TABLE machine_interface_addresses DROP CONSTRAINT machine_interface_addresses_host_address_check;", ) .execute(pool) .await .map(|_| ()) }Also applies to: 685-692
🤖 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-db/src/machine_interface_address.rs` around lines 621 - 628, Extract the duplicated constraint-drop SQL into a helper adjacent to create_test_segment, such as drop_address_uniqueness_constraints, returning Result<(), sqlx::Error> and executing against the provided pool. Replace the inline ALTER TABLE blocks in both migration tests with calls to this helper, preserving their existing error propagation.crates/api-core/src/tests/machine_discovery.rs (1)
897-900: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific error, not any error.
assert!(missing.is_err())passes for any failure, including a connection or query error unrelated to the missing mapping. Match the expected "not found" variant so the test verifies the lookup contract.♻️ Proposed assertion
- assert!(missing.is_err()); + assert!( + matches!(missing, Err(db::DatabaseError::NotFound(_))), + "expected a not-found error, got {missing:?}" + );Adjust the variant to the error
find_for_update_by_ipreturns for an unmapped address.🤖 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/tests/machine_discovery.rs` around lines 897 - 900, Update the assertion for the `find_for_update_by_ip` lookup of the unmapped address to match the specific “not found” error variant it returns, rather than accepting any error. Preserve the async lookup and ensure unrelated database or query failures do not satisfy the test.crates/api-db/src/machine_interface/tests.rs (1)
212-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared race scaffolding.
The three runners repeat the same sequence: begin the winner transaction, read its backend PID, spawn the contender, hand its PID back over a
oneshot, wait for the lock conflict, commit the winner, then join with a 5-second timeout and abort on expiry. Only the contender operation and the assertions differ.One helper that accepts the contender future and returns its result would remove roughly 40 duplicated lines per runner. This is a test-only cleanup and can be deferred.
Also applies to: 335-428, 430-530
🤖 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-db/src/machine_interface/tests.rs` around lines 212 - 333, Extract the repeated transaction-race orchestration from run_fixed_address_race and the other two race runners into one test-only helper. Have it begin the winner transaction, obtain and exchange backend PIDs via oneshot, wait with wait_for_transaction_conflict, commit the winner, and join the contender with the existing 5-second timeout and abort handling; parameterize only the contender operation and return its result so each runner retains its own assertions.crates/api-db/src/machine_interface.rs (1)
2396-2405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
query_scalarfor the single-column lookup.The query returns one column.
query_scalarremoves the one-element tuple and its destructuring.♻️ Proposed simplification
- let interface_id: Option<(MachineInterfaceId,)> = sqlx::query_as(query) - .bind(remote_ip) - .fetch_optional(&mut *txn) - .await - .map_err(|e| DatabaseError::query(query, e))?; - - match interface_id { - Some((interface_id,)) => find_one(txn, interface_id).await.map(Some), - None => Ok(None), - } + let interface_id: Option<MachineInterfaceId> = sqlx::query_scalar(query) + .bind(remote_ip) + .fetch_optional(&mut *txn) + .await + .map_err(|e| DatabaseError::query(query, e))?; + + match interface_id { + Some(interface_id) => find_one(txn, interface_id).await.map(Some), + None => Ok(None), + }Based on learnings: in Rust code under
crates/api-db/, prefersqlx::query_scalar(query).bind(...)oversqlx::query_as(query).bind(...)when fetching exactly one scalar value, because it avoids destructuring a single-element tuple.🤖 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-db/src/machine_interface.rs` around lines 2396 - 2405, Update the single-column lookup in the surrounding function to use sqlx::query_scalar instead of query_as, and change the result type and match arm to handle MachineInterfaceId directly without a one-element tuple destructuring. Preserve the existing bind, error mapping, and find_one behavior.Source: Learnings
🤖 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/migrations/20260730222152_machine_interface_addresses_unique.sql`:
- Around line 256-265: Update the remapping logic for update_admin_network and
tmp_admin_network_address_mapping to retain each interface’s previous address,
and conditionally rewrite hostname only when the stored value matches the old
IP-derived hostname; preserve fun, serial, MAC, and other non-IP hostnames
unchanged while still updating domain_id.
---
Nitpick comments:
In `@crates/api-core/src/dhcp/v6.rs`:
- Around line 98-106: Track the remaining same-interface family-replacement gap
by opening an issue covering a shared transaction policy for the SLAAC, DHCPv6,
and static writers. Reference the per-interface family constraint and missing ON
CONFLICT handling, and specify that competing writes must return a typed
conflict instead of a generic database error.
In `@crates/api-core/src/tests/machine_discovery.rs`:
- Around line 897-900: Update the assertion for the `find_for_update_by_ip`
lookup of the unmapped address to match the specific “not found” error variant
it returns, rather than accepting any error. Preserve the async lookup and
ensure unrelated database or query failures do not satisfy the test.
In
`@crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql`:
- Around line 75-84: The migration’s UNIQUE constraint build on
machine_interface_addresses requires an ACCESS EXCLUSIVE lock and may block
reads and writes while scanning the table. Confirm the expected constraint-build
duration against the largest deployment and schedule this migration within an
adequate maintenance window; no SQL change is required.
In `@crates/api-db/src/machine_interface_address.rs`:
- Around line 621-628: Extract the duplicated constraint-drop SQL into a helper
adjacent to create_test_segment, such as drop_address_uniqueness_constraints,
returning Result<(), sqlx::Error> and executing against the provided pool.
Replace the inline ALTER TABLE blocks in both migration tests with calls to this
helper, preserving their existing error propagation.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 2396-2405: Update the single-column lookup in the surrounding
function to use sqlx::query_scalar instead of query_as, and change the result
type and match arm to handle MachineInterfaceId directly without a one-element
tuple destructuring. Preserve the existing bind, error mapping, and find_one
behavior.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 212-333: Extract the repeated transaction-race orchestration from
run_fixed_address_race and the other two race runners into one test-only helper.
Have it begin the winner transaction, obtain and exchange backend PIDs via
oneshot, wait with wait_for_transaction_conflict, commit the winner, and join
the contender with the existing 5-second timeout and abort handling;
parameterize only the contender operation and return its result so each runner
retains its own assertions.
🪄 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: 06d5d9be-181c-48bc-96fd-8ea41bd13888
📒 Files selected for processing (8)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/tests/machine_discovery.rscrates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sqlcrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rs
Enforce one database owner per machine-interface IP and stop the migration with owner details if existing rows conflict. Return typed fixed-address conflicts while dynamic allocation retries another candidate. Keep address changes atomic and claim addresses before publishing DNS names so concurrent writers cannot leave partial rows or invert constraint lock order. Stage admin-network remaps before applying them so overlapping updates remain valid under the new unique key. This supports NVIDIA#4150 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
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
`@crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sql`:
- Around line 135-138: Replace the masklen-based guard with an address-capacity
check in the migration’s remap procedure, declaring v_address_count and placing
the new logic after v_num_reserved normalization. Count existing
machine-interface addresses for admin_network in the previous prefix’s address
family, calculate new_range capacity using its family and prefix length, and
return with a notice when capacity is less than v_num_reserved plus
v_address_count. Preserve the existing remap flow when capacity is sufficient.
🪄 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: f05ed12f-195c-46ec-bed2-7b9a74bd4556
📒 Files selected for processing (8)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/tests/machine_discovery.rscrates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sqlcrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/api-core/src/tests/machine_discovery.rs
- crates/api-core/src/dhcp/v6.rs
- crates/api-core/src/dhcp/expire.rs
- crates/api-db/src/network_segment.rs
- crates/api-db/src/machine_interface_address.rs
- crates/api-db/src/machine_interface/tests.rs
- crates/api-db/src/machine_interface.rs
| IF NOT masklen(new_range) <= masklen(v_previous_prefix) THEN | ||
| RAISE NOTICE 'New range is smaller than old range, aborting because it cannot be guaranteed to have enough space'; | ||
| RETURN; | ||
| END IF; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bound the remap by usable address count, not by masklen.
masklen is only comparable inside one address family. Lines 121-127 explicitly allow a cross-family replacement of a single prefix, so this guard can pass while the new prefix is far smaller. Example: previous prefix 2001:db8::/64, new_range 198.18.0.0/29 satisfies 29 <= 64.
When capacity is insufficient, line 260-263 still evaluates host(new_range + offset)::inet. PostgreSQL only rejects an overflow of the whole address space, so the procedure writes addresses outside the segment prefix and commits them. Gate the remap on the number of addresses to move instead.
Place the guard after the v_num_reserved normalization at lines 146-149.
🛡️ Proposed capacity guard
- IF NOT masklen(new_range) <= masklen(v_previous_prefix) THEN
- RAISE NOTICE 'New range is smaller than old range, aborting because it cannot be guaranteed to have enough space';
- RETURN;
- END IF;Declare v_address_count bigint; and add this block after line 149:
SELECT count(*)
INTO v_address_count
FROM machine_interface_addresses mia
JOIN machine_interfaces mi ON mi.id = mia.interface_id
WHERE mi.segment_id = admin_network
AND family(mia.address) = family(v_previous_prefix);
IF 2::numeric ^ (
(CASE family(new_range) WHEN 4 THEN 32 ELSE 128 END) - masklen(new_range)
) < v_num_reserved + v_address_count THEN
RAISE NOTICE 'New range holds fewer than % reserved plus % remapped addresses, aborting',
v_num_reserved, v_address_count;
RETURN;
END IF;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IF NOT masklen(new_range) <= masklen(v_previous_prefix) THEN | |
| RAISE NOTICE 'New range is smaller than old range, aborting because it cannot be guaranteed to have enough space'; | |
| RETURN; | |
| END IF; | |
| SELECT count(*) | |
| INTO v_address_count | |
| FROM machine_interface_addresses mia | |
| JOIN machine_interfaces mi ON mi.id = mia.interface_id | |
| WHERE mi.segment_id = admin_network | |
| AND family(mia.address) = family(v_previous_prefix); | |
| IF 2::numeric ^ ( | |
| (CASE family(new_range) WHEN 4 THEN 32 ELSE 128 END) - masklen(new_range) | |
| ) < v_num_reserved + v_address_count THEN | |
| RAISE NOTICE 'New range holds fewer than % reserved plus % remapped addresses, aborting', | |
| v_num_reserved, v_address_count; | |
| RETURN; | |
| END IF; |
🤖 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-db/migrations/20260730222152_machine_interface_addresses_unique.sql`
around lines 135 - 138, Replace the masklen-based guard with an address-capacity
check in the migration’s remap procedure, declaring v_address_count and placing
the new logic after v_num_reserved normalization. Count existing
machine-interface addresses for admin_network in the previous prefix’s address
family, calculate new_range capacity using its family and prefix length, and
return with a notice when capacity is less than v_num_reserved plus
v_address_count. Preserve the existing remap flow when capacity is sufficient.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/api-core/src/dhcp/v6.rs (1)
98-106: 📐 Maintainability & Code Quality | 🔵 TrivialThe narrowed TODO is well scoped. Consider tracking it.
The comment now states the remaining gap precisely: a competing static or DHCPv6 write on the same interface family still surfaces a raw database error to the losing caller, because the per-interface family constraint rejects it without a typed conflict. Unlike the allocation paths, SLAAC cannot retry with a different candidate, so this needs the shared transaction policy the comment describes.
Do you want me to open an issue to track the same-interface family replacement policy across SLAAC, DHCP, and static writers?
🤖 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/dhcp/v6.rs` around lines 98 - 106, Track the remaining same-interface family replacement gap as an issue, covering the shared transaction policy across SLAAC, DHCPv6, and static writers and ensuring losing callers receive a typed conflict instead of a raw database error.crates/api-db/src/machine_interface.rs (1)
2396-2404: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
query_scalarfor this single-column lookup.The query selects one column.
query_asforces a one-element tuple and a destructuringmatch.query_scalarremoves both.♻️ Proposed refactor
- let interface_id: Option<(MachineInterfaceId,)> = sqlx::query_as(query) - .bind(remote_ip) - .fetch_optional(&mut *txn) - .await - .map_err(|e| DatabaseError::query(query, e))?; - - match interface_id { - Some((interface_id,)) => find_one(txn, interface_id).await.map(Some), - None => Ok(None), - } + let interface_id: Option<MachineInterfaceId> = sqlx::query_scalar(query) + .bind(remote_ip) + .fetch_optional(&mut *txn) + .await + .map_err(|e| DatabaseError::query(query, e))?; + + match interface_id { + Some(interface_id) => find_one(txn, interface_id).await.map(Some), + None => Ok(None), + }Based on learnings: "when using sqlx to fetch exactly one scalar value from a query ... prefer
sqlx::query_scalar(query).bind(...).fetch_one(txn)oversqlx::query_as(...).query_scalar()is more concise and avoids destructuring a single-element tuple."🤖 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-db/src/machine_interface.rs` around lines 2396 - 2404, Update the single-column lookup in the surrounding machine-interface function to use sqlx::query_scalar with the existing binding and optional fetch, replacing the one-element tuple type and destructuring match. Preserve the current find_one call for a returned interface ID and Ok(None) behavior when no row is found.Source: Learnings
crates/api-db/src/machine_interface/tests.rs (1)
462-518: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared race harness.
Three helpers repeat the same scaffolding: capture the owner backend PID, spawn the contender, report its PID over a
oneshot, callwait_for_transaction_conflict, commit the owner, then await the contender under a 5 s timeout with abort-on-elapse.run_fixed_address_raceat Lines 234-299,run_fixed_dynamic_create_raceat Lines 346-416, and this helper at Lines 462-518 differ only in the owner write and the contender operation.A single harness that accepts the owner setup and the contender closure would remove roughly forty duplicated lines and keep the timeout and abort handling in one place.
🤖 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-db/src/machine_interface/tests.rs` around lines 462 - 518, Extract the repeated transaction-race scaffolding from run_fixed_address_race, run_fixed_dynamic_create_race, and the current helper into one shared harness. Parameterize it with the owner setup and contender operation while retaining backend-PID reporting, wait_for_transaction_conflict, owner commit, the 5-second timeout, and abort-on-timeout behavior; update all three helpers to use the harness.crates/api-core/src/tests/machine_discovery.rs (1)
896-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
DatabaseError::NotFoundError.
find_for_update_by_ipmaps a missing interface toDatabaseError::NotFoundError { .. }. Match this variant so unrelated query failures do not pass the test.🤖 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/tests/machine_discovery.rs` around lines 896 - 901, Update the test around find_for_update_by_ip to assert that the returned error matches DatabaseError::NotFoundError, rather than only checking that an error occurred. Preserve the existing missing-IP lookup and ensure unrelated database errors cause the test to fail.
🤖 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_interface.rs`:
- Around line 3504-3537: The retry handling in allocate_address_for_family must
also treat DatabaseError::TryAgain as a retryable allocation conflict. In the
attempt match around the existing AddressAlreadyInUse(_) branch, match both
errors, roll back fast_txn, and continue selecting a new address set; update the
corresponding sibling site in crates/api-db/src/machine_interface.rs lines
1896-1901 consistently, with no direct change needed there if it is covered by
the shared root-cause logic.
---
Nitpick comments:
In `@crates/api-core/src/dhcp/v6.rs`:
- Around line 98-106: Track the remaining same-interface family replacement gap
as an issue, covering the shared transaction policy across SLAAC, DHCPv6, and
static writers and ensuring losing callers receive a typed conflict instead of a
raw database error.
In `@crates/api-core/src/tests/machine_discovery.rs`:
- Around line 896-901: Update the test around find_for_update_by_ip to assert
that the returned error matches DatabaseError::NotFoundError, rather than only
checking that an error occurred. Preserve the existing missing-IP lookup and
ensure unrelated database errors cause the test to fail.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 2396-2404: Update the single-column lookup in the surrounding
machine-interface function to use sqlx::query_scalar with the existing binding
and optional fetch, replacing the one-element tuple type and destructuring
match. Preserve the current find_one call for a returned interface ID and
Ok(None) behavior when no row is found.
In `@crates/api-db/src/machine_interface/tests.rs`:
- Around line 462-518: Extract the repeated transaction-race scaffolding from
run_fixed_address_race, run_fixed_dynamic_create_race, and the current helper
into one shared harness. Parameterize it with the owner setup and contender
operation while retaining backend-PID reporting, wait_for_transaction_conflict,
owner commit, the 5-second timeout, and abort-on-timeout behavior; update all
three helpers to use the harness.
🪄 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: a99037b3-3c75-45d9-b24b-8b19ae3c8eeb
📒 Files selected for processing (8)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/tests/machine_discovery.rscrates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sqlcrates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rscrates/api-db/src/network_segment.rs
| for _ in 0..ALLOCATION_CONFLICT_MAX_RETRIES { | ||
| let mut fast_txn = Transaction::begin_inner(txn).await?; | ||
| if family == IpAddressFamily::Ipv6 { | ||
| lock_network_segment_exclusive(&mut fast_txn, segment).await?; | ||
| } else { | ||
| lock_network_segment_shared(&mut fast_txn, segment).await?; | ||
| } | ||
|
|
||
| // Nothing allocated (no prefix for the requested family): leave the | ||
| // hostname and domain exactly as they were. | ||
| if allocated_addresses.is_empty() { | ||
| return Ok(allocated_addresses); | ||
| let attempt = | ||
| try_allocate_addresses_for_family(&mut fast_txn, interface_id, &family_segment).await; | ||
|
|
||
| match attempt { | ||
| Ok(allocated_addresses) => { | ||
| fast_txn.commit().await?; | ||
| sync_hostname_after_address_assignment( | ||
| txn, | ||
| interface_id, | ||
| segment.config.subdomain_id, | ||
| ) | ||
| .await?; | ||
| return Ok(allocated_addresses); | ||
| } | ||
| Err(DatabaseError::AddressAlreadyInUse(_)) => { | ||
| // A concurrent allocator or static writer took the selected | ||
| // address. The savepoint also removes any earlier address rows | ||
| // from this attempt before we select again. | ||
| fast_txn.rollback().await?; | ||
| } | ||
| Err(error) => { | ||
| fast_txn.rollback().await?; | ||
| return Err(error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether TryAgain can propagate out of the shared allocation helpers.
set -euo pipefail
ast-grep outline crates/api-db/src/machine_interface.rs --items all --type function \
--match 'allocate_next_ip_with_retry|allocate_addresses_from_segment|try_create_slow_path|try_allocate_addresses_for_family'
ast-grep run --lang rust \
--pattern 'async fn allocate_next_ip_with_retry($$$) -> $R { $$$ }' \
crates/api-db/src/machine_interface.rs
rg -n -C4 'TryAgain' crates/api-db/srcRepository: NVIDIA/infra-controller
Length of output: 7472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- create_fast_path and create_slow_path callers ---'
sed -n '1680,1775p' crates/api-db/src/machine_interface.rs
sed -n '1845,2045p' crates/api-db/src/machine_interface.rs
printf '%s\n' '--- family allocation loop and helper ---'
sed -n '3460,3605p' crates/api-db/src/machine_interface.rs
printf '%s\n' '--- allocation error construction and helper call graph ---'
rg -n -C6 'allocate_addresses_from_segment|allocate_next_ip_with_retry|try_allocate_addresses_for_family|AddressAlreadyInUse|DatabaseError::TryAgain' crates/api-db/src/machine_interface.rs crates/api-db/src/lib.rsRepository: NVIDIA/infra-controller
Length of output: 32914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- create_slow_path allocation body ---'
sed -n '1890,2035p' crates/api-db/src/machine_interface.rs
printf '%s\n' '--- family allocation body ---'
sed -n '3500,3585p' crates/api-db/src/machine_interface.rs
printf '%s\n' '--- all TryAgain returns in the relevant helper region ---'
rg -n -C8 'TryAgain|allocate_next_ip_with_retry|allocate_addresses_from_segment' crates/api-db/src/machine_interface.rsRepository: NVIDIA/infra-controller
Length of output: 13950
Retry DatabaseError::TryAgain in allocate_address_for_family. For IPv4, try_allocate_addresses_for_family calls allocate_next_ip_with_retry, which returns TryAgain when all candidate locks fail. Match DatabaseError::TryAgain | DatabaseError::AddressAlreadyInUse(_) so the operation rolls back and selects a new address set.
📍 Affects 1 file
crates/api-db/src/machine_interface.rs#L3504-L3537(this comment)crates/api-db/src/machine_interface.rs#L1896-L1901
🤖 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-db/src/machine_interface.rs` around lines 3504 - 3537, The retry
handling in allocate_address_for_family must also treat DatabaseError::TryAgain
as a retryable allocation conflict. In the attempt match around the existing
AddressAlreadyInUse(_) branch, match both errors, roll back fast_txn, and
continue selecting a new address set; update the corresponding sibling site in
crates/api-db/src/machine_interface.rs lines 1896-1901 consistently, with no
direct change needed there if it is covered by the shared root-cause logic.
|
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. |
1 participant
Machine-interface IP allocation previously relied on separate safeguards in each path. Dynamic IPv4 used candidate locks, IPv6 and whole-prefix allocation used segment locks, and fixed or SLAAC writers checked existing rows before inserting. Those paths did not share one database ownership boundary, so writers following different paths could claim the same address concurrently.
Enforce one database owner per machine-interface IP and stop the migration with owner details if existing rows conflict.
Return typed fixed-address conflicts while dynamic allocation retries another candidate. Keep address changes atomic and claim addresses before publishing DNS names so concurrent writers cannot leave partial rows or invert constraint lock order.
Stage admin-network remaps before applying them so overlapping updates remain valid under the new unique key.
Related issues
This supports #4150
Type of Change
Breaking Changes
No API or configuration changes are required.
Testing
Focused database tests covered fixed/fixed, fixed/DHCP, whole-prefix, family-reallocation, IPv4, IPv6, address/FQDN lock ordering, historical masks, duplicate migration data, and admin-network remaps.
The following checks passed:
cargo test -p carbide-api-db machine_interface:: --lib -- --test-threads=1cargo test -p carbide-api-db machine_interface_address:: --lib -- --test-threads=1cargo test -p carbide-api-core test_discovery_ip_lookup_rejects_missing_mapping -- --nocapturecargo test -p carbide-api-db migration_versions_are_unique --libscripts/check-migration-filenames.sh crates/api-db/migrations/20260730222152_machine_interface_addresses_unique.sqlcargo make format-nightlycargo make clippycargo make carbide-lintsAdditional Notes
The migration takes an
ACCESS EXCLUSIVElock while auditing the address table and adding the constraint. If historical duplicate host addresses exist, it stops without choosing or deleting an owner and reports the total count plus up to 20 owner groups. Those rows must be resolved before retrying the migration.Single-owner rows with non-host masks are normalized to
/32or/128before the constraint is added. Sites without duplicate ownership require no operator changes.