fix(rds): deduct expiring RIs from family coverage in target-coverage sizing

[cristim]

Closes #2132

What

applyCoverageAndOverrides adjusted per-rec coverage for RIs expiring inside --rebuy-window-days, then handed the original coverage map to the RDS family-NU pass, so a family at target was dropped before the rebuy window could matter. It now calls ApplyFamilyNUSizingRDSWithExpiryOptions (cloud-commitments-go#341 plus #347, pin bumped in #2173 and again here) with the existing commitments, the window, and CoverageLookbackDays = the same coverageLookbackDays(cfg) given to GetRICoverageMap. Replacement RIs bought inside the lagged coverage window are credited, so a daily re-run does not buy them again (gate F1). FamilyExpiryReport.Unusable is logged as a warning ("N RDS reserved instances expiring within D days could not be applied to family sizing ... their expiry was NOT deducted"); the log line reports the expiring NU and the credited replacement NU (it no longer claims a net deduction; exact net is go#342).

RDS purchase counts and costs deliberately increase in the expiring-RI case (the family is sized against post-expiry coverage). No purchase flag or execution path changes.

Callers: fetchAndFilterRegionRecs passes the real inventory. The legacy test-only per-region path (multi_service_helpers.go:472) keeps nil commitments, which makes the new call identical to ApplyFamilyNUSizingRDS; documented on both functions. Non-RDS and EC2 paths are untouched. The deduction is capped at the expiring NU because the CLI inventory is single-account (#2131, out of scope here); CSV AZ (#2174) is also separate.

Tests

Extends the recorded-HTTP completenessProxy harness (new rds-expiry mode, dry run, no purchase op allowed) with TestRDSFamilyExpiryCommand: already-at-target, below-target (exact count and cost), large RI vs xlarge usage, multi-size with 3 linked-account rows (exact count, no double deduction), partial inventory (deduction capped at expiring), unusable RIs warned, re-run after the replacements were bought (lagged coverage 82.5%, 4 expiring, 4 replacements bought 2 days ago, 12 other-account large not in the inventory: 0 purchased); controls: replacement outside the lookback (3), outside-window, engine isolation. The non-RDS control is the existing EC2 TestReservationExpiryCommand. Fixtures are synthetic, account IDs built at runtime.

Fails before the fix (same tests, cmd/multi_service_helpers.go reverted to the parent):

scenario	parent	fix
already-at-target	family dropped (0 rows)	3
below-target	8	10
large-ri-xlarge-usage	dropped	2
multi-size-linked-accounts	1 / 2 / 1	2 / 4 / 2
partial-inventory	dropped	4
unusable-expiring-warned	no warning	warning for 2 RIs
rerun-replacement-bought	3 (over-purchase, pre-F1 head 9e3f9eb)	0
control-outside-window, control-engine-isolation	pass	pass

Local: go test ./cmd full pass, golangci-lint 0 issues, pre-commit on changed files and git secrets --scan clean.

Go bump in this update (218912bb, on top of #2173's 2c1e87b)

• go#347 218912bb: credit own replacement RIs in RDS family expiry sizing (the F1 fix; adds ApplyFamilyNUSizingRDSWithExpiryOptions, ReplacementNU).
• go#344 / feat(frontend): chip-select component + global filter chips in topbar (#344 T1+T2) #345: SavingsPlanDetails.Currency field in pkg and Azure Savings Plan currency requirement; the CLI Azure pin is unchanged, no CLI behavior change.
• go#343: .gitallowed anchoring, tooling only.
• pkg also moves past earlier merged insurance changes (opaque insurance.Secret API key feat(cli): size purchases by target RI/SP utilization (--target-utilization) #338, decode errors no longer echo vendor values fix(email): unbreak factory_test.go after #333 merge glitch #337): archera client now wraps the key with insurance.NewSecret, and the decode-path archera tests assert that the key never appears instead of the withheld marker.

Follow-up (not in this PR): surface the Unusable warning in the end-of-run summary.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Improvements
  • RDS target-coverage recommendations now account for expiring reservations and eligible replacement reservations within the rebuy window, including across instance sizes and linked accounts.
  • Coverage lookback defaults to 30 days when the configured value is nonpositive.
  • Reports now summarize expiring capacity considered and replacement capacity credited, and warn when expiring reservations cannot be applied to family sizing.
• Tests
  • Added regression coverage for RDS sizing, expiry handling, warnings, and dry-run behavior.

Gate addendum: full pkg range in this bump (30bf383 -> 218912b, verified with git log)

providers/aws@218912b requires pkg@3838efd, so MVS forces the whole pkg range below; it cannot be split from the RDS fix. Commits the list above did not name:

• go#331 e8af37b: insurance keeps the API key out of every fmt-printable field (Client no longer stores the Config).
• go#334 7e217b1: exported contract-term list (moved to terms.go; same enum set).
• go#339 9dae64b: vendor-text sanitizing and key handling hardened. Behavior change visible in archera-comparison: an ARCHERA_API_KEY containing a non-printable or non-ASCII byte is now rejected before any request ("archera API key contains a non-printable or non-ASCII character at byte N"; the key is not echoed). Retry-After values past the int range now clamp to the 24h cap.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 7a45f7d2-da97-4553-9a74-6b2dbe76edb4

---

📥 Commits

Reviewing files that changed from the base of the PR and between 1cd8507 and f4eb6a0.

---

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

---

📒 Files selected for processing (9)

• cmd/archera_comparison.go
• cmd/archera_comparison_key_test.go
• cmd/archera_comparison_test.go
• cmd/multi_service.go
• cmd/multi_service_helpers.go
• cmd/rds_family_expiry_command_test.go
• cmd/recommendation_completeness_proxy_test.go
• cmd/recommendation_completeness_test.go
• go.mod

---

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

---

---

📝 Walkthrough

📝 Walkthrough

📝 Walkthrough

Walkthrough

RDS target-coverage sizing now uses expiry-aware family normalized-unit sizing with replacement-RI credits and shared coverage lookback handling. Command and completeness tests cover the related scenarios. Archera comparison passes its API key as a secret, and decode-error tests check that rejected values are not echoed. AWS dependencies were updated.

Changes

RDS Family Expiry Sizing

Layer / File(s)	Summary
Expiry-aware family sizing cmd/multi_service.go, cmd/multi_service_helpers.go, go.mod	The CLI uses a shared coverage lookback helper and calls expiry-aware RDS family sizing with expiring commitments, the rebuy window, and lookback days. It logs expiry and replacement-credit details. The AWS dependencies were updated.
Command-level expiry scenarios cmd/rds_family_expiry_command_test.go	Command tests cover expiry deductions, linked accounts and instance sizes, partial or unusable inventory, engine and window controls, and replacement lookback behavior. They check recommendations, coverage, warnings, output, requests, and that no purchase operation occurs.
Recommendation completeness test routing cmd/recommendation_completeness_test.go, cmd/recommendation_completeness_proxy_test.go	The completeness test maps the RDS expiry alias to RDS arguments and routes its proxy requests and CSV assertions through RDS-expiry-specific helpers.

Archera API Key and Decode Errors

Layer / File(s)	Summary
Secret construction and decode-error assertions cmd/archera_comparison.go, cmd/archera_comparison_key_test.go, cmd/archera_comparison_test.go	The Archera client receives the API key as an insurance.Secret. Decode-error tests check that rejected values do not expose readable key fragments and validate the revised error text.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant CostExplorer
  participant RDSInventory
  participant RDSFamilySizing
  participant Output
  CLI->>CostExplorer: Fetch existing coverage
  CLI->>RDSInventory: Fetch reserved-instance inventory
  CLI->>RDSFamilySizing: Pass expiring commitments and sizing windows
  RDSFamilySizing->>CLI: Return family recommendations and expiry details
  CLI->>Output: Report recommendations and expiry details

Loading

---

Merge Risk: ⚪ Minimal · up to f4eb6

No specific issue remains that calls for a change before merging. Normal checks should still be completed.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 8 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Issue #2132 requires expiry-aware RDS family sizing, CLI dry-run coverage, behavior isolation, and verification of the pinned SharedGo change. The PR calls ApplyFamilyNUSizingRDSWithExpiryOptions wi…
Out of Scope Changes check		The changed files remain connected to issue #2132. The Archera source and test updates adapt the CLI to dependency changes from the required SharedGo pin. The coverage helper centralizes the lookback …
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely identifies the main change: deducting expiring RIs from RDS family coverage during target-coverage sizing.

---

Full details: Docstring Coverage

Explanation

Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 8 files. (1 skipped: 1 unsupported.)

---

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

---

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent merge-gate review (Opus 5.5, claude-opus-5-5): CHANGES REQUESTED

Reviewed SHA: 9e3f9eb5128a7b65861f8f3104ac8c73d2b39f11 (base origin/main 1cd8507, up to date). Not merged.

All runs were local on macOS with go1.26.9, GOWORK=off, through the real root command and the recorded-HTTP completenessProxy harness. Fixtures are synthetic. No AWS calls were made and nothing was purchased.

Blocking

F1 (money path, over-purchase on re-run): a replacement RI the CLI already bought is ignored once the inventory is partial, so the next run buys again.

The RDS family pass now calls ApplyFamilyNUSizingRDSWithExpiry, which computes post-expiry coverage as max(C - E, min(C, A - E)). When other accounts own RIs in the family (cli#2131), A is small and the C - E branch wins. That branch ignores the account's own active, non-expiring RIs. Cost Explorer coverage C is a lookback average, so it does not yet reflect a replacement bought a few days ago.

Reproduced through the command path with a scratch scenario (dry run). Usage is 20 x db.r6g.large (80 NU) and lagged coverage is 82.5% (66 NU). The inventory holds 4 large expiring in 15 days plus the 4 large replacements bought 2 days ago, after the 24h idempotency window. Another account owns 12 large that the inventory does not list. True post-expiry coverage is 48 + 16 = 64 NU, which is 80%, so the gap is 0.

scenario	parent (cmd/multi_service_helpers.go from 1cd8507)	this PR
re-run, partial inventory	0 selected	Count 3 (db.r6g.large, upfront 300.00), log "deducting 16 normalized units"
re-run, full inventory (same plus the 12 other-account RIs listed)	0	0 (correct)
replacement payment-pending under 24h, partial	0	family sized 4, then checkDuplicates "4 instances → 0" (safe only inside --idempotency-window)

A daily scheduled run with --rebuy-window-days rebuys like this on each run until the coverage lookback catches up. RDS did not do this before this PR.

Suggested direction (design call for the plan owner; the fix belongs in cloud-commitments-go adjustFamilyCoverageForExpiry, followed by a pin bump): credit visible active, non-expiring RIs in the family that started recently as replacements already bought in the C - E branch. For example, max(0, max(min(C, C - E + R), min(C, A - E))), where R is the NU of those RIs. With that change the re-run case buys 0 and the existing partial-inventory case still buys 4. Add the re-run case as a regression. The EC2 per-pool path (adjustCoverageForExpiry) has the same re-run property even with full inventory. That behaviour already ships, so it should be a separate follow-up issue and not part of this PR.

Verified clean

• Call site (multi_service_helpers.go:651-654): passes coverageMap, cfg.TargetCoverage, the real GetExistingCommitments list and cfg.RebuyWindowDays. The library deducts from a fresh family aggregate, so the per-rec adjusted values are not reused, and annotateFamilyProjection overwrites them. Multi-size and linked-account rows give 2/4/2, so expiry is not deducted twice. The legacy caller at :474 passes nil commitments, which makes it identical to ApplyFamilyNUSizingRDS (the go#341 golden test covers this). Non-RDS recs pass through rest unchanged, and the EC2 TestReservationExpiryCommand passes.
• Regression tests reproduced (parent helper restored in a scratch copy): 6 of 8 fail and both controls pass.
  • already-at-target, large-ri-xlarge-usage, partial-inventory: "0 recommendations selected (0 via family-NU)", no CSV written.
  • below-target: expected "10", actual "8".
  • multi-size-linked-accounts: expected "2", actual "1".
  • unusable-expiring-warned: logs do not contain "2 RDS reserved instances expiring within 30 days could not be applied to family sizing".
  • All 8 pass at this head.
• Other over-purchase probes (scratch, at this head):
  • Underutilized full inventory (14 large owned, 10 running, 4 expiring): 0.
  • payment-pending replacement in full inventory: 0.
  • Retired RI: 0.
  • 100% coverage with a partial inventory: capped at 80%, buys 0.
  • Expiring NU greater than covered NU: floors at 0 and buys 16 (correct).
• No purchase reachable: the proxy fails on any unexpected operation and asserts that no PurchaseReservedDBInstancesOffering call was made. The runs are dry runs. Account IDs are built at runtime with strings.Repeat; there are no 12-digit literals in the new file.
• Unusable warning: the text is accurate and goes through AppLogger, which is the same path in dry and real runs. It does not appear in the CSV or in the end-of-run summary (non-blocking).
• PR body: says Closes #2132, states the deliberate RDS count and cost increase, and keeps cli#2131 and cli#2174 out of scope.
• Build, vet and lint: go build, go vet, go test -p 2 ./... and golangci-lint all pass at this head (lint: 0 issues). git ls-files -z | xargs -0 git secrets --scan with --register-aws is clean under BSD grep and GNU grep 3.12.
• CI and labels: CI is green at this SHA (CI Success, pre-commit, unit, integration) and the PR is CLEAN. Labels match cli#2132.

Non-blocking

• The log line "deducting N normalized units" prints the expiring NU, not the amount actually deducted. With the underutilized full inventory it reads 16 when 0 was deducted. Tracked in go#342.
• Mutation run (8 mutants on changed lines): 6 killed, 2 survived. Unusable > 1 survives because the only fixture uses 2. Disabling the ExpiringNU log line also survives because that line is not asserted.

[cristim]

Pushed f4eb6a0: merged origin/main (already up to date), bumped go pins to 218912bb, call site now uses ApplyFamilyNUSizingRDSWithExpiryOptions with the same lookback as GetRICoverageMap, added rerun-replacement-bought (fails on 9e3f9eb with count 3, passes now) and control-replacement-outside-lookback, log line reports expiring and credited NU. Archera client/tests adapted to the pkg bump. Unusable warning in the end-of-run summary left as follow-up. Ready for re-review.

[cristim]

Independent merge-gate review (Opus 5.5, claude-opus-5-5): APPROVED

Reviewed SHA: f4eb6a01f2977191d5d4b990783857ba03b26793 (base origin/main 1cd8507, head up to date with main). Previous gate F1 (at 9e3f9eb) is resolved. All runs from git archive of the exact SHA, GOTOOLCHAIN=go1.26.9, synthetic fixtures only, dry-run, no AWS, no purchases.

(1) Call site / one source of truth. applyCoverageAndOverrides (cmd/multi_service_helpers.go:652-657) calls ApplyFamilyNUSizingRDSWithExpiryOptions(recs, coverageMap, cfg.TargetCoverage, expiringCommitments, {WindowDays: cfg.RebuyWindowDays, CoverageLookbackDays: coverageLookbackDays(cfg)}); fetchExistingCoverage (cmd/multi_service.go:54) uses the same coverageLookbackDays(cfg) for GetRICoverageMap. expiringCommitments is the full owned inventory from GetExistingCommitments (active + payment-pending count as active in the library), so replacements are visible. Gate probe through the real command: --coverage-lookback-days 1 -> Cost Explorer TimePeriod is 1 day and the 2-day-old replacements are not credited (buys 3); --coverage-lookback-days 3 -> TimePeriod 3 days, credited (buys 0).

(2) F1 reproduced at CLI level. Scenario lagged coverage 82.5%, 4 large expiring, 4 replacements 2 days ago, 12 other-account large not in inventory: head = 0 purchased (no CSV, log "16 normalized units ... 16 units of replacement RIs ... credited back"); old head 9e3f9eb (with head's tests) = 3 large; head with the call reverted to CoverageLookbackDays: 0 = 3 large; control (replacements 45 days ago) = 3 at head. Extra over-purchase probes through the real command path, all as expected: payment-pending replacement 0; retired replacement 3 (not credited); underutilized family (20 non-expiring + 4 expiring large, 80%) 0; replacement in another engine 3; multi-AZ replacement vs single-AZ family 3; replacements beyond the expiring amount (8 vs 4) 0 (no negative effect); partial replacement (2 of 4) 1; multi-size + 3 linked-account rows re-run 0/0/0; unusable expiring RI plus replacement 0 with the warning. Multi-region is not drivable in this harness (single us-east-1 fixture); family keys carry region and commitments come from the regional client.

(3) Logging. Line reports expiring NU and credited ReplacementNU, no net-deduction claim; Unusable warning still emitted and asserted. End-of-run summary surfacing is listed as follow-up in the body.

(4) Archera scope. Not separable: providers/aws@218912b requires pkg@3838efd, which contains go#331/#334/#337/#338/#339/#345. The CLI change is the minimal insurance.NewSecret(key) adaptation (only APIKey construction site; grep). Test relaxation is legitimate: pkg now prints non-enum vendor values as "(N bytes, not shown)"; the CLI archeraSafeError detector still runs on every path. Own probe (1331 checks): long key, every path (HTTP with pads 0-199 across the 200-byte cut, decode, transport, deadline, canceled) x {exact, lower, upper, swapcase, fullwidth, 32-char windows and their swapcase (which match the pkg enum regex and ARE echoed by pkg), 8-char fragments split by ./_/-/0/x, every invisible separator, every key-alphabet separator}: no readable 8-char fragment, archeraKeyReadable false. Short key on decode: swapcase/lower now come back as "[message withheld: contained the API key]". Non-ASCII key: rejected with byte position, key not echoed. Non-blocking suggestion: the decode subtest in TestArcheraErrorsWithholdKeyHiddenBehindSurvivingCharacters now checks only the exact key; asserting archeraReadableKeyFragment there too would keep it as strict as the key test.

(5) Bump range. git log pkg 30bf383..218912b and providers/aws 2c1e87b..218912b: #331, #334, #337, #338, #339, #345 (pkg), #344, #347 (aws), #343 (tooling). The body missed #331, #334, #339; I appended a gate addendum to the PR body, including the #339 behaviour change (non-ASCII key rejected before any request).

(6) Hygiene. Pins are real pseudo-versions of go main commit 218912bb (UTC 20261012195142, ancestor of origin/main), no replace, no vendor; go mod tidy -diff clean, go mod verify all verified; build, vet OK; go test -p 2 ./... exit 0; golangci-lint 0 issues; git ls-files -z | xargs -0 git secrets --scan with --register-aws clean under BSD grep 2.6.0 and GNU grep 3.12 (account IDs built at runtime). CI green on the SHA incl. pre-commit; CodeRabbit no actionable; mergeStateStatus CLEAN; labels mirror #2132; Closes #2132.

Mutation (optional, one run, 13 mutants on changed lines, PR tests only): 8 killed. Non-blocking survivors: fetchExistingCoverage lookback hard-coded to 30 (decoupling the two uses; killed by my lookback-flag probe, not by the PR's tests; suggest a follow-up test asserting the CE TimePeriod with --coverage-lookback-days), default 30->29 and <=0-><0 (unreachable: flag default 30, validator >= 1), Unusable > 1 (fixture has 2), ExpiringNU >= 0 (log only).

Verdict: APPROVED at f4eb6a0.