feat(aws): add cleanup tool - #2157
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an orphan-resource cleanup capability for AWS BYOC deployments: a new ChangesOrphan Cleanup Tool and AWS Implementation
Sequence Diagram(s)sequenceDiagram
participant User
participant CleanupTool as HandleCleanupTool
participant ByocAws
participant AWS as AWS APIs
participant EC as ElicitationController
User->>CleanupTool: invoke cleanup_resources
CleanupTool->>ByocAws: DiscoverOrphans(projectName)
ByocAws->>AWS: scan ALB/RDS/ECR/Route53 by prefix
AWS-->>ByocAws: matching resources
ByocAws-->>CleanupTool: []OrphanResource
alt no orphans found
CleanupTool-->>User: "no blocking leftovers"
else orphans found, non-interactive
CleanupTool-->>User: report + rerun instructions
else orphans found, interactive
loop each orphan
CleanupTool->>EC: RequestEnum(yes/no)
EC-->>User: prompt
User-->>EC: answer
EC-->>CleanupTool: choice
alt yes
CleanupTool->>ByocAws: CleanupOrphan(resource)
ByocAws->>AWS: unblock resource (disable protection / delete images / delete record)
AWS-->>ByocAws: result
end
end
CleanupTool-->>User: summary + "run defang down" guidance
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (2)
src/pkg/agent/tools/cleanup_test.go (1)
76-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
CleanupOrphanfailure case to the table.This suite never exercises
cleanupErr, so the new failed-cleanup reporting path inHandleCleanupToolcan regress without a test catching it. Add a case that returnscleanupErrand assert both the failed item output and the finalfailedcounter.Example table entry
{ name: "discovery error is surfaced", provider: &mockCleanerProvider{discoverErr: errors.New("boom")}, expectErr: "failed to discover leftover resources: boom", }, + { + name: "cleanup error is reported", + provider: &mockCleanerProvider{orphans: twoOrphans[:1], cleanupErr: errors.New("cannot clean")}, + confirm: "yes", + expectContains: []string{"failed: cannot clean", "0 cleaned, 0 skipped, 1 failed"}, + expectCleaned: nil, + }, }As per coding guidelines, "Use table-driven tests for multiple scenarios" and "Add tests for new behavior and important failure modes."
🤖 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 `@src/pkg/agent/tools/cleanup_test.go` around lines 76 - 121, Add a table-driven failure case in cleanup_test.go for the CleanupOrphan path that returns cleanupErr from the mock cleaner provider. Update the test table around HandleCleanupTool to include a scenario with orphans plus a failing cleanup, and assert both the failed item is reported in the output and the final summary shows the expected failed count. Use the existing mockCleanerProvider, cleanupErr, and expectContains/expectCleaned patterns so the new case covers the regression path without changing other scenarios.Source: Coding guidelines
src/pkg/clouds/aws/elbv2.go (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap AWS errors with operation context.
These helpers currently return bare SDK errors, so downstream cleanup output loses whether listing load balancers or toggling deletion protection failed. Please wrap the original error with the operation and target identifier while preserving the provider detail. As per coding guidelines, "Preserve cloud-provider error detail while wrapping errors with operation context in cloud SDK wrappers."
Also applies to: 49-55
🤖 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 `@src/pkg/clouds/aws/elbv2.go` around lines 25 - 27, The AWS ELBv2 helper calls are returning bare SDK errors, so add operation context while preserving the original provider error detail in the error path. Update the relevant wrapper functions in elbv2.go, including the DescribeLoadBalancers call and the deletion-protection-related helper mentioned in the comment, to wrap returned errors with the operation name and target identifier using their existing function names and parameters. Keep the underlying AWS error attached so downstream cleanup can still surface the provider-specific failure.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 `@src/cmd/cli/command/compose.go`:
- Around line 485-500: Skip initializing the debugger in the non-interactive
failure path. In the compose command’s error handling block, guard the call to
debug.NewDebugger so it only runs when interactive prompting is possible, since
handleTailAndMonitorErr already exits early for global.NonInteractive. Keep the
existing deploymentErr return flow, but avoid the agent.New/paid-tier lookup
work and the “Failed to initialize debugger” warning in CI/non-interactive runs.
In `@src/pkg/agent/tools/cleanup.go`:
- Around line 32-35: The cleanup flow is ignoring the error from
loader.ProjectWorkingDir and falling back to an empty working directory, which
can send stacks.NewManager down the wrong project context. Update the code in
cleanup.go so the ProjectWorkingDir call is checked and any failure is returned
immediately as a wrapped error before creating the stack manager. Keep the fix
localized around ProjectWorkingDir and stacks.NewManager so failures are
deterministic and not masked by the empty string fallback.
In `@src/pkg/cli/client/byoc/aws/cleanup.go`:
- Around line 78-80: `DiscoverOrphans` is swallowing category lookup failures
and can return an empty orphan list with no error, which makes
`HandleCleanupTool` report a false “No leftover resources found”. Update
`DiscoverOrphans` to track errors from the AWS lookup helpers (for example the
`aws.FindLoadBalancersByPrefix` / other category lookups at the referenced
blocks) and, if nothing was discovered successfully, return a wrapped aggregated
error instead of nil; only return nil when at least one discovery path
succeeded. Keep the existing `term.Warnf` logging, but make sure the final
return reflects total discovery failure so `HandleCleanupTool` can surface it.
In `@src/pkg/cli/client/byoc/aws/domain_test.go`:
- Around line 40-43: The r53Mock.ChangeResourceRecordSets stub currently returns
nil, nil, which can hide unexpected DNS mutation calls during tests. Update this
mock to fail fast in the test path by panicking or otherwise recording and
asserting the request when ChangeResourceRecordSets is invoked, so accidental
cleanup mutations are surfaced immediately. Use the ChangeResourceRecordSets
method on r53Mock as the place to make this behavior explicit.
In `@src/pkg/clouds/aws/ecr.go`:
- Around line 61-65: The BatchDeleteImage call in the ECR helper currently
treats any HTTP 200 as success, which can hide per-image failures. Update the
deletion flow in the BatchDeleteImage-related helper to inspect the returned
BatchDeleteImageOutput.Failures after the svc.BatchDeleteImage call, and return
an error when that slice is non-empty. Keep the check alongside the existing
error handling so the helper only reports success when all requested images were
actually deleted.
In `@src/pkg/debug/debug.go`:
- Around line 90-104: The prompt-default lookup in isPaidAccount is performing a
live WhoAmI network call and can block NewDebugger on the full command context;
make that remote side effect explicit and bounded. Rename or wrap the helper so
the call site in NewDebugger clearly signals network I/O, and use a short-lived
context with timeout/deadline for the Fabric lookup instead of the long command
context. Keep the fallback behavior to false in the error path and preserve the
existing term.Debug logging around the whoami failure.
---
Nitpick comments:
In `@src/pkg/agent/tools/cleanup_test.go`:
- Around line 76-121: Add a table-driven failure case in cleanup_test.go for the
CleanupOrphan path that returns cleanupErr from the mock cleaner provider.
Update the test table around HandleCleanupTool to include a scenario with
orphans plus a failing cleanup, and assert both the failed item is reported in
the output and the final summary shows the expected failed count. Use the
existing mockCleanerProvider, cleanupErr, and expectContains/expectCleaned
patterns so the new case covers the regression path without changing other
scenarios.
In `@src/pkg/clouds/aws/elbv2.go`:
- Around line 25-27: The AWS ELBv2 helper calls are returning bare SDK errors,
so add operation context while preserving the original provider error detail in
the error path. Update the relevant wrapper functions in elbv2.go, including the
DescribeLoadBalancers call and the deletion-protection-related helper mentioned
in the comment, to wrap returned errors with the operation name and target
identifier using their existing function names and parameters. Keep the
underlying AWS error attached so downstream cleanup can still surface the
provider-specific failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 133488ad-b0e1-4d3e-b56c-950cf8fbc213
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
src/cmd/cli/command/compose.gosrc/go.modsrc/pkg/agent/tools/cleanup.gosrc/pkg/agent/tools/cleanup_test.gosrc/pkg/agent/tools/tools.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/cleanup.gosrc/pkg/cli/client/byoc/aws/domain_test.gosrc/pkg/cli/client/cleanup.gosrc/pkg/cli/stacks.gosrc/pkg/cli/stacks_test.gosrc/pkg/clouds/aws/ecr.gosrc/pkg/clouds/aws/ecr_test.gosrc/pkg/clouds/aws/elbv2.gosrc/pkg/clouds/aws/elbv2_test.gosrc/pkg/clouds/aws/rds.gosrc/pkg/clouds/aws/rds_test.gosrc/pkg/clouds/aws/route53.gosrc/pkg/clouds/aws/route53_cleanup_test.gosrc/pkg/debug/debug.gosrc/pkg/utils.go
| func (r r53Mock) ChangeResourceRecordSets(ctx context.Context, params *route53.ChangeResourceRecordSetsInput, optFns ...func(*route53.Options)) (*route53.ChangeResourceRecordSetsOutput, error) { | ||
| // TODO: implement if needed | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail fast on unexpected DNS mutation calls in this mock.
Returning nil, nil here makes any accidental ChangeResourceRecordSets call look successful, so tests can miss a broken cleanup path. Panic or record/assert the request instead. Based on learnings, in Go test files under this repo it's acceptable for mocks to panic to surface issues quickly during tests.
🤖 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 `@src/pkg/cli/client/byoc/aws/domain_test.go` around lines 40 - 43, The
r53Mock.ChangeResourceRecordSets stub currently returns nil, nil, which can hide
unexpected DNS mutation calls during tests. Update this mock to fail fast in the
test path by panicking or otherwise recording and asserting the request when
ChangeResourceRecordSets is invoked, so accidental cleanup mutations are
surfaced immediately. Use the ChangeResourceRecordSets method on r53Mock as the
place to make this behavior explicit.
Source: Learnings
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/debug/debug_test.go (1)
27-36: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMock now silently no-ops instead of failing fast, and the new feedback flow has no test coverage.
AskOnepreviously presumably failed loudly on an unexpected response type; theok-guarded check now silently skips setting*stringresponses (used by the newpromptForFeedback), so a wiring bug there would go unnoticed. Based on learnings, mocks in this repo should panic on type mismatches rather than add defensive handling, since panics surface issues fast.Additionally, no test exercises
promptForFeedback's new string-based flow or the"Feedback Prompt Answered"tracking property added indebug.go.♻️ Suggested mock + test addition
type mockSurveyor struct { - response bool + response bool + feedbackResponse string } func (s *mockSurveyor) AskOne(q survey.Prompt, response interface{}, opts ...survey.AskOpt) error { - if boolptr, ok := response.(*bool); ok { - *boolptr = s.response + switch v := response.(type) { + case *bool: + *v = s.response + case *string: + *v = s.feedbackResponse + default: + panic(fmt.Sprintf("unexpected response type %T", response)) } return nil }🤖 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 `@src/pkg/debug/debug_test.go` around lines 27 - 36, Update mockSurveyor.AskOne in debug_test.go to fail fast on unexpected response types instead of silently ignoring them, so the mock panics or otherwise clearly errors when the passed response is not the expected type. Then add test coverage for promptForFeedback in debug.go using the mockSurveyor to exercise the new string-based prompt flow and verify the "Feedback Prompt Answered" tracking property is recorded.Sources: Path instructions, 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 `@src/pkg/debug/debug.go`:
- Around line 132-139: The feedback tracking in promptAndTrackDebugSession is
now sending raw free-text from promptForFeedback to analytics, which should be
avoided. Update the event emitted via track.Evt for “Feedback Prompt Answered”
to stop forwarding the full feedback string, and instead either track only that
feedback was provided or pass a truncated/redacted value before calling
track.Track. Keep the failure path and the promptForFeedback flow intact, and
make the change wherever this feedback-answer tracking is duplicated.
---
Outside diff comments:
In `@src/pkg/debug/debug_test.go`:
- Around line 27-36: Update mockSurveyor.AskOne in debug_test.go to fail fast on
unexpected response types instead of silently ignoring them, so the mock panics
or otherwise clearly errors when the passed response is not the expected type.
Then add test coverage for promptForFeedback in debug.go using the mockSurveyor
to exercise the new string-based prompt flow and verify the "Feedback Prompt
Answered" tracking property is recorded.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0bde84f-f4da-488c-97c1-24e68df7467a
📒 Files selected for processing (4)
src/cmd/cli/command/compose.gosrc/pkg/cli/client/byoc/aws/cleanup.gosrc/pkg/debug/debug.gosrc/pkg/debug/debug_test.go
💤 Files with no reviewable changes (1)
- src/pkg/cli/client/byoc/aws/cleanup.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cmd/cli/command/compose.go
b5c9e07 to
43a0aa3
Compare
43a0aa3 to
b63a996
Compare
|
Slimmed this PR down to pure cleanup: the debugger commits (switch AI debugger to default true, feedback prompt refactor) and the compose-down debugger hook moved to PR 2158, which now targets main directly. What remains here: the |
lionello
changed the title
|
What about Azure/GCP? @defangdevs |
|
Checked the pulumi-defang providers. AWS is the only one where teardown can get stuck on a resource:
GCP and Azure retain some resources too, but by design, not as a side effect:
So the only concrete follow-up I see is GCP Cloud SQL deletion protection, if you want it covered. Want that added to this PR, or should I file a follow-up issue for it? |
|
point of cleanup is to clean up 100%:
|
Adds OrphanCleaner support for Azure: the project resource group is deliberately retained on `defang down` to preserve the project's Key Vault, so DiscoverOrphans reports it and CleanupOrphan deletes it (which cascades to the vault too). Generalizes the tool's gating message and description now that it's no longer AWS-only.
|
Pushed Azure support (459b625): GCP Cloud SQL needs more thought before I write it. In Two ways to unblock this safely:
Let me know which way you want it, and I'll follow up with the GCP piece. |
|
Picking up your three points on this branch. Plan below before I write anything. 1. GCP cleanup lands here, not in a stacked PRMerged New Discovery cannot use labels for the two resources that matter. A name prefix alone is not safe enough, because the VPC name has no stack in it — two stacks of one compose project produce identically-prefixed VPCs, and picking wrong would delete a live network. So the name is only the candidate filter, and the safety check is the real invariant, read from GCP: a candidate is reported only if nothing is still attached to it (no Cloud Run service, instance, or forwarding rule on its subnets). A VPC with nothing attached is safe to delete no matter which stack it came from; one with attachments is skipped with the reason shown. Order matters here, unlike AWS. The 1-2h Cloud Run IP-release window gets named explicitly: a subnet delete returning 2. Cobra command, not only MCP
3. A failed teardown points the AI at the cleanup toolThe hook already exists on Two things I could not verify
|
…aware debugger Three additions to the cleanup capability, all on this branch rather than stacked on it. **GCP networking cleanup.** The Pulumi program creates the VPC network, subnet, service networking connection and MIG instance templates with RetainOnDelete, so a successful `down` drops them from Pulumi state but not from GCP. The leaked networks eventually exhaust the project's NETWORKS quota, which is what blocked the new-provider GCP sanity test (pulumi-defang#183). New byoc/gcp/cleanup.go implements client.OrphanCleaner beside the AWS and Azure ones, and pkg/clouds/gcp/network.go holds the compute calls. No new dependency: google.golang.org/api/compute/v1 was already used by GetInstanceGroupManagerLabels and covers everything needed, including networks.removePeering. Two design points worth knowing: - Discovery cannot use labels. cd/program/gcp.go sets defang-project and defang-stack as DefaultLabels on every resource, but GCP networks and subnetworks have no labels field, so the deterministic name is the only handle: Pulumi auto-naming of the logical name projectName+"-vpc" gives <project>-vpc-<hex>. - That name carries no stack, so two stacks of one compose project share a prefix and a prefix match alone must never authorise a delete. DiscoverOrphans therefore checks GCP for live attachments -- instances, forwarding rules, and Cloud Run services with Direct VPC egress -- and skips any network that has one, or whose usage could not be established. This is what makes a wrong name guess harmless. Orphans are returned in the order GCP requires (instance templates, peering, reserved range, subnet, network) because unlike the AWS resources these have hard dependencies. The interface documents that the order is significant. The one expected failure is explained rather than dumped: Cloud Run holds a subnet's IP addresses for 1-2 hours after the last service using it is deleted, so a subnet delete rejected with resourceInUseByAnotherResource reports when to come back instead of a raw API error. **A cleanup command.** `defang cleanup`, with --dry-run and --yes. The confirm-and-report loop moves out of HandleCleanupTool into pkg/cli.Cleanup so the command and the cleanup_resources tool share one implementation and cannot drift. **A teardown-aware debugger.** The hook already existed: a failed `down` prompts the debugger, and the agent already registers cleanup_resources. But the prompt opened with "An error occurred while deploying this project", which tells the model nothing about orphaned resources. DebugConfig now carries the operation, and a teardown or cleanup failure says resources were probably left behind and to use cleanup_resources. A failed `defang cleanup` offers the debugger the same way, via DebugCleanupError. Tests: the shared loop (order preservation, dry run, the non-interactive report-only path, --yes, skip and failure counts, a broken confirmation transport), GCP discovery and ordering against a mock driver including every in-use veto, and the compute helpers (self-link comparison, region extraction, async operation error mapping). Not covered: the compute and Cloud Run API calls themselves, and a real subnet delete inside the release window. Those need a GCP down on a stack with Postgres plus the ~2h wait. The naming is also unconfirmed against a live project -- gcloud credentials in my sandbox had expired -- which is why the attachment check, not the name, is what keeps this safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3WmpdY3zc555sNdkY9dzQ
|
Pushed 1. GCP networking cleanup
Two things came out of the reading that are worth knowing: Labels cannot be used for discovery, which surprised me. That name has no stack in it, so two stacks of one compose project share a prefix, and a prefix match alone must never authorise a delete. Orphans come back in the order GCP requires — instance templates, peering, reserved range, subnet, network — because unlike the AWS resources these have hard dependencies. The interface now documents that the order is significant. The one expected failure is explained rather than dumped: a subnet delete rejected with 2. Cobra command
3. Teardown failure points the AI at the toolThe hook was already on What is not coveredUnit tests cover the shared loop (order preservation, dry run, the non-interactive report-only path, Not covered, and it needs a real run: the compute and Cloud Run API calls themselves, and a subnet delete inside the release window — that wants a GCP One consequence worth deciding on: if this is the cleanup path we want, then pulumi-defang#462 is the alternative to it rather than a complement, and shipping both would leave two teardown paths for the same resources. I have left 462 as a draft. |
Four merges landed on main after the previous merge: #2225, #2226, #2224 (buildkit bump) and #2221. The conflicts were all dependency state, none in the cleanup code: - src/go.mod, src/go.sum: this branch carried AWS SDK pins from when it was opened in June, main has newer ones. Resolved to main's versions and re-ran `go mod tidy`, which re-added the ecr, elbv2 and rds modules this branch needs at their current versions. - pkgs/defang/cli.nix: took main's vendorHash as the closer starting point. The Update vendorHash step in go.yml corrects and commits it if the tidy moved it. src/pkg/cli/client/byoc/aws/byoc.go merged cleanly. `go build ./...` and `go test -short ./...` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3WmpdY3zc555sNdkY9dzQ
…p errors Removes the `isPaidAccount`/`AutoApprove` gate as requested in review. The WhoAmI round-trip is gone from the post-failure path, and no caller auto-runs the AI debugger in CI anymore: the non-interactive branches print the debug hint and skip building the debugger entirely, so `agent.New` is no longer paid for where nothing can prompt. Also from review: - agent cleanup tool: return the ProjectWorkingDir error instead of resolving the project against an empty directory. - AWS DiscoverOrphans: collect per-category failures and return them when nothing was discovered, so a permissions or throttling failure is no longer reported as "no leftover resources found". - ECR DeleteImages: BatchDeleteImage answers 200 with a per-image Failures list, so check it; one undeleted image still blocks the repository. - debug feedback: cap the free-form survey answer sent to analytics. - route53 test mock: panic on unexpected ChangeResourceRecordSets rather than reporting success. - cover the CleanupOrphan failure path and the ECR per-image failure path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3WmpdY3zc555sNdkY9dzQ
|
Addressed the review findings in 73c407f. The gate is gone, as you said. The rest:
Note this review was from June 30 — the findings all still applied to today's head. |
main gained #2228 (compose up must exit non-zero when the AI debugger runs), which touches the same functions as the review fixes on this branch. Kept main's fix: the debugger runs for its side effect and handleComposeUpErr returns the original error. Its regression test survives the reconciliation and still fails with "got <nil>" if the fix is reverted. Reconciled the removal of the paid-tier gate with that test's needs. The gate is gone, so the debugger is no longer built at all when there is nobody to prompt, but #2228 needs it injectable to test how the caller handles its result. The handlers now take a debuggerFactory that is only called once the caller knows it will use one, which satisfies both: no agent connection in CI, and a stub in tests. NewDebuggerForTest loses its defaultPermission argument. TestHandleComposeUpErrWithoutAutoApprove becomes TestHandleComposeUpErrNonInteractiveSkipsTheDebugger: with no tier gate, the thing worth asserting is that CI never builds a debugger, and still gets the deployment error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3WmpdY3zc555sNdkY9dzQ
|
Merged main and reconciled with #2228, which landed while I was pushing the review fixes and rewrites the same functions. Kept the #2228 fix. The debugger runs for its side effect and The collision, and how it is resolved. #2228 needs the debugger injectable so the test can control what it returns. My first pass built it inside the handler, which would have made that test impossible — it would have removed coverage of a bug that just reached production. The handlers now take a
One consequence of #2228 reading against the gate removal worth stating plainly: that PR's description notes the false green "hits exactly the paid accounts whose CI runs the debugger unattended". With the gate gone, no account runs the debugger unattended in CI, so that path no longer exists at all. The exit-code fix still matters for the interactive case, which is why I kept it.
|
DiscoverOrphans looked for "<project>-vpc", which is the legacy CD's naming.
The current CD sets pulumi:autonaming to
"<lower(prefix)>-${project}-${stack}-${name}-${hex(7)}", so the network is
actually named "defang-<project>-<stack>-vpc-<hex7>" and never matched — the
tool reported no orphans while the leaked VPC was sitting right there.
Found by a live up/down cycle against defang-playground-dev, which left
"defang-cdtest-min-local1-vpc-be420a3" standing and undiscovered.
Match both shapes: the legacy networks are the ones most likely to have leaked
already, and the current one is every network from here on. Deduplicate, since
offering the same network twice would 404 on the second pass. The extra stack
segment also narrows the current prefix, so it can no longer reach a sibling
stack of the same project.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T3WmpdY3zc555sNdkY9dzQ
Live GCP up/down cycle: one bug found and fixed, and the design confirmedRan a real cycle against Bug: discovery never matched a current network
The live cycle left Fixed in fe873de: match both shapes and deduplicate. The legacy networks are the ones most likely to have leaked already; the current shape is every network from here on. The extra stack segment also narrows the current prefix so it cannot reach a sibling stack of the same project. After the fix, the same project discovers both resources in the right order. The rest of the design is confirmed against real GCP
So the The cleanup tool then behaves as designed: it finds both, attempts them in order, and annotates both failures with the release-window explanation and the instruction to re-run What actually holds the subnet, and why nothing can force itThe blocker is a regional address with
One improvement worth considering: Not yet coveredThe debugger hand-off itself — |
An earlier logical name for the network was "<project>-vpc" rather than "vpc", so the autonaming pattern produced "defang-<project>-<stack>-<project>-vpc-<hex7>" — the project name twice. Those networks exist in live projects today, side by side with the current shape and the legacy CD's; all three were observed in defang-playground-dev. Networks from older code are exactly the ones most likely to have leaked already, so missing this shape would miss much of the backlog the cleanup command exists to clear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3WmpdY3zc555sNdkY9dzQ
2 participants
Description
Add cleanup tool to AI, for cleaning up protected resources.
Linked Issues
Fixes #1041
Checklist
Summary by CodeRabbit
New Features
Bug Fixes