#1996 - feat(retention): load the history gate — Postgres projection + backfill - sean/pipelines

sean commented

2026-08-07 13:41:34 +00:00

Owner

Loads the retention gate: the sweeper now writes every domain CR to pipeline_history in the shared CNPG before it deletes it, and a failed write blocks the delete.

Revised after three adversarial reviews returned FIX_REQUIRED with five HIGH defects. Four of the five lived in the backfill CronJob, which is removed from this branch and deferred; the fifth and the upsert key are fixed here.

The key is `(kind, namespace, uid)`, not `(..., name)`

Both objections to the original upsert were correct, which means its premise was wrong. It keyed on the name because "the name is the LOGICAL identity". But Build and Change names are deterministic per (project, commit) and per (project, PR), so a swept name is recreated by a genuinely DIFFERENT object — and one row for two objects forced two mutually exclusive requirements onto one predicate:

	consequence
with `or excluded.activity_at > ...`	a projection of the RECREATED object overwrites the sweep row of the previous one — the only surviving copy of a deleted CR. Reproduced against real Postgres 18.
without it	the second sweep of the recreated name is REFUSED while the sweeper deletes the CR anyway — the newer record lost.

The uid is the identity; the name is a label that repeats. Keyed on the uid, two distinct objects are two rows, neither failure can occur, and activity_at reverts to an idempotency guard for a genuine re-write of the same object. uid was already a column. The duplicate-pane worry that argued for the name is a READ-PATH problem and the schema comment now says so.

v1 is rewritten in place rather than appended to, under a stated exemption: no released image carries --history-database-url, so no process can have created the table anywhere.

A test changed on purpose. postgres_test.go pinned the activity_at clause with the rationale "this clause keeps the newest record for a NAME". That is the wrong invariant — it is the sentence that made the data loss mandatory, and it also banned the natural fix. The clause survives; its stated job does not.

`history.New` is no longer fatal at boot

pgconn reads sslrootcert EAGERLY at ParseConfig, and cmd/main.go turns a New error into os.Exit(1) — so a pod scheduled before trust-manager writes ConfigMap ca-bundle CrashLooped the whole CI control plane on every fresh cluster and every make rebuild. ParseConfig moves into session.connect(); New still validates the URL's text, which needs no file. An unreadable CA now fails the DIAL, so the sweep records nothing and deletes nothing.

One DSN, three consumers

No Go test constructed the DSN that ships. history.DefaultDatabaseURL is now the flag default, the value the wiring guard pins the manifest to by exact equality, and the value TestShippedDSNIsAccepted parses — asserting verify-full on what pgx built, not on a substring.

Guards anchored, not substring-matched

tests/history-store-contract.sh decided "a real exporter is wired" with a whole-file substring test that a combined mutation walked through. It now starts at the WorkflowProjector's History field and follows the identifier to a terminal history.New that must name history.SourceSweep; an unresolvable chain FAILS. runBackfill's SourceBackfill is asserted positively — flipping it was a live defect nothing caught.

DEFERRED: the periodic backfill

operator/config/seanfarm/history-backfill.yaml and its kustomization entry are removed. The binary keeps --mode=backfill and internal/backfill; nothing schedules it, and tests/history-postgres-wiring.sh FAILS if any rendered workload passes --mode=backfill, and fails if it scanned no pod templates at all.

The follow-up starts from these four, all found by review:

the CronJob rendered at pipelines-operator:v0.1.149 — a bare tag on an abandoned release line — so it could never start;
its pod template carried the exact selector labels of BOTH Services in the namespace, hijacking the EndpointSlices of pipelines-operator-query that the dashboard and pipectl depend on;
the same labels put it in the manager's PodDisruptionBudget pool (ALLOWED DISRUPTIONS 1 → 2, so both manager replicas become evictable during a node roll);
and collided with the manager's required pod anti-affinity.

Consequence while deferred: the archive covers what the SWEEPER removes and nothing else. The ~4,800 objects alive today are outside it, and a CR that make rebuild or an ArgoCD prune deletes leaves no record. docs/DOMAIN-MODEL.md and DURABLE-STATE-AND-PLUGGABLE-EXECUTION.md say so rather than claiming the hourly pass.

Also deferred (low severity, review-verified as correct in the live code, missing only a spec): sweepRetention swallowing a Begin error, and storeHistory's || sess == nil half. Both mutations stay green today.

Verification

go build ./... && go vet ./... && gofmt -l . clean
go test -count=1 ./... — 17 packages ok, envtest included (internal/controller 37.936s)
PIPELINE_GUARDS_STRICT=1 tests/check.sh → guards: 119 invoked — 119 passed, 0 skipped, 0 failed (strict=1)
go mod tidy — moves pgx into the direct require block. It also moves prometheus/client_golang, whose mis-marking predates this branch; tidy is deterministic and cannot produce one without the other.

Mutation rehearsal: 10/10 caught, each one asserted to have actually applied (git diff --stat after the edit) before its result was trusted, and the harness itself sanity-checked green on an unmutated tree. The first harness run reported a false GREEN because a pipe into tail swallowed the exit code; pipefail fixed it.

mutation	caught by
key reverted to the name in BOTH files together	`TestTheKeyIsTheUIDAndNotTheName`
upsert conflicts on name while the PK stays uid	`TestConflictTargetMatchesThePrimaryKey`
`ParseConfig` moved back into `New`	`TestNewToleratesAnUnreadableCABundle` + the fail-closed pair
`DefaultDatabaseURL` weakened to `sslmode=require`	`TestShippedDSNIsAccepted` AND the wiring guard
`manager.yaml` DSN drifts (`connect_timeout` 10→30)	wiring guard, exact-equality
`runBackfill` writes `SourceSweep` (previously caught by NOTHING)	store-contract guard
combined bypass: package-`main` no-op writer + `SourceSweep` moved to the backfill	store-contract guard, both arms
the backfill CronJob re-added to the overlay	wiring guard `--mode=backfill` scan
the guard's pod-template traversal finds nothing	wiring guard zero-match failure
the manager declares no `--history-database-url` at all	wiring guard, naming the missing flag

No retention cap moved. That is P4-d.

Loads the retention gate: the sweeper now writes every domain CR to `pipeline_history` in the shared CNPG before it deletes it, and a failed write blocks the delete. Revised after three adversarial reviews returned FIX_REQUIRED with five HIGH defects. Four of the five lived in the backfill CronJob, which is **removed from this branch and deferred**; the fifth and the upsert key are fixed here. ## The key is `(kind, namespace, uid)`, not `(..., name)` Both objections to the original upsert were correct, which means its premise was wrong. It keyed on the name because "the name is the LOGICAL identity". But Build and Change names are deterministic per (project, commit) and per (project, PR), so a swept name is recreated by a genuinely DIFFERENT object — and one row for two objects forced two mutually exclusive requirements onto one predicate: | | consequence | | --- | --- | | with `or excluded.activity_at > ...` | a projection of the RECREATED object overwrites the sweep row of the previous one — the only surviving copy of a deleted CR. Reproduced against real Postgres 18. | | without it | the second sweep of the recreated name is REFUSED while the sweeper deletes the CR anyway — the newer record lost. | The uid is the identity; the name is a label that repeats. Keyed on the uid, two distinct objects are two rows, neither failure can occur, and `activity_at` reverts to an idempotency guard for a genuine re-write of the same object. `uid` was already a column. The duplicate-pane worry that argued for the name is a READ-PATH problem and the schema comment now says so. `v1` is rewritten in place rather than appended to, under a stated exemption: no released image carries `--history-database-url`, so no process can have created the table anywhere. **A test changed on purpose.** `postgres_test.go` pinned the `activity_at` clause with the rationale "this clause keeps the newest record for a NAME". That is the wrong invariant — it is the sentence that made the data loss mandatory, and it also banned the natural fix. The clause survives; its stated job does not. ## `history.New` is no longer fatal at boot `pgconn` reads `sslrootcert` EAGERLY at `ParseConfig`, and `cmd/main.go` turns a `New` error into `os.Exit(1)` — so a pod scheduled before trust-manager writes ConfigMap `ca-bundle` CrashLooped the whole CI control plane on every fresh cluster and every `make rebuild`. `ParseConfig` moves into `session.connect()`; `New` still validates the URL's text, which needs no file. An unreadable CA now fails the DIAL, so the sweep records nothing and deletes nothing. ## One DSN, three consumers No Go test constructed the DSN that ships. `history.DefaultDatabaseURL` is now the flag default, the value the wiring guard pins the manifest to by **exact equality**, and the value `TestShippedDSNIsAccepted` parses — asserting verify-full on what pgx built, not on a substring. ## Guards anchored, not substring-matched `tests/history-store-contract.sh` decided "a real exporter is wired" with a whole-file substring test that a combined mutation walked through. It now starts at the WorkflowProjector's `History` field and follows the identifier to a terminal `history.New` that must name `history.SourceSweep`; an unresolvable chain FAILS. `runBackfill`'s `SourceBackfill` is asserted positively — flipping it was a live defect nothing caught. ## DEFERRED: the periodic backfill `operator/config/seanfarm/history-backfill.yaml` and its kustomization entry are removed. The binary keeps `--mode=backfill` and `internal/backfill`; **nothing schedules it**, and `tests/history-postgres-wiring.sh` FAILS if any rendered workload passes `--mode=backfill`, and fails if it scanned no pod templates at all. The follow-up starts from these four, all found by review: 1. the CronJob rendered at `pipelines-operator:v0.1.149` — a bare tag on an abandoned release line — so it could never start; 2. its pod template carried the exact selector labels of BOTH Services in the namespace, hijacking the EndpointSlices of `pipelines-operator-query` that the dashboard and pipectl depend on; 3. the same labels put it in the manager's PodDisruptionBudget pool (ALLOWED DISRUPTIONS 1 → 2, so both manager replicas become evictable during a node roll); 4. and collided with the manager's required pod anti-affinity. Consequence while deferred: the archive covers what the SWEEPER removes and nothing else. The ~4,800 objects alive today are outside it, and a CR that `make rebuild` or an ArgoCD prune deletes leaves no record. `docs/DOMAIN-MODEL.md` and `DURABLE-STATE-AND-PLUGGABLE-EXECUTION.md` say so rather than claiming the hourly pass. **Also deferred** (low severity, review-verified as correct in the live code, missing only a spec): `sweepRetention` swallowing a `Begin` error, and `storeHistory`'s `|| sess == nil` half. Both mutations stay green today. ## Verification - `go build ./... && go vet ./... && gofmt -l .` clean - `go test -count=1 ./...` — 17 packages ok, envtest included (`internal/controller 37.936s`) - `PIPELINE_GUARDS_STRICT=1 tests/check.sh` → `guards: 119 invoked — 119 passed, 0 skipped, 0 failed (strict=1)` - `go mod tidy` — moves `pgx` into the direct require block. It also moves `prometheus/client_golang`, whose mis-marking predates this branch; tidy is deterministic and cannot produce one without the other. **Mutation rehearsal: 10/10 caught**, each one asserted to have actually applied (`git diff --stat` after the edit) before its result was trusted, and the harness itself sanity-checked green on an unmutated tree. The first harness run reported a false GREEN because a pipe into `tail` swallowed the exit code; `pipefail` fixed it. | mutation | caught by | | --- | --- | | key reverted to the name in BOTH files together | `TestTheKeyIsTheUIDAndNotTheName` | | upsert conflicts on name while the PK stays uid | `TestConflictTargetMatchesThePrimaryKey` | | `ParseConfig` moved back into `New` | `TestNewToleratesAnUnreadableCABundle` + the fail-closed pair | | `DefaultDatabaseURL` weakened to `sslmode=require` | `TestShippedDSNIsAccepted` AND the wiring guard | | `manager.yaml` DSN drifts (`connect_timeout` 10→30) | wiring guard, exact-equality | | `runBackfill` writes `SourceSweep` (previously caught by NOTHING) | store-contract guard | | combined bypass: package-`main` no-op writer + `SourceSweep` moved to the backfill | store-contract guard, both arms | | the backfill CronJob re-added to the overlay | wiring guard `--mode=backfill` scan | | the guard's pod-template traversal finds nothing | wiring guard zero-match failure | | the manager declares no `--history-database-url` at all | wiring guard, naming the missing flag | No retention cap moved. That is P4-d.

sean force-pushed feat/postgres-history-projection from 2fd299a0be

pipeline/ci CI failed @ 2fd299a0be8b

Details

to 9c551288af

pipeline/ci CI failed @ 9c551288af4a

Details

2026-08-07 16:11:23 +00:00

Compare

sean force-pushed feat/postgres-history-projection from 9c551288af

pipeline/ci CI failed @ 9c551288af4a

Details

to 80fd143b2d