Skip to content

Phoebe: make reconcile index EXPLAIN test robust (perf=soft, structural=hard) + dedupe#20

Closed
hhuuggoo wants to merge 1 commit into
yaml-rater-fixes-4from
yaml-rater-fixes-6
Closed

Phoebe: make reconcile index EXPLAIN test robust (perf=soft, structural=hard) + dedupe#20
hhuuggoo wants to merge 1 commit into
yaml-rater-fixes-4from
yaml-rater-fixes-6

Conversation

@hhuuggoo

Copy link
Copy Markdown
Contributor

Test-quality cleanup pass on the rating tests. The battery on PR #19 found zero production-code issues — every finding was about the EXPLAIN/index integration tests being fragile or redundant. This fixes those so the suite is robust.

Stacking note: intended to stack on yaml-rater-fixes-5, but that branch was merged (PR #19) and deleted while this work was in flight. PR #19 merged into yaml-rater-fixes-4 (merge commit 301e9f6), which now contains all of fixes-5's content. This PR is therefore based on and targets yaml-rater-fixes-4 — no fixes-5 content is lost (the trees were identical).

Contracts

NO production or contract change. Test-quality only. All three changes are in test files (internal/rating/store_integration_test.go, internal/rating/rater_test.go); no .go production source, schema, migration, or behavior was touched. The reconcile/rating semantics, log severities, exit codes, and the rated_usage contract are all unchanged. All pre-existing tests still pass.

Fixes

FIX 1 — TestIntegration_ReconcileDeleteUsesWindowStartIndex…CanUseWindowStartIndex: separate the structural gate from the perf signal.
The test conflated two claims: (a) the window_start index can serve the reconcile's window-only predicate (a structural/correctness invariant), and (b) the planner prefers it at default cost over the auth-leading composite (a performance assertion coupled to Postgres's cost model, fragile across PG majors / GUCs). A perf property whose failure mode is a slow reconcile — not a wrong bill — must not be a fragile correctness-style gate.

  • Removed the vacuous Seq Scan on rated_usage ru guard. Proven vacuous in the live plan: the priced WHERE-false CTE collapses to a One-Time Filter: false, so rated_usage's only access is the DELETE target; dropping the window_start index falls back to the composite index scan, never a literal Seq Scan — so the guard could never fire independently of the index assertion above it.
  • The default-cost check is now discriminating (asserts the index by NAME via the index-scan node, using rated_usage_window_start_ix, not a bare substring the composite would also match) but a SOFT signalt.Logf, never t.Fatalf.
  • The HARD gate (t.Fatalf) is the enable_seqscan=off force-the-index check: the structural proof the index can serve the predicate at all.
  • Renamed so the name claims only what is hard-asserted — the index is usable, not that the planner picks it.

FIX 2 — dedupe the explainPlan helper (fix-plus-cleanup).
TestIntegration_RatingInstantIndexServesScan had an inline EXPLAIN-row-scanning loop identical in intent to the explainPlan helper PR #19 introduced — same single-column scan, same plan-row concatenation, same rows.Err() handling, same plain EXPLAIN (no ANALYZE/FORMAT options). Confirmed no subtle difference, then swapped it to explainPlan and deleted the duplicated loop.

FIX 3 — correct an inaccurate docstring on TestRater_RoutineReconcileDeleteLogsError.
The docstring claimed it was RED before the fix because "testLogger discards INFO" — but this test uses captureLogger(), which buffers both the INFO and ERROR streams (it does not discard INFO; testLogger() is a different logger). Corrected to the actual mechanism: the assertion is on which severity the line fired at; the old always-INFO code left the captured ERROR stream empty, failing errBuf.Len()==0. (Test names/docstrings are claims — fixed per that rule.)

Gate

go build ./..., go vet ./... (+ -tags=integration), golangci-lint v1.64.8 (+ --build-tags=integration), gofmt -l . — all clean. go test -race ./... and PHOEBE_TEST_DATABASE_URL=… go test -tags=integration -race ./... (live PG 16) — all pass, including the e2e suite and the rebuilt index EXPLAIN tests (ran, not skipped). Verified the renamed test's soft-log fires and the live plan confirms the removed Seq-Scan guard was vacuous (One-Time Filter: false).

🤖 Generated with Claude Code

…ral=hard) + dedupe

Test-quality only; no production/contract change.

FIX 1 — TestIntegration_ReconcileDeleteUsesWindowStartIndex was conflating a
structural invariant (the window_start index CAN serve the reconcile's
window-only predicate) with a planner-cost preference (it is chosen at default
cost over the auth-leading composite). The latter couples the suite to
Postgres's cost model and flakes across PG majors / GUC changes; its failure
mode is a slow reconcile, not a wrong bill. So:
  - Removed the vacuous `Seq Scan on rated_usage ru` guard: the `priced`
    WHERE-false CTE collapses to a One-Time Filter, so rated_usage's only scan
    is the DELETE target; dropping the window_start index falls back to the
    composite index scan, never a literal Seq Scan — the guard could never fire
    independently.
  - Default-cost check is now discriminating (index by NAME via the index-scan
    node, "using rated_usage_window_start_ix", not a bare substring the
    composite would also match) but a SOFT signal (t.Logf, never t.Fatalf).
  - The HARD gate (t.Fatalf) is the enable_seqscan=off force-the-index check:
    the structural proof the index can serve the predicate at all.
  - Renamed to TestIntegration_ReconcileDeleteCanUseWindowStartIndex so the name
    claims only what is hard-asserted (the index is USABLE), not a perf choice.

FIX 2 — TestIntegration_RatingInstantIndexServesScan had an inline
EXPLAIN-row-scanning loop identical in intent to the explainPlan helper the PR
introduced (same single-column scan, same plan-row concatenation, same plain
EXPLAIN with no options). Swapped it to use explainPlan; deleted the duplicate.

FIX 3 — TestRater_RoutineReconcileDeleteLogsError's docstring claimed it was RED
because "testLogger discards INFO", but this test uses captureLogger() (buffers
BOTH streams). Corrected the docstring to the actual mechanism: the assertion is
on WHICH severity the line fired at; the old always-INFO code left the captured
ERROR stream empty.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@hhuuggoo

Copy link
Copy Markdown
Contributor Author

Landed on main via the squashed rating merge (3b22908). Closing the stack.

@hhuuggoo hhuuggoo closed this Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant