feat: Gmail OAuth token persistence in Postgres #163

Merged

forgejo_admin merged 2 commits from 130-spike-gmail-oauth-token-persistence into main 2026-03-26 03:36:43 +00:00

Conversation 2 Commits 2 Files changed 8 +576 -11

forgejo_admin commented 2026-03-24 21:14:34 +00:00

Owner

Copy link

Summary

Gmail OAuth tokens were stored on the filesystem via gmail-sdk, which breaks in k8s with read-only secret mounts. Access tokens expire ~60min and refreshed tokens were lost on pod restart, causing a ~29-hour undetected email outage (2026-03-19 to 2026-03-21). This adds a Postgres-backed token store that loads, refreshes, and persists tokens via an oauth_tokens table, eliminating the writable filesystem dependency.

Changes

• src/basketball_api/models.py -- Added OAuthToken model (provider, account, token_data JSONB, expires_at, timestamps) with unique constraint on (provider, account)
• src/basketball_api/services/token_store.py -- New Postgres-backed token store with load_token, save_token, get_access_token (refresh + persist flow), and seed_from_file (one-time migration)
• src/basketball_api/services/email.py -- Updated get_gmail_client() to accept optional db param; when provided, loads token from DB via token store with fallback to file-based. Updated all 7 email send functions to pass db through.
• src/basketball_api/routes/password_reset.py -- Pass db to send_password_reset_email call
• alembic/env.py -- Import OAuthToken for migration awareness
• alembic/versions/019_add_oauth_tokens_table.py -- New migration creating oauth_tokens table
• scripts/seed_oauth_token.py -- New one-time seed script to import file-based tokens into DB
• tests/test_token_store.py -- New 16 tests covering save/load, upsert, refresh flow, expiry detection, refresh token preservation, seed from file, and get_gmail_client integration

Test Plan

• 534 existing tests pass (no regressions)
• 16 new tests for token store (save/load, upsert, refresh, seed, integration)
• Deploy to k8s, run seed script, verify email sends survive pod restart

Review Checklist

• ruff format and ruff check pass
• All tests pass (534 total)
• Migration is reversible (downgrade drops table)
• Backwards compatible -- falls back to file-based when no DB token exists
• No new dependencies added (uses existing httpx, sqlalchemy)

Spike findings

gmail-sdk token storage: AuthMixin._load_and_refresh_token() reads from file, refreshes via Google if within 300s of expiry, writes back. No pluggable interface. Solution: bypass by passing access_token= directly to GmailClient().

Schema: New oauth_tokens table rather than JSONB on tenants -- cleaner separation, supports multiple providers. No encryption at rest -- Postgres auth + k8s network policy matches current k8s secret mount security posture.

Refresh flow: Proactive 5-min buffer, last-write-wins concurrency (single replica), graceful fallback on DB write failure.

Migration path: (1) Deploy this PR, (2) Run python scripts/seed_oauth_token.py marcus, (3) Remove init container + volume mounts from pal-e-deployments.

Cross-repo: Follow-up needed in pal-e-deployments to remove copy-gmail-oauth init container and gmail-oauth-token secret volume.

Related

• Closes #130
• basketball-api #129 -- Enterprise login (depends on working email)
• pal-e-platform #122 -- Remove SendGrid

## Summary Gmail OAuth tokens were stored on the filesystem via gmail-sdk, which breaks in k8s with read-only secret mounts. Access tokens expire ~60min and refreshed tokens were lost on pod restart, causing a ~29-hour undetected email outage (2026-03-19 to 2026-03-21). This adds a Postgres-backed token store that loads, refreshes, and persists tokens via an `oauth_tokens` table, eliminating the writable filesystem dependency. ## Changes - `src/basketball_api/models.py` -- Added `OAuthToken` model (provider, account, token_data JSONB, expires_at, timestamps) with unique constraint on (provider, account) - `src/basketball_api/services/token_store.py` -- New Postgres-backed token store with `load_token`, `save_token`, `get_access_token` (refresh + persist flow), and `seed_from_file` (one-time migration) - `src/basketball_api/services/email.py` -- Updated `get_gmail_client()` to accept optional `db` param; when provided, loads token from DB via token store with fallback to file-based. Updated all 7 email send functions to pass `db` through. - `src/basketball_api/routes/password_reset.py` -- Pass `db` to `send_password_reset_email` call - `alembic/env.py` -- Import `OAuthToken` for migration awareness - `alembic/versions/019_add_oauth_tokens_table.py` -- New migration creating `oauth_tokens` table - `scripts/seed_oauth_token.py` -- New one-time seed script to import file-based tokens into DB - `tests/test_token_store.py` -- New 16 tests covering save/load, upsert, refresh flow, expiry detection, refresh token preservation, seed from file, and get_gmail_client integration ## Test Plan - [x] 534 existing tests pass (no regressions) - [x] 16 new tests for token store (save/load, upsert, refresh, seed, integration) - [ ] Deploy to k8s, run seed script, verify email sends survive pod restart ## Review Checklist - [x] ruff format and ruff check pass - [x] All tests pass (534 total) - [x] Migration is reversible (downgrade drops table) - [x] Backwards compatible -- falls back to file-based when no DB token exists - [x] No new dependencies added (uses existing httpx, sqlalchemy) ### Spike findings **gmail-sdk token storage**: `AuthMixin._load_and_refresh_token()` reads from file, refreshes via Google if within 300s of expiry, writes back. No pluggable interface. Solution: bypass by passing `access_token=` directly to `GmailClient()`. **Schema**: New `oauth_tokens` table rather than JSONB on `tenants` -- cleaner separation, supports multiple providers. No encryption at rest -- Postgres auth + k8s network policy matches current k8s secret mount security posture. **Refresh flow**: Proactive 5-min buffer, last-write-wins concurrency (single replica), graceful fallback on DB write failure. **Migration path**: (1) Deploy this PR, (2) Run `python scripts/seed_oauth_token.py marcus`, (3) Remove init container + volume mounts from pal-e-deployments. **Cross-repo**: Follow-up needed in `pal-e-deployments` to remove `copy-gmail-oauth` init container and `gmail-oauth-token` secret volume. ## Related - Closes #130 - basketball-api #129 -- Enterprise login (depends on working email) - pal-e-platform #122 -- Remove SendGrid

forgejo_admin added 1 commit 2026-03-24 21:14:34 +00:00

feat: persist Gmail OAuth tokens in Postgres for pod-restart resilience ... de2ef2b0e1

Gmail OAuth tokens were stored on the filesystem via gmail-sdk, which
breaks in k8s with read-only secret mounts. Access tokens expire ~60min
and refreshed tokens were lost on pod restart, causing a ~29-hour email
outage. This adds a Postgres-backed token store that loads/refreshes/
persists tokens via an oauth_tokens table, eliminating the writable
filesystem dependency.

Closes #130

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin referenced this pull request 2026-03-24 21:23:35 +00:00

feat: email verification before payment #162

forgejo_admin referenced this pull request 2026-03-24 21:45:36 +00:00

feat: email verification before payment #162

forgejo_admin referenced this pull request 2026-03-24 22:36:08 +00:00

feat: add custom_notes column to players table #165

forgejo_admin referenced this pull request 2026-03-25 00:18:37 +00:00

feat: email verification before payment #162

forgejo_admin commented 2026-03-25 00:19:44 +00:00

Author

Owner

Copy link

PR #163 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / httpx / gmail-sdk

This PR solves a real production incident (29-hour email outage from lost OAuth tokens on pod restart) with a clean Postgres-backed token store. The architecture is sound: bypass gmail-sdk's file-based storage by refreshing tokens in-process via Google's token endpoint and passing access_token= directly to GmailClient().

Positive observations:

1. Clean separation -- OAuthToken model is its own table rather than JSONB on tenants. Correct choice for multi-provider extensibility.
2. Backwards compatibility -- db: Session | None = None on get_gmail_client and send_password_reset_email means all existing call sites (tests, routes that don't pass db) continue working via file-based fallback.
3. Proactive refresh -- 5-minute buffer matching gmail-sdk's 300s convention. Avoids mid-request expiry.
4. Refresh token preservation -- Lines 147-148 in token_store.py correctly preserve the existing refresh_token when Google doesn't return a new one (standard Google behavior).
5. Graceful DB write failure -- Lines 153-161: if persisting the refreshed token to Postgres fails, the valid in-memory access_token is still returned. Smart resilience.
6. Well-structured tests -- 16 new tests cover save/load, upsert, expired/expiring-soon refresh, refresh token preservation vs. rotation, seed from file, missing file, and get_gmail_client integration wiring.

Domain-specific findings:

• SQLAlchemy: Session usage is correct. db.commit() + db.refresh() in save_token is the standard pattern. JSONB column typed properly with Mapped[dict].
• PEP compliance: Type hints present on all functions. Docstrings present. Module-level docstring on token_store.py is thorough.
• httpx usage: resp.raise_for_status() in _refresh_access_token correctly bubbles HTTP errors. The caller in get_access_token catches httpx.HTTPStatusError, re-raises after logging.
• Security posture: Token data stored as JSONB (not encrypted at rest). The PR body explicitly justifies this -- Postgres auth + k8s network policy matches current k8s secret mount security level. Acceptable for current deployment model.

BLOCKERS

1. Migration chain conflict (BLOCKER)

Migration 019 has down_revision = "018", but migration 020_add_custom_notes_to_player.py ALSO has down_revision = "018". This creates two Alembic heads. Running alembic upgrade head will fail with:

alembic.util.exc.CommandError: Multiple head revisions are present

Fix: Migration 020 must be updated to down_revision = "019" so the chain is 018 -> 019 -> 020. This is a merge ordering issue -- PR #165 (custom_notes) apparently merged before this PR.

2. alembic/env.py missing other model imports (BLOCKER)

The current alembic/env.py on main imports only Coach, Outbox, Parent, Player, Registration, Tenant. The PR adds OAuthToken but does NOT add many other models that exist in models.py: Team, EmailLog, Product, Order, PasswordResetToken, Outbox. While this is a pre-existing issue (not introduced by this PR), the migration chain conflict in item 1 means this file will need to be touched anyway, and the PR should not make the problem worse. The OAuthToken import is correct for this PR's scope.

Downgrading item 2 to NIT -- the missing imports are a pre-existing issue. The PR correctly adds its own model import. However, item 1 (migration chain conflict) remains a BLOCKER.

NITS

1. Lazy import in get_gmail_client (line 27 of diff for email.py): from basketball_api.services.token_store import get_access_token is imported inside the function body. This avoids a circular import, which is fine, but a module-level comment explaining why would help future readers. Minor.

2. _load_credentials error handling: If credentials.json is malformed JSON, json.load() will raise json.JSONDecodeError with a generic message. A more descriptive error wrapping the path would help debugging in production. Minor.

3. seed_oauth_token.py has no error handling for DB connection: If BASKETBALL_DATABASE_URL is misconfigured, the error will be a raw SQLAlchemy exception. Since this is a one-time migration script, acceptable but could be friendlier.

4. alembic/env.py model imports are incomplete (pre-existing): Team, EmailLog, Product, Order, PasswordResetToken are missing from the import list. Not introduced by this PR, but worth a follow-up cleanup issue.

5. Test coverage for _refresh_access_token and _load_credentials: These are tested indirectly through mocks, which is fine for unit tests. However, the _load_credentials function handles both installed and web credential formats -- only the mock path is exercised. A unit test for _load_credentials with both formats and the error case would strengthen coverage. Non-blocking.

SOP COMPLIANCE

• Branch named after issue: 130-spike-gmail-oauth-token-persistence matches issue #130
• PR body has: Summary, Changes, Test Plan, Related -- all present and thorough
• Related references parent issue: "Closes #130" with cross-references to #129 and pal-e-platform #122
• No related plan slug -- this is a spike issue, not plan-based work. Acceptable.
• Tests exist: 16 new tests covering happy path, edge cases, and error handling
• No secrets committed: token fixtures use dummy values (ya29.valid-access-token, 1//valid-refresh-token)
• No unnecessary file changes: all 8 files are directly related to the feature
• Commit messages: single commit implied by the PR structure

PROCESS OBSERVATIONS

• DORA impact: This directly improves MTTR -- the 29-hour email outage was caused by lost tokens on pod restart. Postgres persistence eliminates this failure mode entirely.
• Change failure risk: LOW. The backwards-compatible fallback to file-based tokens means this can be deployed before the seed script runs without breaking email.
• Cross-repo dependency: The PR body correctly identifies follow-up work in pal-e-deployments to remove the init container and volume mount. This should be tracked as a Forgejo issue.
• Migration conflict: The 019/020 collision must be resolved before merge. This is likely a rebase issue from PR #165 merging first.

VERDICT: NOT APPROVED

One blocker: the Alembic migration chain conflict (019 and 020 both claim down_revision = "018"). Fix this by rebasing and updating 020 to depend on 019, or renumber accordingly. Once the migration chain is linear, this PR is ready to merge.

## PR #163 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic / httpx / gmail-sdk This PR solves a real production incident (29-hour email outage from lost OAuth tokens on pod restart) with a clean Postgres-backed token store. The architecture is sound: bypass gmail-sdk's file-based storage by refreshing tokens in-process via Google's token endpoint and passing `access_token=` directly to `GmailClient()`. **Positive observations:** 1. **Clean separation** -- `OAuthToken` model is its own table rather than JSONB on tenants. Correct choice for multi-provider extensibility. 2. **Backwards compatibility** -- `db: Session | None = None` on `get_gmail_client` and `send_password_reset_email` means all existing call sites (tests, routes that don't pass `db`) continue working via file-based fallback. 3. **Proactive refresh** -- 5-minute buffer matching gmail-sdk's 300s convention. Avoids mid-request expiry. 4. **Refresh token preservation** -- Lines 147-148 in `token_store.py` correctly preserve the existing refresh_token when Google doesn't return a new one (standard Google behavior). 5. **Graceful DB write failure** -- Lines 153-161: if persisting the refreshed token to Postgres fails, the valid in-memory access_token is still returned. Smart resilience. 6. **Well-structured tests** -- 16 new tests cover save/load, upsert, expired/expiring-soon refresh, refresh token preservation vs. rotation, seed from file, missing file, and `get_gmail_client` integration wiring. **Domain-specific findings:** - **SQLAlchemy**: Session usage is correct. `db.commit()` + `db.refresh()` in `save_token` is the standard pattern. JSONB column typed properly with `Mapped[dict]`. - **PEP compliance**: Type hints present on all functions. Docstrings present. Module-level docstring on `token_store.py` is thorough. - **httpx usage**: `resp.raise_for_status()` in `_refresh_access_token` correctly bubbles HTTP errors. The caller in `get_access_token` catches `httpx.HTTPStatusError`, re-raises after logging. - **Security posture**: Token data stored as JSONB (not encrypted at rest). The PR body explicitly justifies this -- Postgres auth + k8s network policy matches current k8s secret mount security level. Acceptable for current deployment model. ### BLOCKERS **1. Migration chain conflict (BLOCKER)** Migration `019` has `down_revision = "018"`, but migration `020_add_custom_notes_to_player.py` ALSO has `down_revision = "018"`. This creates two Alembic heads. Running `alembic upgrade head` will fail with: ``` alembic.util.exc.CommandError: Multiple head revisions are present ``` Fix: Migration 020 must be updated to `down_revision = "019"` so the chain is `018 -> 019 -> 020`. This is a merge ordering issue -- PR #165 (custom_notes) apparently merged before this PR. **2. `alembic/env.py` missing other model imports (BLOCKER)** The current `alembic/env.py` on main imports only `Coach, Outbox, Parent, Player, Registration, Tenant`. The PR adds `OAuthToken` but does NOT add many other models that exist in `models.py`: `Team, EmailLog, Product, Order, PasswordResetToken, Outbox`. While this is a pre-existing issue (not introduced by this PR), the migration chain conflict in item 1 means this file will need to be touched anyway, and the PR should not make the problem worse. The `OAuthToken` import is correct for this PR's scope. **Downgrading item 2 to NIT** -- the missing imports are a pre-existing issue. The PR correctly adds its own model import. However, item 1 (migration chain conflict) remains a BLOCKER. ### NITS 1. **Lazy import in `get_gmail_client`** (line 27 of diff for `email.py`): `from basketball_api.services.token_store import get_access_token` is imported inside the function body. This avoids a circular import, which is fine, but a module-level comment explaining why would help future readers. Minor. 2. **`_load_credentials` error handling**: If `credentials.json` is malformed JSON, `json.load()` will raise `json.JSONDecodeError` with a generic message. A more descriptive error wrapping the path would help debugging in production. Minor. 3. **`seed_oauth_token.py` has no error handling for DB connection**: If `BASKETBALL_DATABASE_URL` is misconfigured, the error will be a raw SQLAlchemy exception. Since this is a one-time migration script, acceptable but could be friendlier. 4. **`alembic/env.py` model imports are incomplete** (pre-existing): `Team, EmailLog, Product, Order, PasswordResetToken` are missing from the import list. Not introduced by this PR, but worth a follow-up cleanup issue. 5. **Test coverage for `_refresh_access_token` and `_load_credentials`**: These are tested indirectly through mocks, which is fine for unit tests. However, the `_load_credentials` function handles both `installed` and `web` credential formats -- only the mock path is exercised. A unit test for `_load_credentials` with both formats and the error case would strengthen coverage. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `130-spike-gmail-oauth-token-persistence` matches issue #130 - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and thorough - [x] Related references parent issue: "Closes #130" with cross-references to #129 and pal-e-platform #122 - [x] No related plan slug -- this is a spike issue, not plan-based work. Acceptable. - [x] Tests exist: 16 new tests covering happy path, edge cases, and error handling - [x] No secrets committed: token fixtures use dummy values (`ya29.valid-access-token`, `1//valid-refresh-token`) - [x] No unnecessary file changes: all 8 files are directly related to the feature - [x] Commit messages: single commit implied by the PR structure ### PROCESS OBSERVATIONS - **DORA impact**: This directly improves MTTR -- the 29-hour email outage was caused by lost tokens on pod restart. Postgres persistence eliminates this failure mode entirely. - **Change failure risk**: LOW. The backwards-compatible fallback to file-based tokens means this can be deployed before the seed script runs without breaking email. - **Cross-repo dependency**: The PR body correctly identifies follow-up work in `pal-e-deployments` to remove the init container and volume mount. This should be tracked as a Forgejo issue. - **Migration conflict**: The `019`/`020` collision must be resolved before merge. This is likely a rebase issue from PR #165 merging first. ### VERDICT: NOT APPROVED One blocker: the Alembic migration chain conflict (`019` and `020` both claim `down_revision = "018"`). Fix this by rebasing and updating `020` to depend on `019`, or renumber accordingly. Once the migration chain is linear, this PR is ready to merge.

forgejo_admin force-pushed 130-spike-gmail-oauth-token-persistence from de2ef2b0e1 to 6876db7f59 2026-03-25 00:34:09 +00:00 Compare

forgejo_admin commented 2026-03-25 00:37:36 +00:00

Author

Owner

Copy link

PR #163 Re-Review

Re-review after previous blocker: migration chain conflict (019 and existing 020 both had down_revision="018"). Fix was to rebase onto main and renumber migration to 021 with down_revision="020".

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Alembic / httpx

Migration chain verification (primary focus of re-review):

The PR migration 021_add_oauth_tokens_table.py now has revision = "021" and down_revision = "020". Verified against main's migration chain:

• 018 (groupme_and_outbox)
• 019 (player_teams_junction, PR #164, down_revision = "018")
• 020 (custom_notes, PR #165, down_revision = "018")

Result: The original blocker (019/020 collision with the old 019 oauth migration) is RESOLVED. Migration 021 chains off 020 correctly.

Pre-existing issue on main (not introduced by this PR): Both 019 and 020 on main have down_revision = "018", creating a forked Alembic history with two heads. This means alembic upgrade head will fail until a merge migration is created. This is a pre-existing problem from PRs #164 and #165 merging without chain coordination. This PR does NOT introduce or worsen this issue -- it simply extends one of the two existing heads. A separate issue should track the merge migration fix.

Token store (token_store.py): Clean implementation. Proactive 5-minute refresh buffer matches gmail-sdk behavior. Graceful degradation on DB write failure after successful Google refresh. Last-write-wins concurrency is appropriate for single-replica Recreate strategy.

Email integration (email.py): All 7 send_* functions updated to pass db through to get_gmail_client. Backwards-compatible -- db=None falls through to file-based tokens. send_password_reset_email adds db: Session | None = None as optional param; route caller updated.

Model (models.py): OAuthToken uses proper mapped columns, JSONB for token_data, UniqueConstraint on (provider, account), server_default timestamps.

Alembic env.py: OAuthToken import added for autogenerate awareness. Correct.

Seed script (scripts/seed_oauth_token.py): One-time migration utility. Proper error handling (missing args, file not found). Uses SessionLocal directly (appropriate for scripts outside request lifecycle).

BLOCKERS

None. The migration chain blocker from the previous review is resolved. Migration 021 correctly descends from 020.

NITS

1. DB write failure path untested: The except Exception block in get_access_token (lines ~155-161 of token_store.py) that logs but continues after a DB write failure is not covered by tests. Low risk since it is a defensive fallback, but a test would confirm the access_token is still returned when save_token raises.

2. Pre-existing Alembic fork: Main has two heads (019 and 020 both descend from 018). A merge migration is needed. This should be tracked as a separate issue -- it predates this PR and is not within scope.

3. Inline import: from basketball_api.services.token_store import get_access_token is imported inside get_gmail_client rather than at module top. This avoids circular imports, which is a valid reason, but a comment explaining the deferred import would help future readers.

SOP COMPLIANCE

• Branch named after issue: 130-spike-gmail-oauth-token-persistence matches issue #130
• PR body has: Summary, Changes, Test Plan, Related -- all present and detailed
• Related references: Closes #130, references #129 and platform #122
• Tests exist: 16 new tests covering save/load, upsert, refresh flow, expiry, seed, integration
• No secrets committed: token values in tests are dummy strings (ya29.valid-access-token, etc.)
• No unnecessary file changes: all 8 changed files are directly related to the feature
• Commit messages: not visible in diff, PR title is descriptive

PROCESS OBSERVATIONS

• Deployment Frequency: Clean migration + backwards-compatible fallback means this can deploy without coordination. Seed script is a manual post-deploy step (documented in PR body).
• Change Failure Risk: Low. Fallback to file-based tokens means a broken DB token path does not cause email outage. The current 29-hour outage scenario (token refresh lost on pod restart) is directly addressed.
• Follow-up needed: (1) Pre-existing Alembic fork needs a merge migration (separate issue). (2) pal-e-deployments needs init container + volume mount removal after seed script runs (documented in PR).

VERDICT: APPROVED

## PR #163 Re-Review Re-review after previous blocker: migration chain conflict (019 and existing 020 both had `down_revision="018"`). Fix was to rebase onto main and renumber migration to 021 with `down_revision="020"`. ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Alembic / httpx **Migration chain verification (primary focus of re-review)**: The PR migration `021_add_oauth_tokens_table.py` now has `revision = "021"` and `down_revision = "020"`. Verified against main's migration chain: - 018 (groupme_and_outbox) - 019 (player_teams_junction, PR #164, `down_revision = "018"`) - 020 (custom_notes, PR #165, `down_revision = "018"`) **Result**: The original blocker (019/020 collision with the old 019 oauth migration) is RESOLVED. Migration 021 chains off 020 correctly. **Pre-existing issue on main (not introduced by this PR)**: Both 019 and 020 on main have `down_revision = "018"`, creating a forked Alembic history with two heads. This means `alembic upgrade head` will fail until a merge migration is created. This is a pre-existing problem from PRs #164 and #165 merging without chain coordination. This PR does NOT introduce or worsen this issue -- it simply extends one of the two existing heads. A separate issue should track the merge migration fix. **Token store** (`token_store.py`): Clean implementation. Proactive 5-minute refresh buffer matches gmail-sdk behavior. Graceful degradation on DB write failure after successful Google refresh. Last-write-wins concurrency is appropriate for single-replica Recreate strategy. **Email integration** (`email.py`): All 7 `send_*` functions updated to pass `db` through to `get_gmail_client`. Backwards-compatible -- `db=None` falls through to file-based tokens. `send_password_reset_email` adds `db: Session | None = None` as optional param; route caller updated. **Model** (`models.py`): `OAuthToken` uses proper mapped columns, JSONB for token_data, UniqueConstraint on (provider, account), server_default timestamps. **Alembic env.py**: `OAuthToken` import added for autogenerate awareness. Correct. **Seed script** (`scripts/seed_oauth_token.py`): One-time migration utility. Proper error handling (missing args, file not found). Uses `SessionLocal` directly (appropriate for scripts outside request lifecycle). ### BLOCKERS None. The migration chain blocker from the previous review is resolved. Migration 021 correctly descends from 020. ### NITS 1. **DB write failure path untested**: The `except Exception` block in `get_access_token` (lines ~155-161 of token_store.py) that logs but continues after a DB write failure is not covered by tests. Low risk since it is a defensive fallback, but a test would confirm the access_token is still returned when `save_token` raises. 2. **Pre-existing Alembic fork**: Main has two heads (019 and 020 both descend from 018). A merge migration is needed. This should be tracked as a separate issue -- it predates this PR and is not within scope. 3. **Inline import**: `from basketball_api.services.token_store import get_access_token` is imported inside `get_gmail_client` rather than at module top. This avoids circular imports, which is a valid reason, but a comment explaining the deferred import would help future readers. ### SOP COMPLIANCE - [x] Branch named after issue: `130-spike-gmail-oauth-token-persistence` matches issue #130 - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and detailed - [x] Related references: Closes #130, references #129 and platform #122 - [x] Tests exist: 16 new tests covering save/load, upsert, refresh flow, expiry, seed, integration - [x] No secrets committed: token values in tests are dummy strings (ya29.valid-access-token, etc.) - [x] No unnecessary file changes: all 8 changed files are directly related to the feature - [x] Commit messages: not visible in diff, PR title is descriptive ### PROCESS OBSERVATIONS - **Deployment Frequency**: Clean migration + backwards-compatible fallback means this can deploy without coordination. Seed script is a manual post-deploy step (documented in PR body). - **Change Failure Risk**: Low. Fallback to file-based tokens means a broken DB token path does not cause email outage. The current 29-hour outage scenario (token refresh lost on pod restart) is directly addressed. - **Follow-up needed**: (1) Pre-existing Alembic fork needs a merge migration (separate issue). (2) `pal-e-deployments` needs init container + volume mount removal after seed script runs (documented in PR). ### VERDICT: APPROVED

forgejo_admin referenced this pull request 2026-03-25 00:38:19 +00:00

Bug: Alembic forked history — migrations 019 and 020 both descend from 018 #166

forgejo_admin merged commit 400f19d0f2 into main 2026-03-26 03:36:43 +00:00

forgejo_admin deleted branch 130-spike-gmail-oauth-token-persistence 2026-03-26 03:36:43 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-26 03:36:45 +00:00

feat: Gmail OAuth token persistence in Postgres (#163)

forgejo_admin referenced this pull request from a commit 2026-03-26 03:43:56 +00:00

fix: renumber merge migration to 022 — 021 is taken by oauth_tokens

forgejo_admin referenced this pull request 2026-03-26 03:45:35 +00:00

fix: merge forked alembic migration chain #169

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/basketball-api!163

No description provided.