[birme]

Fixes two bugs in the admin CLI:

1. remove-all-instances (Eyevinn/osaas-app#5672)
The for loop had only an outer try/catch — first failure aborted all remaining instances. Added inner try/catch per item (same pattern as remove-exceeding-tenant-instances already had after PR #158), tracking removed/failed counts.

2. remove-exceeding-tenant-instances (Eyevinn/osaas-app#5673)
Added instancesFailed/tenantsFailed counters and set process.exitCode = 1 on any failure, so the admin-daily CronJob correctly records failures rather than always showing Succeeded.

Lessons

Per-item error isolation in admin bulk operations should always use inner try/catch. A single outer catch that catches the loop is a 1-failure-aborts-all pattern. The fix for one command was already done in PR #158 68 lines above — checking sibling commands for the same pattern would have caught this earlier.

🤖 Generated with Claude Code

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

[birme]

Research Review

Verified

• Missing Lessons Section: Present and filled in. ✓
• Co-Authored-By trailer: Present in both PR body and commit message. ✓

remove-all-instances fix (osaas-app#5672): Inner try/catch now wraps each removeInstanceForTenant call. instancesRemoved++ on success, instancesFailed++ on catch. The loop continues past individual failures. Correct.

remove-exceeding-tenant-instances fix (osaas-app#5673): instancesFailed is incremented inside the per-instance inner catch (line ~133); tenantsFailed is incremented inside the per-tenant outer catch (line ~145). The exit-code gate if (instancesFailed > 0 || tenantsFailed > 0) correctly covers both paths — a tenant-level failure alone (e.g. getInstancesToRemove throwing) still triggers process.exitCode = 1.

process.exitCode = 1 vs process.exit(1): Commander v12 is in use. Setting process.exitCode and returning lets the async action handler resolve normally; Node.js exits with code 1 after the event loop drains. No hang risk — the Commander action returns after setting it, and nothing holds the event loop open after that.

Counter coverage: All failure paths increment a counter. instancesFailed is in the per-instance catch; tenantsFailed is in the per-tenant catch. No failure path is counter-silent.

Needs Verification (non-blocking, pre-existing)

• Outer catch on both commands does not set process.exitCode = 1: If setup-phase operations fail (e.g. getTenantPlanMap, getInstancesToRemove, or the confirm() prompt throwing), the outermost catch in each action handler only logs the message and returns, leaving exit code 0. This was pre-existing before this PR and is not introduced here. Worth a follow-up issue if the CronJob needs to detect setup failures too.

Adjacent Stubs Detected

None found in touched file.

Orphaned Files Detected

packages/cli/src/admin/cmd.ts is the implementation behind the CLI command registration — entry point is the CLI binary. Not an orphan.

Escape Hatch Coverage

No new terminal-rejection branches introduced.

Object Shape Verification

No ORM/SDK object spreads introduced.

Impact Analysis

• Blast radius: packages/cli/src/admin/cmd.ts only. No shared library exports changed.
• Downstream consumers: None — this is a CLI binary, not a library.
• Coordinated changes needed: None.
• Breaking change risk: Low — behavior change is additive (errors now continue past failures and set non-zero exit code, which is the correct behavior the CronJob was supposed to see).

Risks Not Tested

• The outer catch path (setup-phase failures) still exits 0 — not introduced by this PR, but the admin-daily CronJob would still record "Succeeded" if getTenantPlanMap or getInstancesToRemove throws. Pre-existing gap, not a blocker for this fix.

---

Verdict: CLEAR — both bugs are correctly fixed. Inner try/catch isolation is verified, counters cover all failure paths, process.exitCode = 1 is the right mechanism with Commander v12, and the Co-Authored-By and Lessons requirements are satisfied.

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

[birme]

PR Research Review

Verified

• PR fix: don't abort suspending a tenant's remaining instances when one is already suspended #158 pattern claim: Confirmed. PR fix: don't abort suspending a tenant's remaining instances when one is already suspended #158 (merged 2026-09-09) added inner try/catch around suspendInstanceForTenant in remove-exceeding-tenant-instances. PR fix(admin): fix error handling and exit codes in admin CLI commands #161 applies the structurally identical pattern to remove-all-instances around removeInstanceForTenant. Both wrap the async call in try/catch, increment a failure counter in catch, log the error with the same serviceId/instance: message format, and set process.exitCode = 1 after the loop if instancesFailed > 0. Pattern is not invented — it is a direct mirror.

• Counter logic: Correct in both commands. instancesRemoved++ runs only in the success path of the inner try (after await removeInstanceForTenant returns without throwing). instancesFailed++ runs in the catch branch. instancesSuspended++ and instancesFailed++/tenantsFailed++ in remove-exceeding-tenant-instances are also correctly placed.

• process.exitCode = 1 placement in remove-exceeding-tenant-instances: Placed after the summary console.log and inside the outer try, before the outer catch (line 163 in the PR branch). This means it fires for per-item and per-tenant failures captured by the inner catches, which is the intended scope.

• process.exitCode = 1 placement in remove-all-instances: Same structure — inside the outer try, after summary log, before the outer catch (line 218). Correct.

• No fabricated API calls: All called functions (removeInstanceForTenant, suspendInstanceForTenant, getTenantPlanMap, getTenantTokenCounts, getInstancesToRemove) are imported from ./instance and ./money — both already present at the top of the file before this PR. No invented signatures or field names.

• cmdAdmin export reachability: cmdAdmin is the default export of cmd.ts and is imported and registered at packages/cli/src/cli.ts line 6 and 52. Not orphaned.

• CI: Passing (single ts check job, 1m37s run).

Needs Verification

• process.exitCode = 1 not set on outer catch: If getTenantPlanMap() or getTenantTokenCounts() throws before the loop in remove-exceeding-tenant-instances, execution falls into the outer catch block, which only logs (err as Error).message — it does not set process.exitCode = 1. The admin-daily CronJob would record Succeeded even though zero tenants were processed. The same applies to getInstancesToRemove in remove-all-instances (though the outer catch there is pre-existing behavior). This is a pre-existing gap that predates this PR and is not claimed to be fixed — but since the PR's stated goal for #5673 is "so the admin-daily CronJob correctly records failures", it is worth noting that a total pre-loop failure is still undetected.

Adjacent Stubs Detected

None found. console.log calls throughout are the established pattern for this admin CLI and are not stubs.

Orphaned Files Detected

None. cmdAdmin is imported at packages/cli/src/cli.ts:6 and registered at line 52.

Escape Hatch Coverage

No new terminal-rejection branches introduced. process.exitCode = 1 is not a process-exit or throw — it defers the non-zero exit to after Commander completes its own teardown, so the summary log still prints before the process exits. This is the correct Node.js pattern for CLI exit codes.

Object Shape Verification

No spreads of ORM/fetched objects in the diff.

Impact Analysis

• Blast radius: Admin CLI only (osc admin remove-all-instances, osc admin remove-exceeding-tenant-instances). No API routes, no shared library exports, no database schema changes.
• Downstream consumers: The admin-daily CronJob invokes these commands. With process.exitCode = 1 now set on partial failure, the CronJob's status will correctly show Failed instead of Succeeded when instances could not be removed/suspended — which is the stated goal of #5673.
• Coordinated changes needed: None identified.
• Breaking change risk: Low — only changes CLI exit behavior (was always 0; now non-zero on failure) and loop-abort behavior (was abort-on-first-failure; now continue-on-failure). Both are improvements over the current behavior.

Risks Not Tested

• No unit tests exist for packages/cli/src/admin/cmd.ts — this is a pre-existing condition noted by PR fix: don't abort suspending a tenant's remaining instances when one is already suspended #158's body ("No existing test file covers remove-exceeding-tenant-instances or admin/cmd.ts"). The fix logic (inner try/catch per item, counter increments, exitCode setting) is simple and low-risk, but the absence of tests means edge cases like "all items fail", "zero items", or "outer function throws" are not mechanically verified.
• The outer-catch-does-not-set-exitCode gap (noted above under Needs Verification) is not tested by any existing or new test.
• Runtime behavior of process.exitCode vs process.exit(1) in the CronJob's container runtime is not verifiable from the diff alone — process.exitCode sets the eventual exit code without immediately stopping execution, which is correct for this pattern, but depends on no other code path resetting it to 0 after the command runs.

Does not block merge. The implementation correctly mirrors PR #158's verified pattern, counter logic is sound, exit-code placement is correct for the stated goals, and no fabricated contracts or invented API shapes were found. The outer-catch exitCode gap is a pre-existing limitation worth a follow-up issue but does not affect the correctness of what this PR claims to fix.

---

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