[devinder-dev]

Stacked on #22 (Postgres). Base is infra/postgres, so this diff shows only step 2.4. When
#22 merges, this PR moves to main and gets one rebase. It can only go live after #22 is
applied, because the migrate Job needs Postgres.

What

Step 2.4 of Phase 2: every deploy runs the commit's database migrations first, before any pod
changes.

Part	Files	What
2.4a	api/db/migrations/…_init.sql, api/package.json + bun.lock, api/Dockerfile	Migration 001 (schema v1, with deleted_at for the soft delete and named indexes; down raises an error on purpose). dbmate 2.37.0, pinned, inside the API image, run as bun run --bun db:migrate (dbmate … migrate)
2.4a	.github/workflows/pipeline.yml	api-image builds the image, smoke-tests the migrations inside it (uid 1000, read-only root, 128 MiB, against a throwaway Postgres made by our first-start script), and only then pushes, on main
2.4b	k8s/migrate/job.yaml	The migrate Job: generateName, backoffLimit: 0, deadline 150 s, pod grace 10 s, TTL 1 h, Pod Security restricted
2.4b	infra/server/relay-deploy.sh	run_migrations: create the Job, then ask for its verdict every 2 s, at most 90 times (≥ 180 s). Complete → continue; Failed / no verdict / can't read → print why, return 1, and the existing rollback takes over. Commits without k8s/migrate/ skip this, so a rollback to Phase 1 still works
2.4b	k8s/platform/deployer.yaml	The Role gets jobs (get, list, watch, create: no update, no delete) and pods/log (get)
2.4b	infra/server/test-relay-deploy.sh	Cases 11–17, a fake sleep, and a 30 s watchdog so a hanging script fails the test instead of CI
2.4b	pipeline.yml	Deploy timeout 35 → 40 min (the migrate step adds up to 3 min, twice on a rollback)
2.4c	docs/decisions/0006-migrations-expand-contract.md	The rule: forward-only, expand first, contract only what the last good release no longer uses. With the rename example in four releases
docs	k8s/README.md, docs/CONTEXT.md	A row for k8s/migrate/; deleted_at and the migrations row in CONTEXT

Tested before the PR

• Locally against Postgres 18.6:
  • the first db:migrate creates the 6 tables, owned by relay, and a second run does nothing;
  • dbmate rollback is refused and the history stays;
  • a file broken halfway leaves nothing behind;
  • with two new files and the second broken, the first stays applied.
• The CI smoke test, run locally with the real image: exit 0.
• test-relay-deploy.sh: 49 passed, 0 failed. A Job that stays Pending creates exactly 1 Job,
  reads its status 90 times, then gives up with "may still be running".
• Server dry runs: the Job is valid (the server invents its name), and only the Role's two new
  rules change. shellcheck, actionlint and bun audit are clean.

After merging (an admin, in this order; output posted here)

The merge itself deploys with the old installed script, which doesn't migrate yet. That's
harmless: nothing in k8s/app/ changes. Then:

1. Apply k8s/platform/deployer.yaml first, and check as the deployer: create jobs and
   get pods/log → yes; delete jobs, patch jobs, get secrets → no. (The other way round,
   the new script couldn't create its Job, and the rollback would fail the same way.)
2. Install the new relay-deploy (install -o root -g root -m 755) and diff it against git.
3. Deploy the same commit by hand (sudo -u deploy env SSH_ORIGINAL_COMMAND=<sha> relay-deploy).
   This runs migration 001: migrating with job.batch/migrate-…, then Applying: …_init.sql.
4. Check schema_migrations and the tables as relay. One hour later, the old Jobs are gone.
5. Negative tests: a hand-made Job with an image that doesn't exist ends as Failed
   (DeadlineExceeded), and the legacy rollback test (Phase 1's commit a78934c…, full SHA)
   still rolls back with 0 failed requests.

For the reviewer

• Is the expand/contract rule clear enough that you could apply it to one of your own changes?
• pods/log lets the deployer read every pod's log in relay. Acceptable, given that nothing
  logs a secret?

[AbdikhaniAbdirashid]

Read the migration against docs/api.md and the data model in docs/CONTEXT.md, field by field, plus the Job, the relay-deploy changes and ADR 0006. One thing I think is missing and two questions below; the rest I checked and agree with.

Checked against the contract, field by field

Every validation rule in the schema matches docs/api.md exactly, which is the part I expected to find drift in and did not:

Contract	Schema
name 1–100 characters	CHECK (length(name) BETWEEN 1 AND 100) on owners and endpoints
url at most 2048	CHECK (length(url) <= 2048)
type matches ^[a-z0-9][a-z0-9_.-]{0,99}$	the same regex, character for character
payload a JSON object	CHECK (jsonb_typeof(payload) = 'object')
event_types 1–20	CHECK (cardinality(event_types) BETWEEN 1 AND 20)
idempotency_key ≤ 255, UNIQUE per owner	both, and NULLs don't collide
status pending/delivered/dead	CHECK (status IN (…))

The 2048 one matters to me beyond tidiness: I enforced exactly that number in the endpoint form yesterday (#9), so the form, the contract and the column now say the same thing.

Also: CONTEXT.md is updated in the same PR, disabled_at → deleted_at. The data model and the schema can't disagree, which is the right way round.

Three choices I want to say out loud, because they are not obvious

• attempts has no number column; it's row_number() when read. The contract says the number runs 1, 2, 3 across the whole history including after replays — which a stored counter would have to be kept in step with, and attempts on the delivery is already a different number (the retry budget, reset by replay). Two counters with similar names is exactly how they end up disagreeing. Deriving it means it cannot.
• text + CHECK instead of an enum. Adding an allowed value is a cheap migration; removing one from a Postgres enum is not. Given forward-only (ADR 0006), that asymmetry is the right way to be wrong.
• migrate:down raising an exception rather than being empty. An empty section would delete the row in schema_migrations while leaving the tables, so the next migrate would try to create them again and fail on something unrelated. Raising rolls the whole rollback back. That is a trap I would have walked into.

One thing I think is missing: nothing maintains updated_at

endpoints.updated_at and deliveries.updated_at are NOT NULL DEFAULT now(), and there is no trigger. So unless every single UPDATE in Phase 3 and Phase 4 sets updated_at = now() by hand, the column quietly means created_at instead — and the one query that forgets is invisible, because a stale timestamp looks like a real one.

Three ways out, and I don't mind which:

1. A BEFORE UPDATE trigger now, while the schema is new. One function, two triggers.
2. Keep it manual, and say so in ADR 0003 so the repositories are written that way from the start.
3. Drop the columns until something needs them — they aren't in docs/api.md and no screen reads them.

I'd pick 1 or 3. Adding a trigger later is its own migration, and option 2 relies on nobody forgetting across two phases of writes.

Two questions

1. next_attempt_at is NOT NULL here, nullable in the contract.

schema:    next_attempt_at timestamptz NOT NULL DEFAULT now()
docs/api.md: next_attempt_at: string | null   // null unless status is "pending"

I can see why the column is NOT NULL — your comment says that while a row is leased it holds the lease's expiry, so it always has an operational meaning. But that makes the column and the API field two different things, and the read has to blank it whenever status != 'pending'. If one query forgets, the dashboard shows a "next attempt" on a delivered delivery, which is the kind of wrong that looks fine.

Is that mapping going to live in the repository layer, or in a view? Either is fine, I'd just like it written down somewhere before Phase 3, because the dashboard reads next_attempt_at === null as "nothing is coming".

2. SET LOCAL lock_timeout = '5s' is the one line here that can't be tested yet.

SET LOCAL only does anything inside a transaction, and your own comment points out that an empty database has no locks to wait for. So the template works by construction today and we won't find out whether it holds until a migration runs against a live database with traffic — which is the moment we most want it to.

Does dbmate wrap each migration in a transaction by default, and have you confirmed that for 2.37.0 rather than assumed it? If it doesn't, the SET LOCAL is silently a no-op and migration 002 could queue every API query behind a lock instead of giving up after 5 s. Worth one BEGIN-level check now, since every future migration will copy this header.

The deploy path

run_migrations reads well and the failure paths are the ones I'd want: Failed, no verdict, and can't-read all return 1 and hand over to the existing rollback. Two things I specifically liked:

• commits without k8s/migrate/ skip it, so a rollback to a Phase 1 commit still works. That's the kind of backwards compatibility that only shows up when you need it at 2 a.m.
• the deployer Role gets create on jobs but not update or delete, and pods/log read-only. Same shape as the no-delete-on-deployments rule: a deploy adds, it never removes.

49 cases passing in test-relay-deploy.sh, including a Job that stays Pending creating exactly one Job and giving up after 90 reads, is the part that makes me comfortable with this touching production's deploy script.

Order

Agreed on applying deployer.yaml before installing the new script — the other way round, the new script can't create its Job and the rollback fails the same way. Please post the output of step 3, the by-hand deploy that runs migration 001, when you do it.

Not approving yet, only because of updated_at: it is a two-line fix now and an extra migration later. Say which of the three you want and I'll re-review straight away — or push back if you think manual is fine and I'll approve on that.

[AbdikhaniAbdirashid]

One more thing, found while reviewing #26 rather than this PR, so it didn't make my first pass.

The migration smoke test hardcodes the Postgres digest in pipeline.yml:

postgres:18.6@sha256:979bccfde35445db618c65107d27d08c6c21339d8dc3ad4c830c6633947be0e4

But #26's api/scripts/test-db.sh reads that digest out of k8s/data/postgres.yaml, which is the right instinct — one place owns the version.

So after both PRs land, the same digest lives in two places with two different mechanisms. Bump postgres.yaml and the integration tests follow it while the smoke test quietly keeps testing the old image — and the smoke test is the one guarding the deploy. A version bump would then be green on a database nobody runs.

Same fix as #26 already uses: read it out of the manifest. Something like

- name: Read the Postgres digest from the manifest
  run: echo "PG_IMAGE=$(grep -oP 'postgres:[0-9.]+@sha256:[0-9a-f]+' k8s/data/postgres.yaml | head -1)" >> "$GITHUB_ENV"

and then use $PG_IMAGE in the docker run. Or extract whatever test-db.sh does into one place both can call, which is probably tidier since there will be a third caller eventually.

Worth folding into the updated_at round rather than its own PR.

[devinder-dev]

Continued in #27. GitHub closed this PR when its base branch was deleted after #22 merged. Every review point is answered there, under 'Changed after the review of #25'.