[IAmJSD]

Root cause

create_savings_snapshot_partition (000003) built the partition bounds as DATE values and passed them to FOR VALUES FROM (%L) TO (%L). The partition key savings_snapshots.timestamp is TIMESTAMPTZ, so Postgres read those literals in the session TimeZone. A Europe/Berlin session cut December at 2031-11-30 23:00Z, and an America/Los_Angeles session cut March at 08:00Z and April at 07:00Z (DST). When neighboring partitions are made in different zones, they leave gaps (rows land in savings_snapshots_default) or overlap (CREATE fails with "would overlap"). Since #900, either outcome makes the daily collection fail. create_future_savings_partitions also used CURRENT_DATE, which is the session's date, not the UTC date.

Fix

New migration 000132_savings_partition_utc_bounds:

• create_savings_snapshot_partition: the bounds are now explicit 'YYYY-MM-DD 00:00:00+00' literals. They parse the same under every TimeZone and DateStyle. DATE_TRUNC runs on ::timestamp, so no step reads the session zone.
• create_future_savings_partitions: uses DATE_TRUNC('month', now() AT TIME ZONE 'UTC') in place of CURRENT_DATE.
• Concurrency: the function now takes pg_advisory_xact_lock(hashtext('savings_snapshots_partition_ddl')) before its exists-check. A second caller for the same month waits for the first to commit, then sees the partition and returns. Before this change, the check-then-create race failed the second caller with relation "savings_snapshots_YYYY_MM" already exists (reproduced below). I fixed it here because this PR replaces the function anyway, and two concurrent callers for the same month are plausible (the collector's current-month ensure plus the future-partition sweep).
• Repair of existing partitions: a DO block takes the same advisory lock and LOCK TABLE savings_snapshots IN ACCESS EXCLUSIVE MODE, so no row can be written between the copy and the DROP. It then finds every savings_snapshots_YYYY_MM child whose bounds are not the UTC month its name says. For each one, it copies the rows into an ON COMMIT DROP temp table and drops the partition. It also moves out any default-partition rows that fall in the affected UTC months; otherwise the CREATE would fail on the default partition's constraint. It recreates the partitions with UTC bounds, adding one for each UTC month the copied rows fall in, so a spilled row does not land in default and block a later CREATE. Finally it re-inserts the rows through the parent. Column lists come from pg_attribute, not SELECT *. The "already aligned?" check renders the expected bounds with the same timestamptz output function that pg_get_expr uses, so it holds under any session TimeZone or DateStyle. Already-aligned partitions are not touched; the test checks that their OID is unchanged.
• Down: restores the 000003 function bodies verbatim. Partitions that were rebuilt keep their UTC bounds, because the zone each one was originally made in is not recorded.

Closes #912

Verification

Everything here is local integration evidence on a throwaway postgres:16-alpine testcontainer. No deployed database was checked. All test runs used AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's ~/.aws/config default profile is invalid.

New tests in internal/database/postgres/migrations/000132_savings_partition_utc_bounds_test.go (integration tag):

• TestMigration_SavingsPartitionUTCBounds:
  • At previousMigrationVersion(t, 132) it creates partitions with the old function under Berlin, UTC and Los Angeles sessions. It inserts rows at the month edges and asserts the bug: a UTC-November row sits in Berlin's December, a 2031-12-31T23:30Z row falls into savings_snapshots_default, and a UTC-April row sits in LA's March.
  • It then migrates to 132. The migrator runs with database defaults TimeZone=Asia/Kolkata and DateStyle='SQL, DMY'; SQL style renders the ambiguous abbreviation IST. The test asserts exact UTC bounds on all five affected months, that every row is in its UTC month's partition, that the row count is unchanged, and that the aligned January partition keeps its OID.
  • Subtests: (a) the new function under Berlin and LA produces UTC bounds, and the Berlin-after-UTC-neighbor case no longer overlaps; (b) create_future_savings_partitions under Pacific/Kiritimati (UTC+14) creates the current and next UTC months; (c) down restores the old behavior (Berlin bounds are back) and a second up repairs it again.
• TestMigration_SavingsPartitionConcurrentCreate: on real Postgres, session 1 creates the partition inside an open transaction. Session 2 calls the function for the same month, and the test waits until pg_stat_activity shows it blocked on a Lock. Session 1 then commits, and the test asserts that session 2 succeeds.

go test -tags integration -count=1 -v -run 'TestMigration_SavingsPartition' ./internal/database/postgres/migrations/

Fail-before (000132 up replaced with SELECT 1;):

Messages: partition savings_snapshots_2031_11 must exist
--- FAIL: TestMigration_SavingsPartitionUTCBounds
ERROR: relation "savings_snapshots_2036_02" already exists (SQLSTATE 42P07)
Messages: a concurrent caller for the same month must not fail
--- FAIL: TestMigration_SavingsPartitionConcurrentCreate

Pass-after: both tests and all three subtests pass.

Mutation probes (each applied alone to the up migration; each one is caught):

Mutant	Result
Function passes the DATE bounds again	function_cuts_UTC_months... FAIL (bounds not equal)
Function passes start_date::timestamp AT TIME ZONE 'UTC' through %L	migration FAIL: "savings_snapshots_2031_12" would overlap "savings_snapshots_2032_01" (IST re-parsed as Israel time under SQL, DMY)
Advisory lock removed from the function	ConcurrentCreate FAIL (already exists)
Default-row move removed from the repair	migration FAIL
Spill-month partitions not created	FAIL: savings_snapshots_2031_11 must exist
Repair block removed	FAIL

Not killable deterministically: reverting create_future_savings_partitions to CURRENT_DATE only shows up when the local date and the UTC date fall in different months, and the test cannot fake now(). The LOCK TABLE in the repair, which guards against concurrent writes during the migration, is also not exercised by a test.

Other checks run:

• go build ./...; go vet -tags integration ./internal/database/postgres/migrations/
• go test -count=1 ./internal/database/postgres/migrations/ (unit, incl. TestMigrationsAreTransactional)
• go test -tags integration -count=1 -run 'TestMigrations_DownAllRoundTrip|TestMigrations_FullStackIdempotent' ./internal/database/postgres/migrations/: PASS
• go test -tags integration -count=1 ./internal/analytics/: PASS
• go test ./internal/mocks/ -count=1: PASS
• golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/database/postgres/migrations/: 0 issues
• gocyclo -over 10 -ignore "_test\.go" internal/database/postgres/migrations/: silent; gofmt -l: clean; scripts/gosec-hook.sh: exit 0
• git-secrets (setup-git-secrets.sh patterns, fresh git init copy, isolated HOME) --scan: clean; scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL
• Migration-number duplicate check: 000132 is unused on main and in open PRs (the highest in open PRs is 000131, in feat(exchange): record an audit trail for direct-API RI exchange executes #1092)

Out of scope

• drop_old_savings_partitions (last redefined in 000074) still computes its cutoff from CURRENT_DATE, so near a month boundary a non-UTC session can move retention by one month. It only affects retention, not bounds, and is left for a follow-up.
• Default-partition rows outside the repaired months are left where they are. A later CREATE for such a month would still fail on the default constraint. This hazard existed before this change and is independent of time zones.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/bug, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1fb3a46d-805e-4d93-b91f-e49244e0e9eb

📥 Commits

Reviewing files that changed from the base of the PR and between 75219c0 and 5c6c936.

📒 Files selected for processing: 3

• internal/database/postgres/migrations/000132_savings_partition_utc_bounds.down.sql
• internal/database/postgres/migrations/000132_savings_partition_utc_bounds.up.sql
• internal/database/postgres/migrations/000132_savings_partition_utc_bounds_test.go

+--------------------------------------------+
| Like a moth to a flame, I'm drawn to bugs. |
+--------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• 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 review 1 (claude-fleet, cold): APPROVE 5c6c936

Findings (none blocking):

1. Merge order is enforced only by people. Nothing in the repo checks that migration numbers are contiguous or applied in order (details below).
2. Two disclosed gaps are untested.
   • Reverting create_future_savings_partitions to CURRENT_DATE passes every test. The local and UTC dates only differ near a month end, and the test cannot fake now().
   • Removing LOCK TABLE savings_snapshots IN ACCESS EXCLUSIVE MODE also passes, because no test writes concurrently during the repair.
   • Neither is wrong code. Taking "today" as a function argument would make the first testable, and that can be a follow-up.
3. Process nit. The head 5c6c936 is a merge of origin/main into the branch. A squash merge hides it, but it should be rebased before the review closes.

Stack and order:

• origin/main has migrations up to 000130, and 000131 is not on main.
• This PR is not stacked on feat(exchange): record an audit trail for direct-API RI exchange executes #1092: its merge-base 75219c0 is on origin/main, and feat(exchange): record an audit trail for direct-API RI exchange executes #1092's head is not an ancestor.
• feat(exchange): record an audit trail for direct-API RI exchange executes #1092 adds 131, this PR 132, and fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105 133. There are no duplicates and no file overlap.
• previousMigrationVersion(t, 132) resolves to the highest version below 132, so the test works with or without 131 present.
• If this merges before feat(exchange): record an audit trail for direct-API RI exchange executes #1092, golang-migrate Up only applies versions above the current one. Every database that auto-migrates to 132 would then skip 131 for good, and feat(exchange): record an audit trail for direct-API RI exchange executes #1092's audit table would be missing at runtime, with no test or CI catching it.
• The order must be strictly feat(exchange): record an audit trail for direct-API RI exchange executes #1092, then fix(analytics): cut savings_snapshots partitions at UTC month starts #1093, then fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105. This PR needs no rebase after feat(exchange): record an audit trail for direct-API RI exchange executes #1092 lands.

Up-down symmetry:

• Up replaces the two partition functions:
  • Bounds become explicit 'YYYY-MM-DD 00:00:00+00' literals.
  • DATE_TRUNC runs on ::timestamp, so the session zone is never read.
  • The future-partitions function uses now() AT TIME ZONE 'UTC' for "today".
  • A per-function advisory xact lock serializes partition DDL.
• A one-time DO repair then runs:
  • It takes the same advisory lock, then ACCESS EXCLUSIVE on the parent.
  • It copies each misaligned monthly partition into an ON COMMIT DROP temp table and drops the partition.
  • It moves default-partition rows for the affected UTC months into that table too.
  • It recreates the partitions with UTC bounds, plus any spill months, and re-inserts the rows through the parent.
• No session TimeZone dependence: the explicit literals and AT TIME ZONE 'UTC' make SET LOCAL TimeZone unnecessary. This holds under Kolkata with SQL, DMY and under Honolulu.
• No-op on an all-UTC database (the RDS, Azure and Cloud SQL default), apart from one brief exclusive lock.
• Locking: on a database whose zone was non-UTC, every partition is rewritten under ACCESS EXCLUSIVE in one migration transaction. That is acceptable at this size, but auto-migrate could block behind long dashboard reads.
• Transactional: TestMigrationsAreTransactional passes.
• Down restores the 000003 function bodies verbatim (diffed against 000003_analytics_partitions.up.sql:5-60). It leaves rebuilt partitions and their data alone, as the header documents. That is the right trade-off: the original zones were never recorded, and moving data back would recreate the bug.
• Idempotent: re-running up after down does not rebuild aligned partitions (the OID is unchanged).

Attacked: eight SQL mutation probes, each restored and confirmed identical with cmp:

• Caught:
  • The bound literal +00 changed to +01: "would overlap".
  • The aligned-bound comparison using session-zone ::timestamptz: "already-aligned partition must not be rebuilt".
  • The final re-insert skipped, which loses data.
  • The spill-month calculation without AT TIME ZONE 'UTC'.
  • The default-row move using the session zone: SQLSTATE 23514.
  • The down replaced by SELECT 1.
• Survived (finding 2): CURRENT_DATE in the future-partitions function, and LOCK TABLE removed.
• Also read:
  • The TO_DATE guard, the column list from pg_attribute, and the composite PK (id, timestamp) with no identity column, so the re-insert is safe.
  • The materialized views have no partition dependency.
  • Lock order: the functions and the repair both take the advisory lock before any table lock, so they cannot deadlock each other.

Verified locally, on a git archive export of HEAD:

• go vet -tags integration ./internal/database/postgres/migrations/: clean.
• go test -count=1 -tags integration -run 'TestMigration_SavingsPartition': both tests and all 3 subtests pass. TestMigrations_DownAllRoundTrip, TestMigrations_FullStackIdempotent and TestMigrationsAreTransactional also pass.
• Scratch case with the old function under Asia/Tokyo (UTC+9) and the migrator's database default TimeZone set to Pacific/Honolulu:
  • Rows at 23:30 and 00:30 local on both sides of the Dec/Jan and Jan/Feb boundaries, plus a November spill and an unrelated default-partition row.
  • After up, after down, and after a second up, every row is in the partition for its UTC month, and the row count is unchanged.
  • The unrelated row stays in the default partition, the November to January partitions have exact UTC bounds, and the rebuilt November partition has its index.

Reviewed by an independent agent with no author context (first reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 5c6c936

Independent adversarial review of #1093 (closes #912) at head 5c6c9369. The head is a merge of main 75219c07 into the branch commit e7b00d9e. The diff vs origin/main (8fa921f3) adds only the three 000132 files.

Findings (none blocking):

• merge-order - The PR is not stacked on feat(exchange): record an audit trail for direct-API RI exchange executes #1092; the two touch disjoint files. 000132 must land after feat(exchange): record an audit trail for direct-API RI exchange executes #1092's 000131. A database that reaches 132 first never applies 131 (golang-migrate only moves forward). Re-run the migrations integration tests on main after feat(exchange): record an audit trail for direct-API RI exchange executes #1092 merges, because this PR's CI ran on an older base. I ran the combined tree locally and it passes.
• low 000132_savings_partition_utc_bounds.up.sql:97 - The repair takes ACCESS EXCLUSIVE on savings_snapshots, which recurses to every partition, on every database.
  • On a database whose partitions are all aligned (sessions in UTC, the RDS default) it holds only for a catalog scan.
  • On a database whose server TimeZone was non-UTC, every monthly partition (24 of retention + 3 ahead) is misaligned. That database copies every row out and back (:128, :153) in one transaction, with analytics reads and writes blocked throughout and the migration context's deadline as the only bound.
  • Suggest noting the expected cost in the PR or the runbook. This is not a code change request.
• nit - The branch carries a merge commit; the repo convention is rebase, not merge. A squash merge flattens it.
• nit up.sql:97 - LOCK TABLE is not covered by any test: the mutant that removes it survives. The author disclosed this.
• nit - drop_old_savings_partitions (000074) still uses CURRENT_DATE. The author disclosed this; it affects retention only.

Correctness checks:

• Bounds: 'YYYY-MM-DD 00:00:00+00' literals parse the same under every zone and DateStyle.
• Aligned-check comparison: renders in the session zone on both sides, as pg_get_expr does.
• Default-row move and spill months: both are needed, and both are killed by probes.
• Row re-insert: uses explicit pg_attribute columns. The PK (id, timestamp) and the cloud_account_id FK still hold after rows move.
• Down: restores the 000003 bodies, which are the only prior definitions of both functions. Rebuilt bounds stay UTC (documented, harmless).
• Go caller: CreatePartition and CreateFuturePartitions (internal/analytics/postgres_analytics.go:458/470) are unchanged and compatible. The migration adds no new columns, so there is no NULL handling to check.

Attacked: mutation probes, run on real Postgres, each reverted:

• B1, remove the default-row move: killed.
• B2, aligned-check bounds in the session zone: killed (aligned January OID changes).
• B3, DATE bounds in the function: killed.
• B4, drop LOCK TABLE: survives (expected).
• B5, drop the function advisory lock: killed (ConcurrentCreate).
• B6, make down a no-op: killed.
• B7, skip spill months: killed.

Local runs (Docker testcontainers under the heavy lock):

• go build ./... and go vet -tags integration ./...: ok.
• Integration TestMigration_SavingsPartition* (both pass, no skips), DownAllRoundTrip and FullStackIdempotent: ok.
• go test -tags integration ./internal/analytics/: ok.
• Unit migrations: ok.
• Combined feat(exchange): record an audit trail for direct-API RI exchange executes #1092 + fix(analytics): cut savings_snapshots partitions at UTC month starts #1093 tree, migrations integration: ok.

[cristim]

Local verification at 5c6c9369fb20792430a3e932b574f19b1b4c6611 merged onto current main (ab8ab187, which now includes #1092's 000131; merge-tree 5bd7e54a; macOS with Docker testcontainers):

• go build ./... and go vet -tags integration ./... pass.
• go test -tags integration passes for ./internal/database/... (all migration tests, including the 000132 savings-partition UTC-bounds and concurrent-create tests, plus the down-all round trip and the full-stack idempotency test, now with 000131 present) and for ./internal/analytics/....
• 000132 applies after 000131, so the order stays feat(exchange): record an audit trail for direct-API RI exchange executes #1092, then fix(analytics): cut savings_snapshots partitions at UTC month starts #1093, then fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at 5c6c9369fb20792430a3e932b574f19b1b4c6611).