fix: deploy CLI destroy handoff - #97
Conversation
23423cc to
01e0653
Compare
Greptile SummaryThis PR fixes the deploy CLI destroy flow so that the setup-teardown actor waits for the runtime manager to finish live-resource cleanup before acquiring the teardown lock, preventing config-validation failures when the runtime compute backend context is absent. It also surfaces already-deleted deployments as a clean success rather than an error.
Confidence Score: 4/5Safe to merge — the ownership boundary fix is correct and the new early-exit path for already-deleted deployments is well-guarded. The core logic of restricting setup-teardown acquisition to only teardown-owned statuses is sound, and the AlreadyDeleted fast-path is cleanly handled. Two minor concerns: the 404/not-found detection uses string matching which could theoretically mask a real error, and the retry log message conflates lock-contention waits with runtime-cleanup waits, making live debugging harder. Neither is a correctness issue in the happy or expected failure paths. crates/alien-deployment/src/manager_api_transport.rs — the 404 string-match heuristic and the shared log message for two distinct wait reasons.
|
| Filename | Overview |
|---|---|
| crates/alien-deployment/src/manager_api_transport.rs | Rewrites acquire_setup_delete_deployment into a bespoke polling loop (up to 45 min) that waits for the runtime manager to hand off before the setup-teardown actor acquires. Adds SetupDeleteAcquireOutcome enum. Minor: 404 detection relies on string matching, and the retry log message is misleading under lock-contention vs runtime-wait conditions. |
| crates/alien-deploy-cli/src/commands/up.rs | push_deletion updated to handle SetupDeleteAcquireOutcome::AlreadyDeleted as a successful early return, avoiding any further teardown work when the deployment record was already cleaned up by the runtime manager. |
| Cargo.lock | Blanket patch version bump (1.10.0 → 1.10.1) across all alien-* workspace crates, consistent with the code changes in this PR. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as deploy-cli (push_deletion)
participant AT as acquire_setup_delete_deployment
participant MGR as Manager API
participant RM as Runtime Manager
CLI->>AT: call (deployment_id, session)
loop up to 45 min (1350 x 2s)
AT->>MGR: "acquire(statuses=[teardown-required, teardown-failed])"
alt lock granted
MGR-->>AT: deployment returned
AT-->>CLI: Ok(Acquired)
else lock not granted
MGR-->>AT: empty list
AT->>MGR: get_deployment(id)
alt 404 / not found
MGR-->>AT: error
AT-->>CLI: Ok(AlreadyDeleted)
else "status = deleted"
MGR-->>AT: "status=deleted"
AT-->>CLI: Ok(AlreadyDeleted)
else "status = delete-failed"
MGR-->>AT: "status=delete-failed"
AT-->>CLI: Err(user must resolve runtime failure)
else "status = delete-pending / deleting"
Note over AT,RM: Runtime Manager owns this phase
AT->>AT: sleep 2s, retry
else "status = teardown-required/teardown-failed (contention)"
AT->>AT: sleep 2s, retry acquire
end
end
end
AT-->>CLI: Err(timeout)
CLI->>CLI: "if AlreadyDeleted -> success, return"
CLI->>MGR: get_deployment (re-fetch under lock)
CLI->>CLI: run_setup_teardown_after_handoff
CLI->>MGR: release_deployment(session)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CLI as deploy-cli (push_deletion)
participant AT as acquire_setup_delete_deployment
participant MGR as Manager API
participant RM as Runtime Manager
CLI->>AT: call (deployment_id, session)
loop up to 45 min (1350 x 2s)
AT->>MGR: "acquire(statuses=[teardown-required, teardown-failed])"
alt lock granted
MGR-->>AT: deployment returned
AT-->>CLI: Ok(Acquired)
else lock not granted
MGR-->>AT: empty list
AT->>MGR: get_deployment(id)
alt 404 / not found
MGR-->>AT: error
AT-->>CLI: Ok(AlreadyDeleted)
else "status = deleted"
MGR-->>AT: "status=deleted"
AT-->>CLI: Ok(AlreadyDeleted)
else "status = delete-failed"
MGR-->>AT: "status=delete-failed"
AT-->>CLI: Err(user must resolve runtime failure)
else "status = delete-pending / deleting"
Note over AT,RM: Runtime Manager owns this phase
AT->>AT: sleep 2s, retry
else "status = teardown-required/teardown-failed (contention)"
AT->>AT: sleep 2s, retry acquire
end
end
end
AT-->>CLI: Err(timeout)
CLI->>CLI: "if AlreadyDeleted -> success, return"
CLI->>MGR: get_deployment (re-fetch under lock)
CLI->>CLI: run_setup_teardown_after_handoff
CLI->>MGR: release_deployment(session)
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/alien-deployment/src/manager_api_transport.rs:293-298
The "Waiting for runtime cleanup handoff" log message is accurate when status is `delete-pending` or `deleting`, but when the deployment is already `teardown-required` or `teardown-failed` and the acquire still returned empty, the real reason for waiting is **lock contention** — another session holds the lock. Logging the same message in both cases makes it harder to diagnose whether the delay is upstream (runtime manager) or local (lock held by another CLI session). Splitting the log message on status would make this immediately clear in traces.
```suggestion
let waiting_reason = match status.as_str() {
"teardown-required" | "teardown-failed" => "lock contention on setup teardown",
_ => "runtime cleanup handoff before setup teardown",
};
info!(
attempt = attempt,
max = MAX_SETUP_DELETE_ACQUIRE_ATTEMPTS,
status = %status,
reason = waiting_reason,
"Waiting for setup teardown acquire"
);
```
### Issue 2 of 2
crates/alien-deployment/src/manager_api_transport.rs:262-264
Detecting a "not found" deployment via substring matching on the error message string is fragile. A message that happens to contain `"404"` or `"not found"` for a completely different reason (e.g., a misconfigured route or an unrelated resource) would silently be treated as a successful deletion, masking real failures. If the SDK exposes a typed HTTP-status accessor, prefer that; otherwise, at minimum, match on a more specific substring to reduce the false-positive surface.
```suggestion
if message.contains("404")
|| message.to_lowercase().contains("deployment not found")
{
return Ok(SetupDeleteAcquireOutcome::AlreadyDeleted);
}
```
Reviews (1): Last reviewed commit: "fix: deploy CLI destroy handoff" | Re-trigger Greptile
| info!( | ||
| attempt = attempt, | ||
| max = MAX_SETUP_DELETE_ACQUIRE_ATTEMPTS, | ||
| status = %status, | ||
| "Waiting for runtime cleanup handoff before setup teardown" | ||
| ); |
There was a problem hiding this comment.
The "Waiting for runtime cleanup handoff" log message is accurate when status is
delete-pending or deleting, but when the deployment is already teardown-required or teardown-failed and the acquire still returned empty, the real reason for waiting is lock contention — another session holds the lock. Logging the same message in both cases makes it harder to diagnose whether the delay is upstream (runtime manager) or local (lock held by another CLI session). Splitting the log message on status would make this immediately clear in traces.
| info!( | |
| attempt = attempt, | |
| max = MAX_SETUP_DELETE_ACQUIRE_ATTEMPTS, | |
| status = %status, | |
| "Waiting for runtime cleanup handoff before setup teardown" | |
| ); | |
| let waiting_reason = match status.as_str() { | |
| "teardown-required" | "teardown-failed" => "lock contention on setup teardown", | |
| _ => "runtime cleanup handoff before setup teardown", | |
| }; | |
| info!( | |
| attempt = attempt, | |
| max = MAX_SETUP_DELETE_ACQUIRE_ATTEMPTS, | |
| status = %status, | |
| reason = waiting_reason, | |
| "Waiting for setup teardown acquire" | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/alien-deployment/src/manager_api_transport.rs
Line: 293-298
Comment:
The "Waiting for runtime cleanup handoff" log message is accurate when status is `delete-pending` or `deleting`, but when the deployment is already `teardown-required` or `teardown-failed` and the acquire still returned empty, the real reason for waiting is **lock contention** — another session holds the lock. Logging the same message in both cases makes it harder to diagnose whether the delay is upstream (runtime manager) or local (lock held by another CLI session). Splitting the log message on status would make this immediately clear in traces.
```suggestion
let waiting_reason = match status.as_str() {
"teardown-required" | "teardown-failed" => "lock contention on setup teardown",
_ => "runtime cleanup handoff before setup teardown",
};
info!(
attempt = attempt,
max = MAX_SETUP_DELETE_ACQUIRE_ATTEMPTS,
status = %status,
reason = waiting_reason,
"Waiting for setup teardown acquire"
);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if message.contains("404") || message.contains("not found") { | ||
| return Ok(SetupDeleteAcquireOutcome::AlreadyDeleted); | ||
| } |
There was a problem hiding this comment.
Detecting a "not found" deployment via substring matching on the error message string is fragile. A message that happens to contain
"404" or "not found" for a completely different reason (e.g., a misconfigured route or an unrelated resource) would silently be treated as a successful deletion, masking real failures. If the SDK exposes a typed HTTP-status accessor, prefer that; otherwise, at minimum, match on a more specific substring to reduce the false-positive surface.
| if message.contains("404") || message.contains("not found") { | |
| return Ok(SetupDeleteAcquireOutcome::AlreadyDeleted); | |
| } | |
| if message.contains("404") | |
| || message.to_lowercase().contains("deployment not found") | |
| { | |
| return Ok(SetupDeleteAcquireOutcome::AlreadyDeleted); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/alien-deployment/src/manager_api_transport.rs
Line: 262-264
Comment:
Detecting a "not found" deployment via substring matching on the error message string is fragile. A message that happens to contain `"404"` or `"not found"` for a completely different reason (e.g., a misconfigured route or an unrelated resource) would silently be treated as a successful deletion, masking real failures. If the SDK exposes a typed HTTP-status accessor, prefer that; otherwise, at minimum, match on a more specific substring to reduce the false-positive surface.
```suggestion
if message.contains("404")
|| message.to_lowercase().contains("deployment not found")
{
return Ok(SetupDeleteAcquireOutcome::AlreadyDeleted);
}
```
How can I resolve this? If you propose a fix, please make it concise.## Background Machine join renders configuration from a release manifest. TOML itself accepts values such as both `true` and `"true"`, but Horizond requires specific scalar types. Replacing the active file before the actual consumer validates it can turn a repair or reconfiguration into a service crash loop. ## Changes - preserve manifest literal types when rendering TOML - write configuration through a durable same-directory candidate file - optionally run the bundle-declared configuration validator against that candidate - atomically replace the active config only after validation succeeds - leave the previous active config untouched and remove the candidate on rejection - recover safely from stale candidate files left by an interrupted join The validator is optional for backward compatibility with existing bundle manifests. Horizon PR #97 adds `horizond validate-config <path>` and publishes it in new manifests. ## Validation - `cargo test -p alien-deploy-cli machine_config_` (4 passed) - rejection test proves the old active file remains byte-for-byte unchanged
Summary
Root cause
Production BYOC sims showed the deployer/setup-teardown actor acquiring
delete-pending/deletingruntime states. That actor does not have the runtime manager compute backend context, so platform runtime resources failed config validation during deletion instead of letting the runtime manager clean up live resources first.Correct ownership is: runtime manager owns runtime deletion through live-resource cleanup, then setup teardown owns
teardown-required/teardown-failed.Linear note: attempted to create the required Linear issue from Codex, but the Linear MCP in this session returned OAuth authorization required.
Validation
cargo fmt --package alien-deployment --package alien-deploy-clicargo check -p alien-deploy-cli -p alien-deploymentcargo check -p alien-deploy-cli -p alien-deployment --locked