Skip to content

fix(migrations): widen alembic_version.version_num so long revision ids stamp on PostgreSQL (#1420)#1421

Merged
vybe merged 1 commit into
devfrom
fix/1420-alembic-version-num-width
Jul 2, 2026
Merged

fix(migrations): widen alembic_version.version_num so long revision ids stamp on PostgreSQL (#1420)#1421
vybe merged 1 commit into
devfrom
fix/1420-alembic-version-num-width

Conversation

@dolho

@dolho dolho commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

P0 outage fix. On every PostgreSQL instance the backend fails to boot: alembic upgrade head aborts on migration 0009 because its 41-char id 0009_agent_ownership_public_channel_model doesn't fit alembic_version.version_num, which Alembic auto-creates as VARCHAR(32). PostgreSQL enforces the width → StringDataRightTruncation → backend never healthy → depends_on: service_healthy takes frontend/mcp/scheduler down too. SQLite doesn't enforce VARCHAR length, so SQLite-only CI never caught it.

Fix (widen to VARCHAR(255) — no revision renames)

  • 0008a_widen_alembic_version — new migration slotted before 0009 (which is rechained onto it). Its own id is ≤32 so it stamps fine on the still-narrow column, then ALTERs the column to 255 on PostgreSQL. No-op on SQLite/other dialects and on already-wide columns. Fixes existing PG DBs on their next upgrade.
  • env.py sets version_table_column_type=String(255) so fresh builds auto-create the version table wide; 0001_baseline downgrade DDL matches.

Regression guards (both requested in the issue)

  • .github/workflows/pg-migrations.yml — the guard that would've caught this: runs a real alembic upgrade head against postgres:16, seeding a narrow (32) alembic_version first to reproduce the exact outage path, then asserts head is reached and width ≥255. Advisory (not a required check) so an infra hiccup can't brick the merge queue.
  • tests/unit/test_alembic_revision_id_length.py — lints every revision id ≤255 and keeps the env.py/baseline widths in lockstep (runs in the existing SQLite lane).

Verification — end-to-end on real PostgreSQL 16

Against the running trinity-postgres (whose live alembic_version is indeed varchar(32)):

  • Reproduced the outage: a narrow-column DB stamped at 0008 → upgrade → hit the truncation point.
  • Fixed: a from-base upgrade over a 32-wide column ran 0008 → 0008a_widen → 0009 (41 chars) → 0010 (head) and the column is now varchar(255).
  • Fresh build: Alembic auto-creates at varchar(255) via env.py and reaches 0010.
  • Chain is linear, single head (alembic heads0010). Scratch DBs cleaned up.

Unit: 41 passing across the new lint + test_schema_parity + test_alembic_parity_guard. Migration is PG-guarded so the SQLite track is untouched; check_alembic_parity.py is satisfied (no db/{schema,tables,migrations}.py DDL added).

Fixes #1420

🤖 Generated with Claude Code

…ds stamp on PostgreSQL (#1420)

P0 — backend failed to boot on every PostgreSQL instance. Alembic auto-creates
`alembic_version.version_num` as VARCHAR(32) (its historical default), but
Trinity's descriptive `NNNN_<table>_<change>` revision ids exceed that
(`0009_agent_ownership_public_channel_model` = 41). PostgreSQL enforces the
width, so the version-stamp UPDATE raised StringDataRightTruncation, the backend
never went healthy, and `depends_on: service_healthy` took frontend/mcp/scheduler
down with it. SQLite doesn't enforce VARCHAR length, so SQLite-only CI never saw it.

Fix (widen to VARCHAR(255), no revision renames):
- New migration `0008a_widen_alembic_version` (slotted before 0009, which is
  rechained onto it) ALTERs the column on existing PG DBs — its own id is ≤32 so
  it stamps fine on the still-narrow column, then widens it. No-op on non-PG.
- `env.py` sets `version_table_column_type=String(255)` so fresh builds
  auto-create the table wide; `0001_baseline` downgrade DDL matches.

Regression guards (both requested in the issue):
- `.github/workflows/pg-migrations.yml` — runs a REAL `alembic upgrade head`
  against postgres:16, seeding a narrow (32) alembic_version first to reproduce
  the exact outage path, and asserts head + width≥255. Advisory (not required)
  so an infra hiccup can't brick the queue.
- `tests/unit/test_alembic_revision_id_length.py` — lints every revision id ≤255
  and keeps env.py/baseline widths in lockstep (runs in the SQLite lane).

Verified end-to-end against the running trinity-postgres (16): reproduced the
narrow-column outage, then upgraded a from-base DB through 0008→0008a→0009(41
chars)→0010(head) with the column widened to varchar(255); fresh auto-create
also lands at 255. Scratch DBs cleaned up.

Closes #1420

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dolho
dolho requested a review from AndriiPasternak31 as a code owner July 2, 2026 13:03
@dolho

dolho commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

/review Report

Branch: fix/1420-alembic-version-num-widthdev
Files changed: 7 (+232 / −4)
Scope: CLEAN
Plan completion: issue's two-part suggested fix — code fix + CI guards — both DONE.

Scope check

  • Intent: unblock backend boot on PostgreSQL — the 41-char revision id 0009_… overflows alembic_version.version_num VARCHAR(32).
  • Delivered: widen to VARCHAR(255) via env.py (fresh builds) + 0008a_widen_alembic_version (existing DBs, rechained before 0009) + baseline downgrade DDL; plus a real-PG CI smoke job and a revision-id lint. No out-of-scope changes. (The .claude / src/backend/enterprise submodule-pointer edits are unstaged working-tree noise and are not in the committed PR — confirmed.)

Critical findings (block merge)

None. Verified:

  • Data safety — the only DDL is ALTER … TYPE VARCHAR(255), a non-destructive widen; static SQL, no interpolation of user input. downgrade() deliberately does not narrow back (documented) so it can't truncation-fail.
  • Migration orderingalembic heads is a single head (0010); chain is linear 0008 → 0008a_widen → 0009 → 0010. The widen runs before 0009's long-id stamp.
  • Cross-track — migration is dialect-guarded (if bind.dialect.name == "postgresql"), so the SQLite track is untouched; check_alembic_parity.py is satisfied (no db/{schema,tables,migrations}.py DDL added).
  • Credentials — the CI job's CREDENTIAL_ENCRYPTION_KEY/Redis values are placeholder dummies (mirrors schema-parity.yml), not real secrets.

Informational findings (review recommended)

[I1] CI: pg-migrations.yml is path-filtered with no run-always self-skip (Confidence: 7/10)
File: .github/workflows/pg-migrations.yml:18-31
The job runs only on PRs that touch src/backend/migrations|db/**. That's correct while it stays advisory (as the header comment states). But schema-parity.yml:9-17 documents the trap: if this is ever added as a required status check, PRs that don't touch those paths leave the context permanently "Expected — waiting for status" and brick the merge queue. This workflow reintroduces exactly the pattern schema-parity.yml was rewritten to avoid.
Suggestion: either keep a hard "do not mark required" note where branch protection is configured, or harden it to run-always + self-skip (the schema-parity.yml shape) so it's safe to require.

[I2] CI: Python-dependency surface is unproven (Confidence: 5/10 — appendix)
File: .github/workflows/pg-migrations.yml:73-77
Installs tests/requirements-test.txt + psycopg2-binary + alembic, but env.py imports db.tables / db.engine / config; if those pull a dep not in requirements-test.txt, the job errors on first run. Can't be proven without a live Actions run (flagged in the PR description). The core alembic upgrade head was verified in-container against real PG, so only the CI packaging is unverified.

[I3] Naming: 0008a_ breaks the strict NNNN_ prefix convention (Confidence: 6/10)
File: src/backend/migrations/versions/0008a_widen_alembic_version.py
A letter-suffixed prefix to slot between released 0008 and 0009. Functionally correct (Alembic orders by down_revision, not filename) and avoids renaming already-chained ids per the issue's preferred approach — just noting the deviation from the numeric convention.

Clean categories

Race/concurrency (migrations run under the #1160 flock; widen is in-transaction) · Auth boundaries (no endpoints) · Frontend/XSS (no UI) · Error handling (dialect-guarded) · Enum completeness (n/a) · Docs staleness (architecture.md invariant #3 updated in the same diff).

Summary

  • Critical: 0 — none found.
  • Informational: 3 — [I1] is the one worth acting on (guard against this job ever becoming a required check).
  • Scope: clean.
  • Verified end-to-end on real PostgreSQL 16 (reproduced the truncation, then upgraded from-base through the 41-char 0009 to head with the column at 255).

Durable learning (for /autoplan)

Every Alembic schema change needs a real-PostgreSQL CI run, not just SQLite: SQLite silently ignores VARCHAR length, so a whole class of PG-only migration failures (over-width alembic_version.version_num, type enforcement, IF NOT EXISTS differences) merges green and only breaks on PG boot.

🤖 Generated with Claude Code

@dolho
dolho requested a review from vybe July 2, 2026 13:12

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P0 PostgreSQL boot fix. Validated: migration chain is linear/single-head; the width-widen (0008a) sequences correctly before 0009's 41-char id stamp; SQLite track correctly untouched (Invariant #3); regression test + advisory pg-migrations CI guard both name #1420 and pass; all 4 required checks green. Approving.

@vybe
vybe merged commit 29d14e3 into dev Jul 2, 2026
20 checks passed
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.

2 participants