Skip to content

[codex] Persist provider quota refresh state#665

Open
Avilianb wants to merge 5 commits into
fawney19:mainfrom
Avilianb:codex/fix-pool-quota-refresh-persistence
Open

[codex] Persist provider quota refresh state#665
Avilianb wants to merge 5 commits into
fawney19:mainfrom
Avilianb:codex/fix-pool-quota-refresh-persistence

Conversation

@Avilianb

@Avilianb Avilianb commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Persist provider key quota refresh/runtime state when update_key writes SQLite/MySQL rows.
  • Cover the SQLite repository update path with quota/status fields so refresh data survives a list reload.

Root cause

persist_provider_quota_refresh_state() updates upstream_metadata, OAuth invalid markers, and status_snapshot on the in-memory key before calling the generic provider catalog key update. The SQLite/MySQL update_key SQL omitted those runtime columns, so the refresh response could briefly show quota progress, then the next list reload read the old row without quota windows and the progress bar disappeared.

Validation

  • cargo fmt --check
  • git diff --check
  • cargo test -p aether-data sqlite_repository_writes_provider_catalog_contract_views -- --nocapture
  • cargo test -p aether-data provider_catalog -- --nocapture

@Avilianb
Avilianb force-pushed the codex/fix-pool-quota-refresh-persistence branch from c0ae1a8 to cd2f1ab Compare July 18, 2026 23:05
@Avilianb
Avilianb marked this pull request as ready for review July 18, 2026 23:06
@Avilianb

Copy link
Copy Markdown
Contributor Author

Rebase complete — ready for CI

Rebased codex/fix-pool-quota-refresh-persistence onto latest origin/main and force-pushed with lease.

What was done

  • Rebase: clean rebase onto current main (no conflict markers; paths moved from repository/provider_catalog/{sqlite,mysql}.rs to adapters/{sqlite,mysql}/src/provider_catalog.rs in upstream layout)
  • Fix retained: update_key still persists quota/runtime columns on both SQLite and MySQL:
    • upstream_metadata
    • oauth_invalid_at / oauth_invalid_reason
    • status_snapshot
  • Local verification:
    • cargo test -p aether-data provider_catalog — 15 passed
    • cargo test -p aether-data-sqlite sqlite_repository_writes_provider_catalog_contract_views — passed
    • cargo fmt --check — clean

Prior CI note

Previous failure was only the unrelated flaky Gateway test (gateway_records_failed_usage_when_all_local_openai_chat_candidates_exhaust_after_retryable_sync_failure). Data-layer jobs were green. Re-running CI on rebased HEAD.

Ready for review.

@Avilianb

Copy link
Copy Markdown
Contributor Author

CI note (unrelated failures)

Post-rebase CI run 29664626456 failed on jobs not related to this PR's SQLite/MySQL update_key quota-persistence change:

  1. Data DB Smoke (Postgres)postgres_request_candidates_preserve_deleted_api_key_identity timed out waiting for local postgres: Connection refused (infra flake; migrations themselves passed).
  2. Test (Gateway)gateway_handles_admin_stats_leaderboard_api_keys_with_auth_snapshot_single_lookup_fallback assertion left: 0, right: 1 (admin stats leaderboard; unrelated to provider catalog UPDATE columns).

All data-layer jobs green: Format, Clippy (Data), Test (Data), all Data Adapter tests, Data Feature checks, MySQL/SQLite smoke.

Cannot gh run rerun (no admin on base repo). Pushing empty commit to re-trigger CI.

@Avilianb

Copy link
Copy Markdown
Contributor Author

Final babysit status

Done

  • Rebased onto latest main (clean; no conflict markers)
  • Force-with-lease pushed to fork head codex/fix-pool-quota-refresh-persistence (44bf8625)
  • Marked ready for review (undrafted)
  • Local verification green:
    • cargo test -p aether-data provider_catalog (15 passed)
    • cargo test -p aether-data-sqlite sqlite_repository_writes_provider_catalog_contract_views
    • cargo fmt --check

Fix still present after rebase

SQLite + MySQL update_key persist:

  • upstream_metadata
  • oauth_invalid_at / oauth_invalid_reason
  • status_snapshot

(Paths on current main: crates/aether-data/adapters/{sqlite,mysql}/src/provider_catalog.rs)

CI blockers (unrelated to this PR)

Both post-rebase runs failed only on:

  1. Data DB Smoke (Postgres) — local postgres connection refused timeout in API-key lifecycle smoke (also failing recently on main)
  2. Test (Gateway)gateway_handles_admin_stats_leaderboard_api_keys_with_auth_snapshot_single_lookup_fallback (left: 0, right: 1)

All data-layer jobs for this change are green (Format/Clippy/Test Data/adapters/feature checks/MySQL+SQLite smoke).

Merge

Could not merge: authenticated as Avilianb without MergePullRequest permission on fawney19/Aether. Maintainer merge needed once CI policy allows (or after base flakes are fixed).

PR is mergeable from git POV (no conflicts).

Main intentionally treats bulk auth snapshot list results as
authoritative for API key leaderboards so deleted historical IDs
do not trigger unbounded per-id N+1 lookups. Update the stale
single-lookup-fallback test to assert that default filtering hides
missing bulk rows and include_inactive surfaces legacy usage names.
@Avilianb

Copy link
Copy Markdown
Contributor Author

CI fix: Gateway admin stats leaderboard test

Root cause (Test Gateway)

origin/main commit 8fbda84a (fix(data): preserve API key history end to end) intentionally changed API-key leaderboard snapshot loading:

  • bulk list_auth_api_key_snapshots_by_ids is now authoritative
  • no per-id find fallback (avoids unbounded N+1 when historical aggregates contain many deleted key IDs)
  • missing bulk snapshots are filtered out under default filters; with include_inactive=true they surface via legacy usage names

The existing test gateway_handles_admin_stats_leaderboard_api_keys_with_auth_snapshot_single_lookup_fallback still expected the old single-lookup fallback (total == 1 / name fresh-key), so CI failed with left: 0, right: 1.

This is a main-branch regression in test expectations, not caused by this PR's quota refresh persistence changes.

Fix

Updated the gateway integration test to assert the intended product behavior:

  • default filters → empty leaderboard when bulk list returns no rows
  • include_inactive=true → historical identity via legacy usage name (legacy-key), without N+1 single lookups

Commit: 973fb603fix(gateway): align admin stats leaderboard bulk snapshot test

Data DB Smoke (Postgres)

postgres_request_candidates_preserve_deleted_api_key_identity failed with:

timed out waiting for local postgres: Connection refused (os error 111)

Same infra flake is failing on recent main runs. Not a code bug in this PR (no related SQL changes here). Documented only.

Local verification

  • cargo test -p aether-admin api_key_summary_leaderboard2 passed
  • Full aether-gateway binary could not be built locally on this Windows host (missing NASM for boring-sys2); CI Linux will run the updated gateway test.

@Avilianb

Copy link
Copy Markdown
Contributor Author

CI status after fix (973fb603)

Check Result
Test (Gateway) PASS (was the merge-blocking code failure)
Test / Clippy / Format / Data / Integration ✅ PASS
Data DB Smoke (Postgres) ❌ still failing — infra flake only

Postgres failure remains:

timed out waiting for local postgres: Connection refused (os error 111)

Same failure is present on recent main runs. Not introduced by this PR; no related SQL/code change in the fix.

Merge

Attempted gh pr merge 665 --squash --delete-branch as Avilianb → permission denied (does not have the correct permissions to execute MergePullRequest). Needs a maintainer with write access on fawney19/Aether (or admin override of the Postgres smoke required check if it is branch-protected).

ManagedPostgresServer now prefers AETHER_TEST_POSTGRES_URL when set and
creates an isolated database per fixture so sequential lifecycle tests
can share the GHA postgres:16 service without clobbering each other.
Wire that URL into the Postgres API key lifecycle CI step so CI no
longer depends on a local initdb/postgres child process that times out.
@Avilianb

Copy link
Copy Markdown
Contributor Author

Fix: Data DB Smoke (Postgres) lifecycle infra flake

Root cause

Job has a healthy postgres:16 service container and sets AETHER_TEST_POSTGRES_URL for migration/export smokes — those pass.

Step Run Postgres API key lifecycle tests only set AETHER_REQUIRE_LOCAL_POSTGRES_TESTS=true and did not pass AETHER_TEST_POSTGRES_URL. Those tests use ManagedPostgresServer::try_start(), which spun up a local initdb/postgres child. On GHA that times out (Connection refused), and with require-local enabled the skip path is disabled → hard fail. Same on recent main.

Fix (48b1325c)

  1. ManagedPostgresServer in crates/aether-data/runtime/src/lifecycle/migrate/tests.rs:
    • Prefer AETHER_TEST_POSTGRES_URL when set
    • Create an isolated DB per fixture (aether_migrate_{pid}_{uuid})
    • No local child process when using external URL
    • Drop isolated DB on cleanup (terminate backends + DROP DATABASE)
    • Bump wait_for_postgres timeout 10s → 30s for local path
  2. Workflow: pass AETHER_TEST_POSTGRES_URL: postgres://aether:aether@127.0.0.1:5432/aether_test into the lifecycle step (keep require-local so missing URL/local still fails in CI)

Local: cargo check -p aether-data --tests --features all-drivers

Default admin stats windows are calendar-day based. Using now-10m for
recent fixtures can land on yesterday right after UTC midnight and
empty the default 1-day window. Clamp recent fixtures to stay inside
today's UTC bounds.
@Avilianb

Copy link
Copy Markdown
Contributor Author

Follow-up: Gateway UTC-midnight flake

Postgres smoke is green on 48b1325c. Gateway then failed on a different test:
gateway_defaults_admin_stats_leaderboard_models_to_bounded_recent_window_when_query_missing

Cause

Default admin stats window is calendar today (1 day when env is test/dev). The test used now - 10 minutes for the recent row. CI ran at ~00:07 UTC, so that timestamp fell on yesterday and the default window returned empty (total: 0).

Fix (a085cb8e)

Clamp recent fixtures to stay inside today's UTC day (recent_within_default_window_unix_secs) for the models leaderboard and error-distribution default-window tests.

@Avilianb

Copy link
Copy Markdown
Contributor Author

CI fully green on a085cb8e

All required checks passed, including:

  • Data DB Smoke (Postgres) (was the service-container URL / local initdb flake)
  • Test (Gateway) (UTC-midnight default window flake fixed)
  • ✅ Format / Clippy / Test / Data / Integration / check

Merge attempted as Avilianb — will report if permission is still denied.

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