Never delete an interrupted-apply module without attempting a destroy

[jgruberf5]

Description

Stack destroy silently orphaned cloud resources on one specific path. BUG-008 recovery handles modules stuck in a transitional status with no live task; for applying with no terminal task on record — the worker died mid-run — it reset the module to not_initialized. That status routed it into modules_to_delete, which deleted the row with no destroy attempt at all.

But tofu apply creates IAM roles, VPCs and the like well before it finishes. A module that reached applying may already own resources, and Forge holds the only record of them. Deleting the row made them invisible: they kept existing, and the next deploy hit EntityAlreadyExists.

Fixes / Implements: #30

Honesty about scope

#30 is one line long, and I could not reproduce its stated symptom ("stack-delete tears down K8s but not AWS-provider modules") — there is no provider conditional anywhere in destroy selection. What I found instead is a concrete, testable path that produces exactly the outcome the issue describes: orphaned AWS/IAM resources + EntityAlreadyExists on redeploy. It's the candidate I flagged on the issue as "the one to look at first."

I'm confident this path is real and worth fixing on its own merits. I'm not certain it's the only cause behind the original report — if the reporter's case was a tofu destroy that partially failed but was still marked destroyed, this PR doesn't touch that. Worth keeping in mind before treating #30 as fully closed.

The two changes

1. Recovery lands an interrupted apply in apply_failed (and an interrupted destroy in destroy_failed), so the module is queued for a real destroy. Destroy is idempotent if nothing was created; the row-delete is unrecoverable if something was. initializing/planning still recover to not_initialized — nothing is applied during those phases, so the fast delete path stays. Also PLANNING + FAILED task now recovers to plan_failed rather than collapsing into init_failed.

2. The delete-vs-destroy classification uses NO_INFRA_STATUSES — the same vocabulary the project-DELETE guard adopted in Refuse project deletion while modules still own cloud resources #129 — instead of a hand-rolled list, and fails closed: anything not positively known to have no infrastructure gets a destroy attempt rather than a row delete. (Self-review found this closes a second orphan path too: the generic failed status fell into the old else and was deleted untried.) This file already imported NO_INFRA_STATUSES for _execute_stack_destroy; the two sites had drifted apart in the dangerous direction.

---

Architectural Decision Record (ADR)

• N/A (Bug fix or minor docs tweak).

---

Type of Change

• Bug fix (non-breaking change fixing an issue)

Behaviour change to be aware of: a stack destroy that previously "succeeded instantly" on an interrupted-apply module (by deleting it) will now actually run a destroy task for it. If that destroy fails — e.g. the workspace is gone — the stack destroy reports the failure instead of quietly succeeding. That's the fix working, but it will look like a new failure to anyone who had this happen before.

---

Verification & Testing

Three tests added to backend/tests/component/test_stack_deployment_service.py (95 passed in file):

• test_destroy_does_not_orphan_an_interrupted_apply — applying, no Task row: recovered to apply_failed, queued for destroy, row still present;
• the symmetric interrupted-destroy case → destroy_failed, queued;
• initializing/planning interrupted → still deleted directly, no destroy dispatched (the fast path is preserved).

Verified non-vacuous: both orphan tests fail against the unpatched service and pass with it; the init/plan control passes both ways. I also wrote and then deleted a fourth test that only asserted "row is gone" — that was true before the fix too, so it proved nothing.

Regression sweep on stack/destroy/project_service/janitor: 530 passed, 2 failed — both pre-existing test_cli_smoke.py failures from the known baseline. ruff clean.

Environment validation needed

Yes — the interesting case needs a real cloud account. One thing to confirm first, because the whole argument rests on it and there is no tofu binary in any local image to check: tofu destroy against a workspace with no state must exit 0 ("No objects need to be destroyed"). That is documented behaviour and CP-008 already relies on it, but if it were ever false, an interrupted-apply module would make the stack destroy fail where it previously succeeded by deleting the row. Then, against AWS:

1. start an apply on a stack containing an IAM-creating module, docker kill the celery worker once tofu has begun creating resources, then destroy the stack — confirm the module goes to apply_failed, a destroy task runs, and the IAM resources are actually removed;
2. redeploy the same stack — no EntityAlreadyExists (this is the reported symptom);
3. a stack with only not_initialized/planned modules still destroys instantly with no destroy tasks.

If step 2 still throws EntityAlreadyExists after this, that would tell us the reporter's case is a different path (likely partial tofu destroy failure), and #30 should be reopened with that detail.

---

Checklist

• My code follows the project's code style and formatting guidelines.
• I have updated documentation where necessary — the recovery-table comment explains why applying may own resources, since "no terminal task → not_initialized" reads as reasonable until you know that.
• N/A — no route/schema changes.

[mwiget]

Code Review Summary

Goal: Fixes issue #30 by ensuring modules whose apply was interrupted by worker death are transitioned to apply_failed and queued for a tofu destroy attempt rather than being silently deleted from the DB.

Key Technical Observations:

• Resource Leak Fix: Prevents orphaned cloud infrastructure (e.g., AWS IAM roles, VPCs) from surviving stack deletions when a worker dies mid-apply.
• Fail-Closed Classification: Stack destruction uses NO_INFRA_STATUSES to fail closed; any status not explicitly known to have zero infrastructure receives a destroy task.
• Fast-Path Preservation: Modules in initializing or planning still skip destroy and are deleted directly since no cloud resources exist.
• Test Coverage: 3 component tests in test_stack_deployment_service.py asserting non-orphaning of interrupted applies and destroys.

Verdict: LGTM! Approved.

[jgruberf5]

Self-review

No defects found. Two things surfaced that the PR body didn't say, one of them a genuine second fix hiding in the change.

🟢 Found — this also closes a second orphan path I hadn't noticed

Enumerated how every ModuleStatus value is now classified by the destroy selection:

NO_INFRA (row deleted)      : destroyed, init_failed, initialized, not_initialized, plan_failed, planned
everything else (destroyed) : applied, apply_failed, applying, destroy_failed, destroying, failed, initializing, planning

The interesting one is bare failed. Under the old hand-rolled list it was not in [APPLIED, APPLY_FAILED, DESTROY_FAILED], so it fell into the else → row deleted, no destroy attempt — the same orphaning outcome as the interrupted-apply case, just via a different door. module_state.py calls FAILED "a generic catch-all rarely used", which is exactly why fail-closed is right for it: if it's ever set, we don't know whether infrastructure exists, and an unnecessary destroy is cheap while an unattempted one is not.

(applying/planning/initializing/destroying show as "destroyed" in that table but can't reach classification — BUG-005 returns early on transitional statuses — so that row is theoretical.)

🟢 Verified — composes correctly with #145's janitor

Since #145 merged, the janitor may reset an interrupted apply to apply_failed before a stack destroy ever runs. In that case the module is no longer transitional, so BUG-008 recovery is skipped entirely — and it lands in modules_to_destroy via the fail-closed classification. Same outcome as if this PR's recovery branch had handled it. Confirmed apply_failed, destroy_failed, applied are all outside NO_INFRA_STATUSES. No double-handling, no gap between the two.

Verified non-vacuous, and targeted at the right branch

The orphan test creates zero Task rows (checked: 0 occurrences of TaskModel/make_task in it), so it hits precisely the no-terminal-task branch that was the orphaning path — not the applying + FAILED task branch, which was already correct. Both orphan tests fail against the unpatched service and pass with the fix; the init/plan control passes both ways. I also wrote and deleted a fourth test that only asserted "row is gone" — true before the fix too, so it proved nothing.

The one thing I could not demonstrate

The PR's argument rests on "destroy is idempotent if nothing was created" — i.e. tofu destroy against a workspace with no state exits 0 with "No objects need to be destroyed". That is standard, documented OpenTofu behaviour, and CP-008 in run_destroy already relies on the same assumption for the skip path — but there is no tofu binary in the test image or the local bnk-forge-api image, so I could not run it here. If that assumption were wrong, an interrupted-apply module would now make the stack destroy fail where it previously "succeeded" by deleting the row. I've listed it as the first thing to confirm in environment validation, and I'd rather state that plainly than assert it.

Checked and found correct

• The NO_INFRA_STATUSES import is lazy, matching the existing import in _execute_stack_destroy in the same file (module-level would create the cycle the comment there warns about).
• The two deployment_error messages are each correct for their branch: interrupted apply/destroy says resources may exist and destroy will be attempted; interrupted init/plan keeps the original "stale transitional state recovered" text.
• PLANNING + FAILED task → plan_failed (was init_failed) is a pure precision improvement — both are NO_INFRA, so it changes no routing, only the recorded terminal.

Regression sweep unchanged: 530 passed, 2 pre-existing test_cli_smoke.py failures. ruff clean.

Scope, restated

This fixes a real, reproducible orphan path — two of them, it turns out — that produce exactly #30's symptom. It is still not proof that this was the reporter's cause; the PR body's diagnostic stands (if EntityAlreadyExists recurs after this, it's the partial-tofu destroy path, and #30 should be reopened with that detail).