feat(rack): make maintenance requests atomic and idempotent#3497
feat(rack): make maintenance requests atomic and idempotent#3497tmcroberts97 wants to merge 1 commit into
Conversation
Add a durable rack maintenance request lifecycle, stage request-scoped credentials outside database transactions, and track request completion through the rack state controller. Signed-off-by: Thomas McRoberts <tmcroberts@nvidia.com>
Summary by CodeRabbit
WalkthroughThe PR adds persisted first-class rack maintenance requests, request-scoped credential handling, transactional scheduling through the state controller, and lifecycle updates across rack maintenance, error, completion, and reprovisioning paths. ChangesRack maintenance request lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant ComponentManager
participant Database
participant RackController
participant CredentialStore
API->>ComponentManager: request rack maintenance
ComponentManager->>Database: reserve and prepare request
ComponentManager->>CredentialStore: stage request access token
ComponentManager->>Database: publish ready request and rack request ID
RackController->>Database: mark request running
RackController->>Database: mark request completed or failed
RackController->>CredentialStore: delete request access token
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/handlers/rack.rs (1)
574-591: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winStale pre-check at Lines 585-591 breaks the idempotent retry this PR adds.
request_rack_maintenance_via_state_controller(called below at 778-815) is explicitly designed to returnAlreadyPendingfor a same-scope retry — a success, not an error. But the leftover pre-check at Line 585 unconditionally rejects the call whenever any maintenance is already requested on the rack, regardless of scope, and it runs beforescopeis even parsed (Line 659), so it structurally cannot special-case idempotent retries. Every legitimate retry of an in-flight on-demand maintenance request will now fail here before ever reaching the atomic scheduler that was built to accept it.The eligibility pre-check at Lines 574-583 is also now redundant (it duplicates the identical
Ready | Errorcriterion enforced atomically insiderequest_rack_maintenance_via_state_controller), but it's at least harmless; only Lines 585-591 introduce an actual regression.🐛 Proposed fix: drop the stale non-transactional "already scheduled" rejection
- if rack.config.maintenance_requested.is_some() { - return Err(CarbideError::InvalidArgument(format!( - "On-demand maintenance for rack {} is already scheduled.", - rack_id, - )) - .into()); - } -Also applies to: 778-815
🤖 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/rack.rs` around lines 574 - 591, Remove the unconditional maintenance_requested pre-check in the rack maintenance handler before scope parsing, specifically the rejection around request_rack_maintenance_via_state_controller, so same-scope retries can reach the atomic scheduler and return AlreadyPending successfully. Leave the existing state eligibility check unchanged unless needed by the scheduler flow.
🧹 Nitpick comments (5)
crates/api-core/src/tests/rack_state_controller/handler.rs (1)
1330-1344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing assertion on the intermediate
Ready → Maintenancetransition.Unlike the success test (Line 1253-1259), the first
handle_object_statecall's outcome here is committed but never asserted before the hardcodedfirmware_stateis fed into the second call. This means a regression infirst_maintenance_state(scope)'s mapping from aFirmwareUpgradeactivity toRackMaintenanceState::FirmwareUpgrade { rack_firmware_upgrade: FirmwareUpgradeState::Start }(inready.rs) would go undetected — the test would still "pass" because the second call blindly assumes that sub-state rather than deriving it from the real outcome.♻️ Proposed fix to assert the intermediate transition
let mut outcome = handler .handle_object_state(&rack_id, &mut rack, &RackState::Ready, &mut ctx) .await?; + assert!(matches!( + &outcome, + StateHandlerOutcome::Transition { + next_state: RackState::Maintenance { + maintenance_state: RackMaintenanceState::FirmwareUpgrade { + rack_firmware_upgrade: FirmwareUpgradeState::Start, + }, + }, + .. + } + )); if let Some(txn) = outcome.take_transaction() { txn.commit().await?; }🤖 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/rack_state_controller/handler.rs` around lines 1330 - 1344, In the test around the first handle_object_state call, assert that the Ready → Maintenance outcome contains the expected FirmwareUpgrade maintenance state before committing it. Replace the hardcoded firmware_state input with the maintenance state derived from that asserted intermediate outcome, while preserving the existing transaction commit and subsequent transition coverage.crates/api-db/src/rack_maintenance_request.rs (2)
25-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
find_by_idshould acceptimpl DbReader, not a barePgConnection.This is a plain, non-locking read. As per path instructions, "For read-only database functions, prefer accepting
impl DbReaderso callers can use aPgPoolwithout starting a transaction."crates/api-db/src/rack.rs::find_byalready establishes this pattern in the same crate; forcing&mut PgConnectionhere needlessly requires callers (e.g. an API handler that just wants request status) to hold a connection/transaction.♻️ Proposed refactor to mirror `find_by`
pub async fn find_by_id( - conn: &mut PgConnection, + conn: &mut DB, request_id: Uuid, -) -> DatabaseResult<Option<RackMaintenanceRequest>> { +) -> DatabaseResult<Option<RackMaintenanceRequest>> +where + for<'db> &'db mut DB: DbReader<'db>, +{ let query = "SELECT * FROM rack_maintenance_requests WHERE id = $1"; sqlx::query_as(query) .bind(request_id) - .fetch_optional(conn) + .fetch_optional(&mut *conn) .await .map_err(|error| DatabaseError::new(query, error)) }🤖 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/rack_maintenance_request.rs` around lines 25 - 35, Update rack maintenance request find_by_id to accept the shared DbReader abstraction instead of &mut PgConnection, matching the existing rack.rs find_by pattern. Preserve the query, binding, optional-result behavior, and DatabaseError mapping while allowing callers to provide a PgPool directly.Source: Path instructions
104-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
mark_failed/mark_preparing_failed/mark_cancelledinto thetransition()helper.These three functions duplicate the same
SET status = ..., error_message = $2, updated = now(), completed = now()template, differing only in target status and eligible source states viastatus IN (...). Generalizingtransition()to accept a slice of expected statuses (bound as a Postgres array withstatus = ANY($2)) would remove the duplication and the risk of the three copies drifting apart.♻️ Sketch of a unified helper
async fn transition_from_any( conn: &mut PgConnection, request_id: Uuid, expected: &[RackMaintenanceRequestStatus], next: RackMaintenanceRequestStatus, error_message: Option<&str>, ) -> DatabaseResult<bool> { let terminal = next.is_terminal(); let query = "UPDATE rack_maintenance_requests \ SET status = $3, error_message = $4, updated = now(), \ started = CASE WHEN $3 = 'running' THEN now() ELSE started END, \ completed = CASE WHEN $5 THEN now() ELSE completed END \ WHERE id = $1 AND status = ANY($2)"; sqlx::query(query) .bind(request_id) .bind(expected) .bind(next) .bind(error_message) .bind(terminal) .execute(conn) .await .map(|result| result.rows_affected() == 1) .map_err(|error| DatabaseError::new(query, error)) }🤖 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/rack_maintenance_request.rs` around lines 104 - 178, Consolidate mark_failed, mark_preparing_failed, and mark_cancelled by generalizing transition to accept a slice of expected RackMaintenanceRequestStatus values and use status = ANY($2). Update each wrapper to call the helper with its eligible source statuses, target status, and error message, preserving their existing behavior and removing the duplicated SQL implementations.crates/secrets/src/credentials.rs (1)
1209-1214: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the exact request-scoped Vault path.
This row only checks that the path starts with
racks/, so it would pass if the request ID were omitted or the path segments were reordered. Assert the complete path forUuid::nil(), includingmaintenance/requests/{request_id}/access-token.As per the supplied credential-path contract, this path is shared by request staging and controller credential loading/deletion.
🤖 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/secrets/src/credentials.rs` around lines 1209 - 1214, Update the test scenario for CredentialKey::RackMaintenanceRequestAccessToken to assert the complete Vault path for Uuid::nil(), including the rack identifier and the ordered maintenance/requests/{request_id}/access-token segments. Replace the current prefix-only assertion while preserving the shared path expectation used by request staging and controller credential loading/deletion.crates/rack-controller/src/maintenance.rs (1)
216-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree call sites duplicate the "clear maintenance request in this transaction" sequence.
clear_maintenance_requested_on_error(216-258) already encapsulates "takemaintenance_request_id, clearmaintenance_requested, mark the request, persist config" — but the firmware-failure (1688-1711), NVOS-failure (2013-2029), andCompleted(2528-2536) branches each re-implement the same ~8-line sequence inline, since they need it to land in the same commit as their job-status writes. Any future fix to this bookkeeping (e.g., a new invariant check) now needs to be applied in 4 places.Consider extracting a small helper that operates on an already-open
&mut PgConnection(mirroringmark_maintenance_request_failed/mark_maintenance_request_completed), e.g.:async fn clear_maintenance_requested_in_txn( txn: &mut sqlx::PgConnection, id: &RackId, state: &mut Rack, outcome: RequestOutcome<'_>, // Failed(&cause) | Completed ) -> Result<(), StateHandlerError> { if state.config.maintenance_requested.is_none() && state.config.maintenance_request_id.is_none() { return Ok(()); } let request_id = state.config.maintenance_request_id.take(); state.config.maintenance_requested = None; if let Some(request_id) = request_id { match outcome { RequestOutcome::Failed(cause) => mark_maintenance_request_failed(txn, request_id, cause).await?, RequestOutcome::Completed => mark_maintenance_request_completed(txn, request_id).await?, } } db_rack::update(txn, id, &state.config).await?; Ok(()) }Each of the three call sites would then just call this helper on their already-open
txnbefore persisting their job status.Also applies to: 1688-1711, 2013-2029, 2528-2536
🤖 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/rack-controller/src/maintenance.rs` around lines 216 - 258, Extract the duplicated maintenance-request cleanup into a transaction-scoped helper near clear_maintenance_requested_on_error, accepting the open PgConnection, rack identifier, mutable Rack state, and failed/completed outcome. Centralize taking maintenance_request_id, clearing maintenance_requested, invoking mark_maintenance_request_failed or mark_maintenance_request_completed, and calling db_rack::update. Replace the inline sequences in the firmware-failure, NVOS-failure, and Completed branches, while preserving their existing transaction ordering with job-status writes.
🤖 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/20260714120000_rack_maintenance_requests.sql`:
- Line 12: Update the rack_id foreign key in the maintenance request migration
to prevent rack deletion from removing maintenance history: replace ON DELETE
CASCADE with ON DELETE RESTRICT, preserving terminal request rows when
admin_force_delete_rack hard-deletes a rack.
In `@crates/component-manager/src/component_manager.rs`:
- Around line 133-181: When superseding a stale request in the existing
`Preparing` branch, also delete its request-scoped credential using the key
derived from prepare_rack_id and existing.id. Perform the credential cleanup
after the transaction commits, preserving the current mark_preparing_failed
error handling and avoiding deletion before the request state update is
committed.
In `@crates/rack-controller/src/error_state.rs`:
- Around line 65-72: The Error-state transition around mark_running must allow
an already-Running maintenance request to recover instead of rejecting it as
“not ready to run.” Update the maintenance request handling in the Error state
to treat Running as idempotently active, or otherwise clear/mark the request
appropriately when entering Error, while preserving the Ready-to-Running
transition; add a regression test covering recovery with an existing Running
request.
---
Outside diff comments:
In `@crates/api-core/src/handlers/rack.rs`:
- Around line 574-591: Remove the unconditional maintenance_requested pre-check
in the rack maintenance handler before scope parsing, specifically the rejection
around request_rack_maintenance_via_state_controller, so same-scope retries can
reach the atomic scheduler and return AlreadyPending successfully. Leave the
existing state eligibility check unchanged unless needed by the scheduler flow.
---
Nitpick comments:
In `@crates/api-core/src/tests/rack_state_controller/handler.rs`:
- Around line 1330-1344: In the test around the first handle_object_state call,
assert that the Ready → Maintenance outcome contains the expected
FirmwareUpgrade maintenance state before committing it. Replace the hardcoded
firmware_state input with the maintenance state derived from that asserted
intermediate outcome, while preserving the existing transaction commit and
subsequent transition coverage.
In `@crates/api-db/src/rack_maintenance_request.rs`:
- Around line 25-35: Update rack maintenance request find_by_id to accept the
shared DbReader abstraction instead of &mut PgConnection, matching the existing
rack.rs find_by pattern. Preserve the query, binding, optional-result behavior,
and DatabaseError mapping while allowing callers to provide a PgPool directly.
- Around line 104-178: Consolidate mark_failed, mark_preparing_failed, and
mark_cancelled by generalizing transition to accept a slice of expected
RackMaintenanceRequestStatus values and use status = ANY($2). Update each
wrapper to call the helper with its eligible source statuses, target status, and
error message, preserving their existing behavior and removing the duplicated
SQL implementations.
In `@crates/rack-controller/src/maintenance.rs`:
- Around line 216-258: Extract the duplicated maintenance-request cleanup into a
transaction-scoped helper near clear_maintenance_requested_on_error, accepting
the open PgConnection, rack identifier, mutable Rack state, and failed/completed
outcome. Centralize taking maintenance_request_id, clearing
maintenance_requested, invoking mark_maintenance_request_failed or
mark_maintenance_request_completed, and calling db_rack::update. Replace the
inline sequences in the firmware-failure, NVOS-failure, and Completed branches,
while preserving their existing transaction ordering with job-status writes.
In `@crates/secrets/src/credentials.rs`:
- Around line 1209-1214: Update the test scenario for
CredentialKey::RackMaintenanceRequestAccessToken to assert the complete Vault
path for Uuid::nil(), including the rack identifier and the ordered
maintenance/requests/{request_id}/access-token segments. Replace the current
prefix-only assertion while preserving the shared path expectation used by
request staging and controller credential loading/deletion.
🪄 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: 6dc07a22-c6f4-4adb-bc49-b8433968c7a8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
crates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/tests/rack_state_controller/handler.rscrates/api-db/migrations/20260714120000_rack_maintenance_requests.sqlcrates/api-db/src/lib.rscrates/api-db/src/rack.rscrates/api-db/src/rack_maintenance_request.rscrates/api-model/src/rack.rscrates/component-manager/Cargo.tomlcrates/component-manager/src/component_manager.rscrates/rack-controller/Cargo.tomlcrates/rack-controller/src/error_state.rscrates/rack-controller/src/maintenance.rscrates/rack-controller/src/ready.rscrates/rack/Cargo.tomlcrates/rack/src/firmware_object.rscrates/secrets/Cargo.tomlcrates/secrets/src/credentials.rs
|
|
||
| CREATE TABLE rack_maintenance_requests ( | ||
| id uuid PRIMARY KEY, | ||
| rack_id varchar(64) NOT NULL REFERENCES racks(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether racks are ever hard-deleted (vs. soft-deleted) in the codebase.
rg -nP --type=rust '\bDELETE\s+FROM\s+racks\b' -i
rg -nP --type=rust 'fn\s+\w*force_delete\w*' -C5 crates/api-db crates/api-coreRepository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- migration ---'
cat -n crates/api-db/migrations/20260714120000_rack_maintenance_requests.sql | sed -n '1,220p'
echo '--- rack module outline ---'
ast-grep outline crates/api-db/src/rack.rs --view expanded || true
echo '--- rack soft-delete / delete-related symbols ---'
rg -n -C3 'deleted\s+IS\s+NULL|deleted\b|remove_rack|delete_rack|hard_delete|force_delete|purge|DROP\s+.*racks|DELETE\s+FROM\s+racks' crates/api-db crates/api-core crates/api* || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n crates/api-db/migrations/20260714120000_rack_maintenance_requests.sql | sed -n '1,220p'
echo '---'
ast-grep outline crates/api-db/src/rack.rs --view expanded || true
echo '---'
rg -n -C3 'deleted\s+IS\s+NULL|deleted\b|remove_rack|delete_rack|hard_delete|force_delete|purge|DROP\s+.*racks|DELETE\s+FROM\s+racks' crates/api-db crates/api-core crates/api* || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- hard-delete search across repo ---'
rg -n -C2 -i '\bDELETE\s+FROM\s+racks\b|force_delete|hard_delete|purge.*rack|delete.*rack' . || true
echo '--- rack model / delete semantics ---'
rg -n -C3 -i 'deleted\s+IS\s+NULL|soft[- ]delete|deleted_at|deleted\b' crates/api-db/src crates/api-core/src crates/api-web/src || true
echo '--- migration file excerpt ---'
sed -n '1,120p' crates/api-db/migrations/20260714120000_rack_maintenance_requests.sqlRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- handlers/rack.rs: force delete slice ---'
sed -n '182,260p' crates/api-core/src/handlers/rack.rs
echo '--- api-db/src/rack.rs: delete helpers slice ---'
sed -n '180,260p' crates/api-db/src/rack.rs
echo '--- migration slice ---'
sed -n '1,80p' crates/api-db/migrations/20260714120000_rack_maintenance_requests.sqlRepository: NVIDIA/infra-controller
Length of output: 6741
Do not cascade rack deletions into maintenance history. admin_force_delete_rack hard-deletes racks, so ON DELETE CASCADE will erase the terminal request rows that are supposed to remain for audit. Use ON DELETE RESTRICT or keep the rack row as a tombstone instead.
🤖 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/20260714120000_rack_maintenance_requests.sql` at
line 12, Update the rack_id foreign key in the maintenance request migration to
prevent rack deletion from removing maintenance history: replace ON DELETE
CASCADE with ON DELETE RESTRICT, preserving terminal request rows when
admin_force_delete_rack hard-deletes a rack.
| if let Some(existing) = | ||
| db::rack_maintenance_request::find_active_for_rack_for_update( | ||
| txn.as_mut(), | ||
| &prepare_rack_id, | ||
| ) | ||
| .await | ||
| .map_err(|error| ComponentManagerError::Internal(error.to_string()))? | ||
| { | ||
| if existing.status != RackMaintenanceRequestStatus::Preparing { | ||
| return Ok(Preparation::Complete(if existing.scope == prepare_scope { | ||
| RackMaintenanceRequestOutcome::AlreadyPending { | ||
| request_id: Some(existing.id), | ||
| } | ||
| } else { | ||
| RackMaintenanceRequestOutcome::Busy | ||
| })); | ||
| } | ||
|
|
||
| let preparation_age = | ||
| chrono::Utc::now().signed_duration_since(existing.updated); | ||
| if preparation_age < chrono::Duration::minutes(5) { | ||
| return Ok(Preparation::Complete(if existing.scope == prepare_scope { | ||
| RackMaintenanceRequestOutcome::AlreadyPending { | ||
| request_id: Some(existing.id), | ||
| } | ||
| } else { | ||
| RackMaintenanceRequestOutcome::Busy | ||
| })); | ||
| } | ||
|
|
||
| if !db::rack_maintenance_request::mark_preparing_failed( | ||
| txn.as_mut(), | ||
| existing.id, | ||
| "rack maintenance request preparation timed out", | ||
| ) | ||
| .await | ||
| .map_err(|error| ComponentManagerError::Internal(error.to_string()))? | ||
| { | ||
| return Err(ComponentManagerError::Internal(format!( | ||
| "stale rack maintenance request {} could not be failed", | ||
| existing.id | ||
| ))); | ||
| } | ||
| tracing::warn!( | ||
| rack_id = %prepare_rack_id, | ||
| request_id = %existing.id, | ||
| "Superseding stale rack maintenance request preparation" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the row returned by find_active_for_rack_for_update exposes
# requires_access_token, and whether any other path deletes credentials for
# requests that are superseded/timed-out rather than explicitly failed by
# their own caller.
rg -n -A5 -B5 'requires_access_token' crates/api-db/src/rack_maintenance_request.rs
rg -n 'fn find_active_for_rack_for_update' -A 30 crates/api-db/src/rack_maintenance_request.rsRepository: NVIDIA/infra-controller
Length of output: 2060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant symbols and read the smallest useful slices.
ast-grep outline crates/component-manager/src/component_manager.rs --view expanded || true
ast-grep outline crates/api-db/src/rack_maintenance_request.rs --view expanded || true
rg -n "cleanup_rack_maintenance_access_token|cleanup_credential|rack_maintenance_access_token_key|requires_access_token|mark_preparing_failed|Preparing" crates -g '!**/target/**'Repository: NVIDIA/infra-controller
Length of output: 6581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,240p' crates/component-manager/src/component_manager.rs
printf '\n----\n'
sed -n '340,480p' crates/component-manager/src/component_manager.rs
printf '\n----\n'
sed -n '100,180p' crates/api-db/src/rack_maintenance_request.rsRepository: NVIDIA/infra-controller
Length of output: 14108
Delete the request-scoped credential when superseding a stale Preparing request.
mark_preparing_failed() only updates the row; it leaves the credential at rack_maintenance_access_token_key(rack_id, Some(existing.id)) in place, so a crashed/hung staging attempt can leave a live secret behind indefinitely. Clean it up after commit, or add a terminal-request reaper.
🤖 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/component-manager/src/component_manager.rs` around lines 133 - 181,
When superseding a stale request in the existing `Preparing` branch, also delete
its request-scoped credential using the key derived from prepare_rack_id and
existing.id. Perform the credential cleanup after the transaction commits,
preserving the current mark_preparing_failed error handling and avoiding
deletion before the request state update is committed.
| let mut txn = ctx.services.db_pool.begin().await?; | ||
| if let Some(request_id) = config.maintenance_request_id | ||
| && !db::rack_maintenance_request::mark_running(txn.as_mut(), request_id).await? | ||
| { | ||
| return Err(StateHandlerError::GenericError(eyre::eyre!( | ||
| "rack maintenance request {request_id} is not ready to run" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not reapply the Ready -> Running gate to an already-running request.
mark_running only accepts requests in Ready. Since the Ready handler leaves maintenance_requested set until maintenance completes, a request that is already Running may reach Error with the same scope; this branch then returns “not ready to run” and blocks recovery. Treat Running as idempotently active, or mark the request failed/clear the scope when entering Error, and add a regression test.
As per path instructions, controller logic must handle state-machine transitions and safe recovery from partial failures.
🤖 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/rack-controller/src/error_state.rs` around lines 65 - 72, The
Error-state transition around mark_running must allow an already-Running
maintenance request to recover instead of rejecting it as “not ready to run.”
Update the maintenance request handling in the Error state to treat Running as
idempotently active, or otherwise clear/mark the request appropriately when
entering Error, while preserving the Ready-to-Running transition; add a
regression test covering recovery with an existing Running request.
🔍 Container Scan SummaryNo Grype artifacts were found to aggregate. |
|
@Matthias247 is designing something here which is aiming to solve the same problem this PR is. Closing to avoid duplicated effort, will go for a different approach in #3249. |
1 participant
Add a durable rack maintenance request lifecycle, stage request-scoped credentials outside database transactions, and track request completion through the rack state controller.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes