[birme]

Closes #164

Part of Eyevinn/osaas-deploy-manager#1878 (AC2).

Changes

• getInstancesToRemove dedupes by {serviceId, instanceName} (deploy-manager lists each pod separately).
• In remove-exceeding-tenant-instances, a FetchError with httpCode === 409 and message exactly Instance is already suspended is counted in instancesAlreadySuspended, logged and skipped. All other 409 reasons and other non-2xx stay failures (exit code 1). Reason strings verified against deploy-manager src/api_suspended.ts:93-115; createFetch puts reason into FetchError.message.
• Summary line keeps existing fields and appends , {n} already suspended.
• Tests: dedupe; 200; 409 already-suspended (exit 0); both other 409 reasons (exit 1); 500 (exit 1).

Impact on existing consumers: the only consumer is the admin-daily CronJob. It parses no output (exit code only, restartPolicy: OnFailure). The summary gains a trailing field; exit code changes only for the already-suspended case, which no longer exits 1. remove-all-instances also benefits from the dedupe (it would otherwise remove the same instance twice). No release commit; releasing is a human step in #1886.

Lessons

The deploy-manager reason surfaces as FetchError.message (not a separate field), so matching on it needs httpCode === 409 as well to avoid skipping a 500 with the same text. Line numbers in the issue had drifted but the premises held.

Co-Authored-By: Claude noreply@anthropic.com

🤖 Generated with Claude Code

[birme]

Research Review

Verified

• 409 reason strings: Instance is already suspended is returned at two paths in deploy-manager src/api_suspended.ts (fresh record within in-flight window, and probe state absent). The other two reasons (Instance suspension state could not be verified, Instance is running under a stale suspension record and was not re-suspended) match verbatim, so the exact-match skip does not swallow them.
• reason -> FetchError.message: confirmed in the published @osaas/client-core@0.26.0 (lib/fetch.js defaultErrorFactory: message: res?.message ?? res?.reason ?? ..., httpCode: response.status). FetchError is exported from the package index, and the CLI depends on ^0.26.0. Same package supplies createFetch and FetchError, so instanceof is safe.
• suspendInstanceForTenant uses createFetch with no custom errorFactory, so the default factory path applies.
• Dedupe: JSON.stringify([serviceId, instanceName]) key is collision-safe; order preserved. remove-all-instances consumers of getInstancesToRemove confirmed to be only cmd.ts (grep of packages).
• Summary line appends the new field at the end; existing fields and order unchanged.
• Impact statement and ## Lessons section present and filled in; Co-Authored-By trailer present.

Adjacent Stubs Detected

• None. Grepped cmd.ts and instance.ts for undefined : undefined, || undefined, TODO/FIXME, empty catch: no hits.

Orphaned Files Detected

• None. instance.ts and cmd.ts are existing, imported modules; the new files are tests.

Escape Hatch Coverage

• cmd.ts new skip branch is a loosening, not a rejection; the pre-existing failure path is unchanged. Passing-path tests exist (200 and 409 already-suspended at cmd.test.ts), plus both other 409 reasons and 500 asserting failure.

Object Shape Verification

• No spreads of fetched/ORM objects in the diff.

Contract diff

• No HTTP request handler changed. The CLI command remove-exceeding-tenant-instances changes exit code only for the already-suspended 409 case (1 -> 0), which is the stated AC2 of deploy-manager#1878 and is disclosed in the PR body. Summary output gains a trailing field; the only consumer (admin-daily CronJob) parses exit code only per the PR body. No new timeouts.

Impact Analysis

• Blast radius: @osaas/cli admin commands only.
• Downstream consumers: admin-daily CronJob (exit-code only). Takes effect only after the human release/image bump tracked in #1886.
• Coordinated changes needed: none in this PR; deploy-manager#1882 behaviour (reason semantics) is assumed deployed before CLI release.
• Breaking change risk: low.

Risks Not Tested

• Live end-to-end behaviour against a real deploy-manager 409 was not exercised; verified from source and the published client-core only.
• The test for cmd.ts mocks ./instance, so it exercises real FetchError objects but not createFetch's real error construction (verified by reading source instead).
• Exact-string match on the reason is brittle if deploy-manager rewords it; the failure mode is loud (exit 1), which is the safe direction.
• Jest/CI results were not re-run by me.

Research Suggestions

• Optionally add a shared constant or contract note linking the reason string to deploy-manager api_suspended.ts, since the two repos must change together.

Verdict

No blocking issues found.

---

Reviewed by PR Research Validator. This review checks whether code is based on verified sources rather than assumptions.