Skip to content

fix(usage): normalize live ingest account ids - #1731

Closed
Komzpa wants to merge 4 commits into
mainfrom
fix/live-usage-account-identity-20260814
Closed

fix(usage): normalize live ingest account ids#1731
Komzpa wants to merge 4 commits into
mainfrom
fix/live-usage-account-identity-20260814

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • normalize live usage ingest account_id values before writing usage_history
  • accept workspace-suffixed codex-lb account ids by resolving them to the stored Account.id
  • keep chatgpt_account_id fallback and separate cache keys for account-id vs ChatGPT-id resolution

Validation

  • uv run pytest tests/integration/test_live_usage_ingest.py -q
  • uv run ruff check app/modules/usage/live_ingest.py tests/integration/test_live_usage_ingest.py
  • uv run ruff format --check app/modules/usage/live_ingest.py tests/integration/test_live_usage_ingest.py
  • uv run ty check app/modules/usage/live_ingest.py tests/integration/test_live_usage_ingest.py

Live evidence: last-day codex-lb logs showed sqlite3.IntegrityError FOREIGN KEY constraint failed from app/modules/usage/live_ingest.py -> app/modules/usage/repository.py while inserting usage_history.account_id values that can differ from stored suffixed workspace Account.id rows.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8a6c12a0e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/usage/live_ingest.py Outdated
Comment thread app/modules/usage/live_ingest.py
Comment thread tests/integration/test_live_usage_ingest.py Outdated
Comment thread app/modules/usage/live_ingest.py
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 13, 2026
@Komzpa

Komzpa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab8d752215

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tests/integration/test_live_usage_ingest.py
Comment thread app/modules/usage/live_ingest.py
@Komzpa
Komzpa force-pushed the fix/live-usage-account-identity-20260814 branch from ede2af1 to 3afaa02 Compare August 13, 2026 23:13
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 13, 2026
@Komzpa

Komzpa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3afaa02365

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/usage/live_ingest.py
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 13, 2026
@Komzpa
Komzpa force-pushed the fix/live-usage-account-identity-20260814 branch from 3afaa02 to 6506670 Compare August 13, 2026 23:36
@Komzpa
Komzpa force-pushed the fix/live-usage-account-identity-20260814 branch from 6506670 to ffbdde5 Compare August 13, 2026 23:40
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 13, 2026
@Komzpa

Komzpa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffbdde5a27

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/usage/live_ingest.py Outdated
Comment thread app/modules/usage/live_ingest.py Outdated
Comment thread app/modules/usage/live_ingest.py
@github-actions github-actions Bot added the db migration PR changes Alembic database migrations; maintainer must coordinate merge order label Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/live-usage-account-identity-20260814 branch from e0e8d08 to f2ccfde Compare August 14, 2026 06:16
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f2ccfde440

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 57 to +58
result = await to_thread.run_sync(lambda: run_upgrade(db_url, "head", bootstrap_legacy=False))
assert result.current_revision == revision
assert result.current_revision == head_revision

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add round-trip coverage for the identity index

This change only advances the expected Alembic head while continuing to inspect telemetry columns, so the test passes even if the new account identity index is never created or its downgrade leaves it behind. Add upgrade and downgrade assertions for idx_accounts_chatgpt_account_id, including the PostgreSQL concurrent path where supported, to satisfy the repository's migration coverage gate.

AGENTS.md reference: AGENTS.md:L116-L120

Useful? React with 👍 / 👎.

Comment on lines +25 to +26
with op.get_context().autocommit_block():
op.execute(sa.text(f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {_INDEX} ON {_TABLE} (chatgpt_account_id)"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rebuild invalid indexes before concurrent creation

If PostgreSQL interrupts CREATE INDEX CONCURRENTLY, it can leave a same-named index with pg_index.indisvalid = false; on the next upgrade, IF NOT EXISTS accepts that unusable index and Alembic stamps the revision as complete, leaving every ChatGPT identity lookup without the intended index. The repository's other concurrent-index migrations explicitly detect and drop invalid leftovers before retrying, and this migration should do the same.

AGENTS.md reference: AGENTS.md:L116-L120

Useful? React with 👍 / 👎.

Comment on lines 277 to +282
async with get_background_session() as session:
rows = (
(await session.execute(select(Account.id).where(Account.chatgpt_account_id == chatgpt_account_id)))
(
await session.execute(
select(Account.id).where(Account.chatgpt_account_id == chatgpt_account_id).limit(2)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Hold identity ownership stable through persistence

When a credential refresh moves a ChatGPT identity after this query completes but before _ingest opens its separate write session, the snapshot is still inserted for the former owner, despite the delta spec requiring attribution to the row that currently owns the identity. Resolve and persist within one transaction while locking the matched account row, or revalidate ownership immediately before the insert, so concurrent identity reassignment cannot contaminate another account's routing inputs.

AGENTS.md reference: AGENTS.md:L24-L26

Useful? React with 👍 / 👎.

- Code: live usage ingestor identity resolution and coalescing.
- Tests: integration coverage for hub-published snapshots, unproven suffix
drops, and resolved-alias coalescing.
- Configuration, schema, response shapes, and background polling are unchanged.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Declare the database index in the change impact

The proposal says schema is unchanged, but this commit adds both an Alembic revision and idx_accounts_chatgpt_account_id to the SQLAlchemy schema. Because OpenSpec is the repository's schema-change SSOT and readiness gate, this understates the deployment impact and can cause reviewers or operators to overlook the required migration; list the new index and migration under Impact instead.

AGENTS.md reference: AGENTS.md:L92-L98

Useful? React with 👍 / 👎.

@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Folded into #1732 (live-ingest account-id normalization joins the hard-continuity owner-selection carrier); history preserved via merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖 codex: needs work [@codex review] raised an issue db migration PR changes Alembic database migrations; maintainer must coordinate merge order

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant