Tear down CLI-owned e2e agent when its suite ends#1204
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughShared teardown support was added to the amctl e2e suite, and the CLI agent platform suite now uses it to remove the CLI lifecycle agent and project. Stale cleanup also now matches ChangesCLI e2e shared teardown and cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
jhivandb
force-pushed
the
cli-e2e-shared-agent-teardown
branch
from
a9fe1c5 to
94a96a2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/testsetup/setup_shared_agents.go (1)
213-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated project-deletion retry logic.
deleteProjectBestEffortreimplements the same 5-attempt / 409-retry / 204|404-success loop that already exists inline inCleanupStaleE2EResources(test/e2e/testsetup/cleanup.golines 84-107). Having two copies risks them drifting (e.g. attempt count or backoff). Consider having the stale-sweep call this helper.🤖 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 `@test/e2e/testsetup/setup_shared_agents.go` around lines 213 - 240, `deleteProjectBestEffort` duplicates the same project-delete retry flow already used by `CleanupStaleE2EResources`; consolidate the logic so there is one source of truth. Update the stale-sweep path to call `deleteProjectBestEffort` for project cleanup, and keep the 5-attempt / 409-retry / 204|404-success behavior in that helper so it can be reused consistently.
🤖 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.
Nitpick comments:
In `@test/e2e/testsetup/setup_shared_agents.go`:
- Around line 213-240: `deleteProjectBestEffort` duplicates the same
project-delete retry flow already used by `CleanupStaleE2EResources`;
consolidate the logic so there is one source of truth. Update the stale-sweep
path to call `deleteProjectBestEffort` for project cleanup, and keep the
5-attempt / 409-retry / 204|404-success behavior in that helper so it can be
reused consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06614805-69a8-4dbb-bc1f-e1162d84ddcd
📒 Files selected for processing (5)
test/e2e/framework/amctl/suite.gotest/e2e/framework/shared_agent.gotest/e2e/tests/cli/agentplatform/suite_test.gotest/e2e/testsetup/cleanup.gotest/e2e/testsetup/setup_shared_agents.go
RAVEENSR
approved these changes
Jun 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The amctl CLI platform-agent e2e suite provisions a dedicated agent (
e2e-cli-it-helpdesk) in its own project (e2e-cli-shared) once on parallel process 1. Until now nothing ever deleted it: it was intentionally excluded from the stale-resource sweep (which only reapse2e-test-*), so the agent and project leaked indefinitely. This wires up an always-on teardown so the CLI suite cleans up the resources it fully owns when it ends.For:
Goals
Approach
amctl.WithSharedTeardown(func())to the CLI harness — the symmetric counterpart toWithSharedSetup. It runs exactly once on parallel process 1 inside the harness's existing singleSynchronizedAfterSuite, after every process has finished, before the built binary is dropped.testsetup.CleanupCLILifecycleAgent(...), a best-effort teardown that deletes the agent (existingDeleteAgentBestEffort) then its project (deleteProjectBestEffort, with 409-retry for the async agent cascade). It deletes by the well-known constant names rather than the provisioned handle, so cleanup still runs even if provisioning only partially succeeded. Failures are logged, never fatal.CleanupStaleE2EResources) to also reape2e-cli-*projects older than the 1h cutoff — the backstop forSynchronizedAfterSuitebeing skipped (e.g.kill -9). IntroducesE2ECLIProjectPrefixand derivesE2ECLIProjectNamefrom it.Why it's safe to always delete:
e2e-cli-it-helpdesk/e2e-cli-sharedare owned solely by this CLI suite. The HTTP observability suites read a different shared agent (e2e-it-helpdeskine2e-test-shared); a grep confirms nothing else references the CLI-owned names.Coverage by exit type:
SynchronizedAfterSuitedeletes it.kill -9/ second Ctrl-C / panic → next run's stale sweep reaps it.The CLI agent now rebuilds on every run; in the ephemeral CI environment the build happens regardless, so there is no added cost.
User stories
N/A — test-infrastructure change, no user-facing behavior.
Release note
N/A — internal e2e test harness only.
Documentation
N/A — no user-facing docs affected (updated in-code doc comments only).
Training
N/A
Certification
N/A
Marketing
N/A
Automation tests
go build ./...,go vet, andgofmtover the e2e module (all clean). The teardown path exercises only when the CLI platform-agent suite runs against a live environment.Security checks
Samples
N/A
Related PRs
N/A
Migrations
N/A — no DB migrations.
Test environment
Standard e2e environment (ephemeral). No special setup.
Learning
N/A
Summary by CodeRabbit