Rehearse the upgrade, rollback, and recovery paths in CI - #177
Conversation
Part 2 of #147, and the last. Closes #147. `docs/releases.md` claims an upgrade path is supported and that a backup can be restored. This makes both claims something a reader can check rather than something the project asserts about itself. `make rehearse` runs against a real Postgres — CI on every pull request, an operator locally — and does four things, each for a failure the others do not cover: **Every revision applied one at a time**, rather than in one `head` jump. A migration that only works when the whole schema is created at once passes the jump and fails the upgrade an operator actually runs. **Reversed one at a time, then re-applied.** Reversibility is what makes rollback mechanically possible, and the re-apply is the assertion: a downgrade that leaves a table, an index or a type behind fails the second upgrade on a name that already exists. **The previous release upgraded onto this tree.** The supported path itself, through a `git worktree` at the tag so the migrations come from the release while `uv run` still resolves this environment. It skips *loudly* until a release tag exists — a rehearsal that reports success without having run is worse than one that says it could not. **A backup destroyed and restored**, with a row written before the dump and asserted after it, then the restored schema compared against the models. A backup nobody has restored is a plan, not a backup. That last comparison earned its place on the first run: it found three drifts between the migrations and the models that SQLite cannot express, and that `test_migrations.py` has therefore been green through. Migration 0016 reconciles them — two VARCHAR lengths declared wider than `enum_type()`/`max_length` produce, and three unique columns that 0014 created as unique *indexes* where `Field(unique=True)` produces unique *constraints*, which is how every other unique column in this schema was created. No behaviour changes. They matter because autogenerate compares against the models: left alone, the next `--autogenerate` would fold these into an unrelated migration and its reviewer would have to work out which half was intended. The rehearsal lives in the existing Postgres job rather than a new one — it needs exactly that service and that environment, and a second Postgres to say the same thing twice is a minute of CI nobody gets back. That job's checkout gains `fetch-depth: 0` (a shallow clone has no tags, so the previous-release leg would skip for the rest of the project's life while reporting success) and `persist-credentials: false` (it checks out a historical tag with `git worktree` and needs no credential to do it). Verified end to end against Postgres 18 locally: all four rehearsals pass, and the schema comparison is clean with 0016 applied. make check green: 1819 passed.
CI failed on the first real run, four rehearsals in:
pg_dump: error: aborting because of server version mismatch
pg_dump: detail: server version: 18.6; pg_dump version: 16.14
The runner ships an older libpq than the service container, and `pg_dump`
refuses to dump a server newer than itself. Nothing to do with this project —
which is exactly why it is worth naming rather than working around.
Two changes. CI installs `postgresql-client-18` before the rehearsal, from the
PGDG repository the image already has configured. And the script checks the two
versions up front and says what to install, because the mismatch otherwise
arrives after several minutes of rehearsals that all succeeded, in a message that
reads like a bug in the script.
Verified both ways locally against Postgres 18: with a matching client all four
rehearsals pass, and with a deliberately old one the pre-check refuses before any
work is done.
`postgresql-client-18` is not in the runner image's package lists, so the previous fix traded one failure for another. Adding the PGDG apt repository would work, but it puts a third-party package source into a security project's CI to fetch a binary the job already has locally — the service container is `postgres:18-alpine`, and its client tools are the server's own version by construction. So the step points `psql` and `pg_dump` at that container through two shims on PATH. No download, no version to keep in step with the service, and the rehearsal script stays oblivious: it calls both from PATH, which is what an operator does too. The version pre-check added alongside it stays, because a local run can still have a mismatched client and deserves to be told which package to install.
|
CI green. The first two runs failed on the rehearsal, both for environment
Rather than add the PGDG apt repository (a third-party package source in a The version pre-check added alongside stays, because a local run can still have The run log confirms it ran rather than skipped: The third leg is the loud skip by design — there is no release tag yet, and a |
There was a problem hiding this comment.
Verdict
APPROVE
Completed bounded review across 1 immutable scope(s). No actionable findings in the supplied change.
Scope health
Convergence: healthy. Review mode: initial.
Recommended action: CONTINUE_INCREMENTAL.
- No escalation signals.
Prior findings
| Finding | Status |
|---|---|
| — | No prior finding state |
New findings
No new findings.
Fix-induced regressions
- None evidenced.
Uncertainty
- No material uncertainty recorded.
Validation
- Reviewed the supplied immutable diff; exact-head CI is reported as passed.
Residual risks
- None identified.
Part 2 of #147, and the last.
Closes #147
docs/releases.mdclaims an upgrade path is supported and that a backup can berestored. This makes both claims something a reader can check rather than something
the project asserts about itself.
make rehearseruns against real Postgres — CI on every pull request, an operatorlocally — and does four things, each for a failure the others do not cover:
headjump and fails the upgrade an operator actually runsgit worktreeat the tag so the migrations come from the release whileuv runstill resolves this environmentThe third leg skips loudly until a release tag exists. A rehearsal that reports
success without having run is worse than one that says it could not.
The comparison earned its place on the first run
It found three drifts between the migrations and the models that SQLite cannot
express — and that
apps/api/tests/test_migrations.pyhas therefore been greenthrough this whole time:
finding.validation_reasondeclaredlength=64in 0009 whileenum_type()produces 32 for every enum in the codebase.
validation_policy.rationalecreated asTEXTwhile the model saysmax_length=2000.owner_group.name,routing_rule.nameandresponse_target.severitycreated by0014 as unique indexes, where
Field(unique=True)produces uniqueconstraints — which is how every other unique column in this schema was made.
Migration
0016reconciles them. No behaviour changes. They matter becauseautogenerate compares against the models: left alone, the next
--autogeneratewould fold these into an unrelated migration, and its reviewer would have to work
out which half was intended.
Where it runs
In the existing Postgres job rather than a new one — it needs exactly that service
and that environment, and a second Postgres to say the same thing twice is a minute
of CI nobody gets back.
That job's checkout gains two things:
fetch-depth: 0, because a shallow clone has no tags, so the previous-releaseleg would skip for the rest of the project's life while reporting success.
persist-credentials: false, because it now checks out a historical tag withgit worktreeand needs no credential to do it. The other four checkouts inci.ymlhave the same pre-existing finding and are deliberately left alone here.Verification
Run end to end against Postgres 18 locally: all four rehearsals pass and the schema
comparison is clean with 0016 applied. Before 0016 it reported exactly the three
drifts above — which is how they were found.
#147 acceptance criteria
make checkgreen: 1819 passed.