Skip to content

Phoebe: drop speculative resource_id index + document oracle NULL-modeling (battery cleanup)#23

Closed
hhuuggoo wants to merge 1 commit into
rated-usage-resource-idfrom
rated-usage-resource-id-cleanup
Closed

Phoebe: drop speculative resource_id index + document oracle NULL-modeling (battery cleanup)#23
hhuuggoo wants to merge 1 commit into
rated-usage-resource-idfrom
rated-usage-resource-id-cleanup

Conversation

@hhuuggoo

Copy link
Copy Markdown
Contributor

Battery cleanup (stacked on rated-usage-resource-id, PR #22). The battery found ZERO production bugs; this is a test/migration-only pass.

1. Drop the speculative (resource_id, window_start) index. Justified by "E2 reads rated_usage by deployment", but no such reader exists in this repo — it's the future Atlas/Stripe consumer. Per "forward intent without speculative build", don't pay write-amplification on every rated_usage upsert for an absent reader; the index lands in the PR that adds the E2 reader (with an EXPLAIN that proves it's used). Removed from 0002_rating.sql, the Alembic mirror (create in upgrade + drop in downgrade), and the integration-test fixture DDL. The resource_id column, the unique grain (auth_id, resource_id, model_id, window_start), and the auth_id / window_start indexes all stay.

2. Document the empty-string-models-NULL invariant at the oracle attribution check (rater_test.go) and the RatedEvent doc comment (oracle_test.go). "" models a NULL identity column; the oracle's == "" faithfully mirrors prod SQL's IS NULL only because production never lets a literal '' reach billing_event — the drainer's nullStr maps ''→NULL and the proxy billing gate fails closed on empty ResourceID before metering. Comment-only; the check logic is unchanged.

3. Tighten two SQL-shape assertions (store_test.go). A bare "AND resource_id IS NOT NULL" matched both the grouped/priced filter and the unpriced-count clause, so neither assertion pinned its intended clause. Anchored each to text unique to its clause so the unpriced-count's partition-exclusivity guard is genuinely verified.

Contracts

Removes an unused (no in-repo reader) (resource_id, window_start) index; no behavior change. Migrations are unapplied (no prod data); the rest is tests/comments only — no production .go logic touched.

🤖 Generated with Claude Code

…eling (battery cleanup)

Battery cleanup on the resource_id rollup-grain change (PR #22). The battery
found ZERO production bugs; this is a tiny test/migration-only pass.

1. Drop the speculative (resource_id, window_start) index. It was justified by
   "E2 reads rated_usage by deployment", but no such reader exists in this repo
   — it's the future Atlas/Stripe consumer. Per "forward intent without
   speculative build", don't pay write-amplification on every rated_usage upsert
   for an absent reader; the index lands in the PR that adds the E2 reader
   (with an EXPLAIN that proves it's used). Removed from both 0002_rating.sql and
   the Alembic mirror (create in upgrade + drop in downgrade), and from the
   integration-test fixture DDL so it still honestly mirrors production. The
   resource_id COLUMN, the unique grain (auth_id, resource_id, model_id,
   window_start), and the auth_id / window_start indexes all stay.

2. Document the empty-string-models-NULL invariant at the oracle. The Go oracle
   has no separate NULL, so "" stands in for a NULL identity column; prod SQL
   filters IS NULL. They agree ONLY because production never lets a literal ''
   reach billing_event — the drainer's nullStr maps ''→NULL and the proxy
   billing gate fails closed on empty ResourceID before metering. Documented at
   the oracle attribution check (rater_test.go) and the RatedEvent doc comment
   (oracle_test.go). Comment-only; the check logic is correct for the modeled
   domain and is unchanged.

3. Tighten two SQL-shape assertions (store_test.go). A bare
   "AND resource_id IS NOT NULL" matched BOTH the grouped/priced filter and the
   unpriced-count clause, so neither assertion actually pinned its intended
   clause (the unpriced count's resource_id guard could be dropped and the test
   would still pass). Anchored each to text unique to its clause (the grouped
   filter's GROUP BY; the unpriced count's full contiguous WHERE), so the
   partition-exclusivity guard is genuinely verified.

No production .go logic touched. Full gate green incl. live-PG integration +
e2e (TestIntegration_ResourceIDGrainAndFailClosed, ConformsToOracle, and the
unattributable/distinct-deployment tests all pass).

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

Copy link
Copy Markdown
Contributor Author

🔋 Re-battery — Round 1 (status: ESCALATE, but effectively DRY)

6 raw → 5 refuted → 1 confirmed (low, test-file). Zero production findings. The single finding is, by the verifier's own words, "NOT a mechanical must-fix for THIS PR" and "a pre-existing property... predating this PR": the integration-test fixture hand-maintains a DDL string that mirrors the migration with no programmatic parity assertion. The verifier confirms index drift cannot cause a false pass/fail of what the test proves (row-for-row oracle conformance is index-independent).

Calling this dry-enough and merging. Rationale (rigor-by-cost-of-being-wrong): three consecutive rounds found zero production issues; the only remaining item is a pre-existing test-robustness wishlist (fixture-vs-migration index parity) on a build-tagged integration test, for a drift class that can't affect money. Chasing it would be the over-engineering the standard's §7.1 warns against in the other direction — a battery that never converges because it can always surface a test nitpick. The resource_id grain change is correct, batteried, live-PG-verified.

Tracked, not dropped: the fixture/migration schema-parity gap is a real (pre-existing) improvement — captured in the decisions ledger as a future item, to fold into the PR that next touches the integration fixture (or the Atlas E2-reader PR that re-adds the resource_id index).

Merging #22#23 to main.

@hhuuggoo

Copy link
Copy Markdown
Contributor Author

Merged to main via the squashed resource_id grain change (e854bba).

@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