[birme]

Bug

In packages/cli/src/admin/cmd.ts, the remove-exceeding-tenant-instances command handler loops over instancesToRemove and calls suspendInstanceForTenant for each, but only the outer per-tenant try/catch (which also wraps getInstancesToRemove) existed. If suspendInstanceForTenant throws for one instance — e.g. a legitimate 409 Instance is already suspended from a prior run — the exception propagates out of the for loop and aborts processing of every remaining instance for that tenant.

Confirmed live impact: tenant maxlax has 17 active services, but the daily job only ever attempts the first (go-gitea-gitea), gets a 409, and the other 16 (including the actual token-draining encore instance) are never even attempted — every single day.

Context: https://github.com/Eyevinn/osaas-app/issues/5661

This PR does NOT close that issue and does NOT fix maxlax's current suspended state — it only prevents this class of failure from recurring going forward. A separate remediation is needed to actually suspend maxlax's remaining instances.

Fix

Moved await suspendInstanceForTenant(...) inside its own try/catch within the for loop. On failure it now logs Failed to suspend ${serviceId}: ${instance} for tenant ${tenantId}: <message> and continues to the next instance, instead of throwing out of the loop. The outer per-tenant catch (still needed for getInstancesToRemove failures) is unchanged.

Testing

Ran npx tsc --noEmit and npx eslint src/admin/cmd.ts / npx prettier --check src/admin/cmd.ts on the changed file — all pass. No existing test file covers remove-exceeding-tenant-instances or admin/cmd.ts (only src/web/cmd.test.ts exists for a different command), so no new test scaffolding was built for this fix per scope guidance.

Lessons

The bug was a granularity mismatch: a per-tenant try/catch was assumed to be sufficient error isolation, but any operation that loops over sub-resources (instances) inside a per-tenant block needs its own per-item try/catch, or one failed sub-resource silently starves all the others in the same iteration.

🤖 Generated with Claude Code

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

[birme]

PR Research Review

Verified

• Diff matches PR description: Read the actual file at the PR head commit (30c98230df5d376eef0f24651f1f2d7ece53b9d5, confirmed via gh pr view --json headRefOid) rather than trusting the PR body. packages/cli/src/admin/cmd.ts:119-134 shows suspendInstanceForTenant(...) moved into its own try/catch inside the per-instance for loop, exactly as described. On failure it logs Failed to suspend ${item.serviceId}: ${item.instance} for tenant ${tenant.tenantId}: ${message} and continues — matches the commit message and PR body verbatim.
• Fix is scoped correctly: The outer per-tenant try/catch (lines ~112-149) still wraps getInstancesToRemove(...) and is unaffected by this change — it still catches failures from that call and logs Failed to process tenant ${tenant.tenantId}: .... No behavior change for that path. The inner catch only swallows errors from suspendInstanceForTenant, nothing broader (no wildcard try around the whole loop body, no swallowing of getInstancesToRemove).
• "No existing test infra" claim confirmed accurate: Searched packages/cli/**/*.test.ts — only cli.test.ts, web/cmd.test.ts, and user/util.test.ts exist. Zero test files reference suspendInstanceForTenant, remove-exceeding-tenant-instances, or getInstancesToRemove (grep across the whole repo for those symbols in *.test.ts returned nothing). No admin/*.test.ts exists at all. The claim in the PR body is accurate, not an excuse to skip work that already existed.
• Attribution and Lessons: PR body contains both 🤖 Generated with [Claude Code] and Co-Authored-By: Claude <noreply@anthropic.com> trailers, and the sole commit also carries the co-author trailer. ## Lessons section is present and filled with a concrete, non-generic takeaway (per-item try/catch needed when looping over sub-resources inside a per-entity block) — passes the mandatory structural check.

Adjacent Stubs Detected

• None. Swept the full touched file (admin/cmd.ts) for the standard stub patterns (undefined : undefined, || undefined, TODO/FIXME/HACK, empty catches, log-only handlers). The console.log matches throughout are the file's normal CLI output pattern (this is a Commander CLI, not a request handler) — not stubs.

Orphaned Files Detected

• N/A. admin/cmd.ts is a CLI command module wired into cmdAdmin(); not applicable to the orphan-file sweep (no component/page reachability question here — this is a Node CLI action, already registered and invoked at CLI startup, unchanged by this PR).

Escape Hatch Coverage

• N/A. No new terminal-rejection branch introduced — this is a bug fix converting an uncaught throw into a caught, logged, continued path for a background admin CLI command, not an API/tool refusal path.

Impact Analysis

• Blast radius: Limited to the remove-exceeding-tenant-instances admin CLI command, run manually or by a scheduled job to suspend over-budget FREE-tier tenants' instances. No other command paths touched.
• Downstream consumers: This is a CLI binary (client-ts packages/cli), not a library export consumed by other repos in ~/Code/eyevinn/. No cross-repo impact.
• Coordinated changes needed: None. The PR body is explicit that this does NOT retroactively fix tenant maxlax's already-stuck-suspended state (osaas-app#5661) — a separate remediation is still needed for that. Good scoping discipline; no false "this closes the issue" claim.
• Breaking change risk: Low. Strictly increases robustness (continues past one bad instance) with no change to the happy path or to the outer catch's contract.

CI Status

• At review time, the single required check (Type Check, run 34354566151) was still in_progress — not yet green. Do not merge until it completes with a passing conclusion.

Risks Not Tested

• No test coverage exists (by design, matching the PR's accurate claim) for either the pre-fix bug or the post-fix behavior — the fix's correctness here rests on the manual code read above, not on an automated regression test. Consider whether a lightweight unit test for the loop-continuation behavior is worth adding given this command runs against production tenant data.
• Runtime behavior against the real maxlax tenant (17 instances, first one already 409ing) was not exercised from this diff review — the fix's practical effect can only be confirmed by running the command with --apply against live data, which is out of scope for a static review.

---

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