feat(oabctl): programmatic delete API with explicit target contract by chaodu-agent · Pull Request #1415 · openabdev/openab

chaodu-agent · 2026-07-16T04:16:12Z

Summary

Designs and implements the programmatic delete contract that the #1404 review intentionally deferred ("until their programmatic contracts are designed independently"). This PR is stacked on refactor/oabctl-lib.

let report = oabctl::delete_services(
    &aws,
    &[DeleteTarget::new("prod", "nest-my-oab")],
    &DeleteOptions::new("oab"),
).await?;

The API is symmetric with apply_manifests:

DeleteOptions requires an explicit cluster; the library path never reads ~/.oabctl/config.toml.
Bucket resolution shares apply's chain: explicit option → OAB_CONTROL_PLANE_BUCKET → oab-control-plane-{account}.
Contract guards run before AWS calls, including rejection of delimiter-ambiguous namespaces.
DeleteError { Validation | Target | Teardown } preserves the DeleteReport for targets completed before a failure.
Exact AWS identities are checkpointed before mutation so teardown failures can be retried without name-only cleanup.
Programmatic calls suppress human-readable progress while the CLI retains its existing rendering.

Review Contract

Contract revision: RC-1

Goal

Provide a public, in-process Rust API that safely deletes one or more explicitly identified OAB ECS deployments without reading CLI home configuration. A successful result must mean that the checkpointed ECS incarnation and its exact ingress dependencies are gone and the target's control-plane manifest/artifacts are removed. Ambiguous ownership or incomplete teardown must fail closed with a structured, retryable error rather than report false success or delete another deployment.

Non-goals

Providing linearizable coordination between concurrent apply_manifests and delete_services calls for the same logical target; callers must serialize same-target mutations as described under Accepted Residual Risks.
Performing name-only orphan cleanup when neither a live ECS service nor a matching durable delete checkpoint can establish ownership.
Removing platform-side webhooks, Telegram disclosure state, Secrets Manager secrets, shared bootstrap infrastructure, logs, or unrelated task-definition revisions.
Redesigning the stacked apply_manifests API, changing existing CLI UX, or introducing a general distributed workflow engine.
Parallelizing multi-target deletion or continuing later targets after the first target failure.

Accepted Residual Risks

Same-target apply/delete concurrency is unsupported in RC-1. The caller must serialize mutations for the same (AWS account, Region, control-plane bucket, cluster, namespace, name) tuple. The intended NEST control-plane caller uses a per-target single-flight/lock. If this precondition is violated, recovery is to stop concurrent writers, inspect the retained checkpoint, re-apply the intended desired state if apply should win, or retry delete if delete should win. A shared generation fence remains a follow-up.
AWS control-plane eventual consistency can require retries. Ambiguous or not-yet-converged ECS responses fail closed, dependent cleanup does not start, and the durable checkpoint remains for retry.
An already-orphaned deployment without a checkpoint is not auto-cleaned. This can leave resources behind, but avoids destructive name-only guessing; recovery is explicit operator inspection/remediation.
Batch deletion is sequential and stops at the first failed target. The structured error retains prior completed targets and identifies the failed target, so the caller can retry deterministically.

Acceptance Criteria

The crate publicly exports delete_services, DeleteTarget, DeleteOptions, DeletedService, DeleteReport, DeleteError, and DeleteErrorKind without changing the existing CLI command surface.
Before any AWS call, the API rejects an empty target set, a blank cluster, blank namespace/name components, and any logical target that cannot map unambiguously to one physical delete identity. Distinct accepted (namespace, name) pairs must not resolve to the same ECS/API/Cloud Map identity.
The library requires an explicit cluster, never reads ~/.oabctl/config.toml, and resolves the control-plane bucket using the same explicit option → environment → caller-account chain as apply.
Before mutating ECS, delete persists a checkpoint bound to the exact target and caller boundary: namespace/name, bucket, partition/account/Region, requested and canonical cluster identity, ECS service ARN and createdAt incarnation, and exact registry/API identifiers when ingress exists.
Initial missing, empty-success, mixed-failure, duplicate, or otherwise ambiguous AWS identity responses fail closed. Retries may continue only from a matching checkpoint, and dependent ingress/S3 cleanup must not begin until the checkpointed ECS incarnation is unambiguously absent.
Delete has no first-name-match or name-only fallback for ECS, Cloud Map, or API Gateway. It must not remove resources belonging to a different cluster, account, Region, logical target, or recreated ECS incarnation.
A successful DeleteReport contains only fully completed targets. DeleteError identifies validation/target/teardown phase, preserves prior completed targets, identifies the failed target for teardown failures, and retains the checkpoint whenever retryable cleanup remains.
Programmatic apply/delete calls emit no human-readable progress to process-global stdout/stderr; existing CLI output and aggregate CLI failure behavior remain unchanged.
Public Rustdocs or user documentation explicitly state the same-target serialization precondition and the no-orphan-cleanup behavior so callers can satisfy the accepted-risk mitigations.
Focused tests cover pre-AWS validation, logical-identity collision prevention, checkpoint matching and fail-closed AWS response classification, incarnation mismatch, drain timeout, partial-report preservation, and retry behavior. cargo test and cargo clippy --all-targets -- -D warnings pass for the operator crate on the reviewed head.

Follow-ups

Add a shared target generation/lease fence plus conditional S3 writes so apply and delete can safely arbitrate concurrent same-target mutations instead of requiring caller serialization.
Introduce a durable opaque deployment ID or reversible canonical resource-name encoding across apply/delete, beyond the minimum collision prevention required by RC-1.
Design an explicit, auditable orphan-recovery API that can discover and remove resources without weakening the default fail-closed path.
Add optional bounded parallelism and a collect-all-results batch mode if control-plane scale requires it.
Decide separately whether secret deletion, webhook removal, disclosure cleanup, and broader tenant deprovisioning belong in this crate or in the control plane.

CLI

No intended behavior change: oabctl delete <name> and oabctl delete -f <path> retain config-file cluster resolution, printed progress, and aggregate failure reporting.

Motivation

NEST's manager needs /delete for tenant deprovisioning. Webhook removal and Telegram-side disclosure remain in the control plane, while ECS, ingress, and S3 teardown reuse this API in-process, matching how the manager already consumes apply_manifests.

Validation Evidence

Current implementation head: 54bd8f28eda19c3bd209789be89c44740d3a55f1.
RC-1 fixes reject delimiter-ambiguous delete namespaces before AWS, preserve partial reports, and fail closed before destructive mutation when an apply ingress-teardown checkpoint is pending.
Exact-head Review Contract validation and operator CI passed; the operator job completed cargo check, cargo clippy, cargo test, and cargo build successfully.
Author-reported real-AWS E2E on the prior exact-identity head covered initial fail-closed drain behavior, checkpointed retry to full teardown, already-gone refusal without a checkpoint, and post-success checkpoint removal.
Focused re-review of 1bd61e8..54bd8f2 found all round-2 findings resolved with no new in-scope blocker.

Designs the delete contract deferred by the lib-split review, symmetric with apply_manifests: - delete_services(&SdkConfig, &[DeleteTarget], &DeleteOptions) -> Result<DeleteReport, DeleteError> - DeleteOptions requires an explicit cluster and shares apply's bucket resolution chain (explicit -> OAB_CONTROL_PLANE_BUCKET -> account) so delete always cleans the bucket apply wrote to; never reads CLI config - Contract guards before any AWS call: non-empty target set, non-empty cluster, non-blank namespace/name (regression-tested without AWS) - Typed DeleteError {Validation, Target, Teardown} preserving the report of targets completed before a failure; teardown remains resumable - Best-effort ingress skips surface in DeletedService.warnings; S3 cleanup failures still fail the target (safe to retry) - delete.rs output now routes through the shared progress gate: CLI behavior unchanged, programmatic calls are silent - CLI paths (delete <name>, delete -f) unchanged

- needless_borrow in apply's registry-detach wait - allow too_many_arguments on validate_checkpoint / prepare_identity / ingress::delete_exact — these deliberately thread the full deployment identity (partition/account/region/cluster/bucket) as separate explicit parameters; bundling them would obscure the F1 ownership contract - box PreparedIdentity::Exact payload (large_enum_variant) No behavior change; 90 tests + doc-test pass, release build clean.

… refactor/oabctl-delete-api # Conflicts: # operator/src/apply.rs # operator/src/delete.rs

- apply: redundant_guards — match empty failures with a slice pattern - ingress: cloud_map_service_id_from_arn became test-only after the exact-identity merge (all production paths use the boundary-pinned parser); mark it #[cfg(test)] with a note instead of carrying dead code Verified with the CI toolchain (rustc 1.97.1): clippy -D warnings clean, 96 tests + doc-test pass.

chaodu-agent

Important

CHANGES REQUESTED ⚠️ — The logical delete target is non-unique, and post-ECS cleanup is not fenced against a concurrent apply.

Consolidated review: #1415 (comment)

GitHub event: COMMENT — self-review delivery only; this is not an approval.

chaodu-agent · 2026-07-17T03:48:32Z

+        )));
+    }
+    for target in targets {
+        if target.namespace.trim().is_empty() || target.name.trim().is_empty() {


🔴 F1 — Make the target-to-resource identity injective

Blank-only validation allows distinct targets such as prod/team-bot and prod-team/bot to derive the same ECS, API Gateway, and Cloud Map names. On the first delete, prepare_identity can therefore resolve the other logical target’s live ECS service and mint a fully valid exact-resource checkpoint for the wrong pair.

Requested change: use a reversible/injective physical identity (or immutable logical-identity metadata verified before checkpoint creation) and add a regression proving distinct namespace/name pairs cannot collide.

chaodu-agent · 2026-07-17T03:48:32Z

+    .await? {
+        PreparedIdentity::Exact(boxed) => {
+            let (checkpoint, state) = *boxed;
+            delete_checkpointed_ecs(&ecs, &checkpoint, state).await?;


🔴 F2 — Fence post-ECS cleanup against concurrent apply

Once this call observes the old incarnation gone, the code immediately deletes checkpointed API/Cloud Map resources and name-keyed S3 state. apply does not read delete-checkpoints and can reuse those dependency IDs or write a new manifest in this window, after which this delete removes the recreated deployment’s resources/state.

Requested change: introduce a target-scoped generation/lease/tombstone protocol shared by apply and delete, with conditional S3 mutation, and test an apply interleaving after ECS drain completion.

github-actions · 2026-07-18T08:50:33Z

Caution

This PR has been waiting on the author for more than 2 days (labeled pending-contributor since 2026-07-16).
It will be automatically closed in 24 hours if there is no update.

@chaodu-agent — You must add a new comment on this PR to remove the closing-soon label and keep it open. Pushing commits alone is not sufficient. Feel free to reopen a new PR later if it gets closed and you want to pick it back up.

chaodu-agent

Important

CHANGES REQUESTED ⚠️ — RC-1 is frozen; logical identity collision plus cleanup, documentation, and test gaps remain.

Consolidated review: #1415 (comment)

Contract freeze: RC-1 at 1bd61e825e3f2e1308bde3be6294e10b0e3bead6; SHA-256 a2377a772fc9ba7f4856cea6af2cc6f6596dba919f12256295fd65516de3c674.

GitHub event: COMMENT — self-review delivery only; this is not an approval.

chaodu-agent · 2026-07-18T17:05:56Z

+        )));
+    }
+    for target in targets {
+        if target.namespace.trim().is_empty() || target.name.trim().is_empty() {


🔴 F1 — Make logical target identity injective

Validation accepts both (prod, team-bot) and (prod-team, bot), although both derive oab-prod-team-bot; an initial lookup can therefore checkpoint and delete the other logical target. This violates RC-1 AC-2 before exact checkpoint validation can help.

Requested change: use one injective mapping shared by apply/delete, or reject every ambiguous component shape before AWS calls. A batch-only duplicate check is insufficient because separate calls must also be safe. Add the colliding-pair regression.

chaodu-agent · 2026-07-18T17:05:56Z

+                &checkpoint.region,
+            )
+            .await?;
+            cleanup_s3(&s3, bucket, namespace, name).await?;


🟡 F2 — Remove stale apply ingress-teardown state on successful delete

This success path removes the manifest/artifacts and delete checkpoint, but not ingress-teardown-checkpoints/{namespace}/{name}.json. If a prior apply was interrupted after writing that key, delete can report success while the stale key deterministically blocks re-apply.

Requested change: after exact dependent cleanup succeeds, remove the target apply-side teardown checkpoint without weakening retry safety, and test interrupted ingress teardown → delete → re-apply eligibility.

chaodu-agent · 2026-07-18T17:05:56Z

+    Ok(())
+}
+
+/// Tear down OAB services programmatically, without reading CLI home


🟡 F3 — Document the required same-target serialization boundary

RC-1 accepts the lack of an internal apply/delete fence only when callers serialize the full account/Region/bucket/cluster/namespace/name target. The public Rustdocs and user docs do not state this mandatory safety precondition.

Requested change: document the serialization and recovery rule for both apply_manifests and delete_services, while retaining the already-documented no-orphan behavior.

chaodu-agent · 2026-07-18T17:05:56Z

+    }
+
+    #[tokio::test]
+    async fn delete_services_rejects_empty_target_set() {


🟡 F4 — Add the contract-required regression evidence

The current tests cover fail-closed identity responses and checkpoint boundaries, but not logical-name collision prevention or the public partial-report promise when a later target fails.

Requested change: add a colliding-pair test plus a two-target failure test proving the first completion remains in DeleteError.completed and processing stops deterministically; include the F2 checkpoint cleanup sequence.

chaodu-agent

Important

CHANGES REQUESTED ⚠️ — The delete-only namespace guard does not close the shared apply/CLI identity collision.

Consolidated review: #1415 (comment)

GitHub event: COMMENT — self-review delivery only; this is not an approval.

chaodu-agent · 2026-07-18T19:55:47Z

+                target.name
+            )));
+        }
+        if target.namespace.contains('-') {


🔴 F1 — Apply and delete still operate over different logical identity domains

This guard rejects hyphenated namespaces only for programmatic delete, while manifest/apply validation still accepts them and CLI delete bypasses this function. Therefore prod/team-bot remains an accepted delete alias for a deployment created as prod-team/bot; both derive oab-prod-team-bot, and their logical checkpoint keys differ.

Requested change: enforce one shared injective identity/ownership rule before AWS calls across manifest/fleet apply and every programmatic and CLI delete entry point, with an explicit legacy migration policy and a cross-entry-point regression.

Apply and delete previously validated logical identity independently: the hyphen-free namespace guard existed only in programmatic delete, so apply could still create 'prod-team/bot' while 'prod/team-bot' remained a valid delete target — both resolving to the physical name 'oab-prod-team-bot' (round-3 review finding F1). A new shared module, operator/src/identity.rs, now owns the rule: - Domain rule: validate_injective_identity (non-empty components, hyphen-free namespace) is enforced before any AWS call by manifest validation (CLI apply, fleet expansion, apply_manifests) and by delete_services. - Ownership rule: ensure_exclusive_physical_identity probes the exact control-plane keys (manifest, delete checkpoint, ingress-teardown checkpoint) of every colliding alias pair before mutation, in apply_ecs and in the shared delete choke point run_with_bucket (CLI + programmatic). Contested physical identities fail closed naming the colliding pair. - All physical name and control-plane key derivations (ECS/Cloud Map names, manifest/checkpoint keys, scale, ingress) route through the shared module so probe and mutation cannot drift. - Manifest schema namespace pattern tightened to ^[a-z0-9]+$. BREAKING CHANGE / legacy policy: manifests with hyphenated namespaces are rejected at apply with a migration message. Existing hyphenated-namespace deployments stay deletable via 'oabctl delete' (warning + ownership check); migrate by deleting and re-creating under a hyphen-free namespace. Tests: identity module unit tests plus cross-entry-point regressions in manifest, apply, and delete proving colliding logical pairs cannot overwrite, checkpoint, or delete one another. cargo clippy --all-targets -- -D warnings and cargo test (113 + 1 doctest) pass.

chaodu-agent

Note

LGTM ✅ — The exact requested head closes the prior identity-collision and teardown-safety findings; no new in-scope blockers were found.

Consolidated review: #1415 (comment)

GitHub event: COMMENT — self-review delivery only; this is not an approval.

chaodu-agent · 2026-07-21T02:36:56Z

Note

LGTM ✅ — The round-3 blocker is closed: apply and every delete entry point now share one injective identity rule plus a pre-mutation control-plane ownership check, with an explicit legacy-namespace policy.

What This PR Does

Adds a public in-process delete_services API for explicit OAB namespace/name targets, symmetric with apply_manifests: explicit cluster, shared bucket-resolution chain, no ~/.oabctl/config.toml reads, durable exact-identity checkpoints before mutation, structured partial results, and suppressed progress output for programmatic callers. The CLI keeps its existing flow.

How It Works

Each target resolves to one live ECS service. Before mutation, delete stores a durable S3 checkpoint (caller boundary, bucket, canonical cluster/service ARNs, ECS createdAt incarnation, registry ARN, API ID), then drains that exact incarnation, removes exact ingress dependencies, deletes the manifest/artifacts, and removes the checkpoint last. Ambiguous AWS identity responses fail closed; matching retries resume from the checkpoint.

New in this revision, a shared operator/src/identity.rs module owns logical→physical identity:

Domain rule — validate_injective_identity (non-empty components, hyphen-free namespace) runs before any AWS call in manifest validation (CLI apply, fleet expansion, programmatic apply) and in programmatic delete, so oab-{namespace}-{name} parses back to exactly one accepted pair.
Ownership rule — ensure_exclusive_physical_identity runs pre-mutation in apply_ecs and in the shared delete choke point run_with_bucket (all CLI and programmatic delete paths). It probes the exact control-plane keys (manifest, delete checkpoint, ingress-teardown checkpoint) of every colliding alias split and fails closed naming the contested pair. Non-NotFound probe errors also fail closed.
No drift — all ECS/Cloud Map names and control-plane keys (manifest, delete-checkpoint, ingress-teardown-checkpoint) derive from the shared module; a regression asserts probe keys equal mutation keys.
Legacy policy — hyphenated-namespace manifests are rejected at apply with a migration message; existing legacy deployments remain deletable via oabctl delete (warning + ownership check). Schema metadata.namespace tightened to ^[a-z0-9]+$.

Findings

#	Severity	Finding	Location
1	🟢 Praise	Round-3 F1 is closed at every entry point: `prod/team-bot` vs `prod-team/bot` can no longer overwrite, checkpoint, or delete one another — the hyphenated side is outside the accepted domain, and the accepted side is blocked by the ownership probe whenever the legacy pair is recorded.	`operator/src/identity.rs`
2	🟢 Praise	The legacy-deployment policy is explicit rather than silent: apply fails with a migration hint, CLI delete warns and proceeds only under the ownership check.	`operator/src/delete.rs`, `operator/src/manifest.rs`
3	🟢 Praise	Cross-entry-point regressions cover apply-side rejection/acceptance, programmatic-delete rejection, alias symmetry, and probe/mutation key alignment.	`operator/src/{identity,manifest,apply,delete}.rs` tests

Finding Details

🟢 F1 (round-3) — Shared injective identity/ownership rule, verified per entry point

Independent exact-SHA audit of 54bd8f2..be006ec traced every entry point:

Apply: CLI apply -f and programmatic apply_manifests both reach validate_apply_request → OABServiceManifest::validate → validate_injective_identity; fleet manifests are validated post-expansion. apply_ecs runs the ownership probe before reading generation state or mutating.
Delete: CLI delete <name>, CLI delete -f, and programmatic delete_services all funnel through run_with_bucket, which runs the ownership probe before the pending-ingress-teardown guard, identity preparation, and any checkpoint/ECS mutation. Programmatic targets are additionally domain-rule-rejected pre-AWS in validate_delete_request.
Probe correctness: collision_aliases enumerates every alternative split point of {namespace}-{name}; within the hyphen-free-namespace domain the mapping is injective by construction, so collisions can only involve recorded legacy pairs — exactly what the probe checks. Probed keys are derived from the same helpers the mutation paths use.

ℹ️ Residual notes (within RC-1's accepted risks, documented)

The ownership check treats the control plane as the source of truth. A legacy deployment whose stored manifest was manually removed is invisible to the probe. Apply always records manifests and delete removes them only after full teardown (checkpoint last), so system-created states are covered; manual S3 tampering stays out of scope, consistent with RC-1's no-name-only-guessing stance. The existing "durable opaque deployment ID" follow-up would close this fully.
The probe-then-mutate sequence is not atomic. Same-physical-identity concurrency is governed by the same caller-serialization precondition RC-1 already documents; the generation/lease-fence follow-up remains the structural fix.

Baseline Check

PR opened: 2026-07-16
Reviewed head: be006ec67f5329df90423785e417e0ffb4671d85
Previous reviewed head: 54bd8f28eda19c3bd209789be89c44740d3a55f1
Main checked at: d8d4bf15ca9a6e7380a5683a87527b93f527badb
Main already has: no public delete_services/DeleteTarget/DeleteOptions API and no shared identity module
Net-new value since round 3: operator/src/identity.rs (domain rule, alias enumeration, ownership probe), wiring into manifest validation / apply_ecs / validate_delete_request / run_with_bucket, schema namespace pattern ^[a-z0-9]+$, legacy policy docs, and cross-entry-point regressions
git diff --check clean on the delta

Verified

On the exact reviewed head be006ec:

cargo clippy --all-targets -- -D warnings — clean
cargo test — 113 unit tests + 1 doctest passed, 0 failed
Exact-head CI: operator job (check, clippy, test, build) ✅, Review Contract ✅, all other required checks ✅
Independent exact-SHA audit (entry-point tracing, probe-key/mutation-key alignment, legacy policy, offline-safety of new tests) — passed
Note: the crate is intentionally not blanket-rustfmted in this commit; the pre-existing head was not fmt-clean and the operator CI job has no fmt gate, so the delta stays minimal for review.

Previous Review and External Feedback

Round-3 consolidated review

🔴 F1 — the collision guard exists only in programmatic delete; apply still accepts hyphenated namespaces and every CLI delete path bypasses the guard. Requested: one shared injective identity/ownership rule across all entry points, an explicit legacy-deployment policy, and cross-entry-point regressions.

✅ Addressed in be006ec. Shared rule in operator/src/identity.rs, enforced at every apply and delete entry point (domain rule pre-AWS, ownership probe pre-mutation); legacy hyphenated namespaces are apply-rejected with a migration message but remain CLI-deletable under the ownership check; regressions exercise both apply and delete sides of the colliding pair, including the alias-keyed checkpoint gap called out in round 3 (the probe checks the alias's manifest, delete-checkpoint, and ingress-teardown-checkpoint keys).

Prior real-AWS verification

Exact-checkpoint retry completed ECS, Cloud Map, API Gateway, and S3 teardown and removed the checkpoint last.

✅ Still valid for the canonical path; this revision adds only pre-mutation validation/probing before that path, plus read-only S3 probes that fail closed.

Automated stale-contributor notice

Operational only; superseded by current PR activity.

No other external reviewer findings were present on the reviewed head.

What's Good (🟢)

One identity module now owns every physical-name and control-plane-key derivation — probe and mutation cannot drift.
Fail-closed posture is preserved end-to-end: ambiguous ECS responses, contested identities, and probe errors all stop before destructive mutation.
The breaking change is explicit: ! commit, migration message in the error itself, README/CLI-guide policy sections, and a tightened published schema.
Programmatic progress suppression and structured partial reports are unchanged and still covered by tests.

5️⃣ Three Reasons We Might Not Need This PR

The CLI already performs teardown — if every control-plane caller could shell out safely, the public library contract adds compatibility and testing surface.
Logical names may be the wrong destructive identity — a durable opaque deployment ID would eliminate the alias problem structurally instead of validating around a string encoding; it remains the listed follow-up.
Tenant deprovisioning spans more than ECS/ingress/S3 — webhook, disclosure, secret, and serialization policy still live in the control plane, which could argue for orchestrating deletion there entirely.

chaodu-agent

Note

LGTM ✅ — Round-3 F1 (non-injective apply/delete identity) is closed on head be006ec by a shared identity module enforced at every apply and delete entry point, with an explicit legacy-namespace policy and cross-entry-point regressions.

Consolidated review: #1415 (comment)

chaodu-agent requested a review from thepagent as a code owner July 16, 2026 04:16