refactor(store): maintain updated_at with a trigger, not by hand (RIG-3495) - #989
Merged
trunk-io[bot] merged 1 commit intoSep 8, 2026
Conversation
This was referenced Sep 7, 2026
|
Compass engineering docs preview: https://compass-managed-rig-3495-upd.compass-eng-docs.pages.dev Deployed from |
…-3495) `updated_at` was hand-maintained in every write statement, with nothing enforcing it — and it had already rotted. `secrets.updated_at` is selected and surfaced to callers as a `time.Time` (`store/secrets.go:71`, `:191`), but the table's only write omits the column and there is no UPDATE at all, so the value could never differ from `created_at`: a field that reads like freshness data and never was. Coverage before this change, over the five tables carrying the column: 5 of 9 write statements bumped it. Nothing distinguished the four that didn't. Add a `set_updated_at()` trigger function and a `BEFORE UPDATE` trigger per table, applied through a `DO` loop over an `updated_at_tables` array — the same shape, and for the same reason, as the RLS policy loop above it. Then delete every hand-written `updated_at = now()` so exactly one mechanism owns the column. Adding a table is now one array entry. `BEFORE UPDATE` and not `BEFORE INSERT OR UPDATE`: the column's `DEFAULT now()` already stamps an inserted row, and an INSERT trigger would destroy the ability to insert a deliberate value. An upsert's `ON CONFLICT DO UPDATE` fires on the conflict path, which is what keeps every upsert's column live for free. `search_path` is pinned to `pg_catalog` alone. The function is SECURITY INVOKER, so it would otherwise resolve names against the caller's `search_path`; pinning makes the body independent of it. `public` is deliberately NOT named — the migration is applied into a per-test isolation schema as often as into public, so `public` would pin to a schema that is not the one holding these tables. Left untouched: `issues.forge_updated_at` and `forge_repo_subscriptions.swept_updated_at`. Those are forge-supplied watermarks, not this row's local mutation time, and their writers set them deliberately (the `issues` upsert guards on the incoming value going forward). They are differently named precisely so this trigger cannot reach them. Matt's ruling (2026-09-07): `created_at` + `updated_at` is the general schema convention going forward, maintained by this trigger rather than by hand. Verified: 3 pgtest proofs — the value advances on UPDATE while `created_at` does not move (the assertion that catches a BEFORE INSERT mistake), the upsert conflict path fires through the real `RecordAgentPlacement`, and the `secrets` table is armed. Red control: disabling only trigger creation fails all three, each with `before` and `after` identical, i.e. still the insert-time default. The pre-existing `forge_cursors` test asserting `updated_at` advances on an enable-flip passes unchanged — it previously passed via the hand-written assignment and now passes via the trigger, corroborating the cutover. Gates: `sqlc-drift`, `sql-migration-gate:check` (squawk + sqruff, 0 issues), `go build ./...`, `golangci-lint run ./internal/store/...` (0 issues), and the full `-tags pgtest` store suite all pass. Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-managed/rig-3495-updated-at-trigger
branch
from
September 7, 2026 22:29
303a133 to
7e61da9
Compare
rigel-mintaka
marked this pull request as ready for review
September 7, 2026 22:29
mattwilkinsonn
approved these changes
Sep 8, 2026
|
This pull request was merged into |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack containing 6 PRs:
mainupdated_atwas hand-maintained in every write statement, with nothingenforcing it — and it had already rotted.
secrets.updated_atis selected andsurfaced to callers as a
time.Time(store/secrets.go:71,:191), but thetable's only write omits the column and there is no UPDATE at all, so the value
could never differ from
created_at: a field that reads like freshness data andnever was.
Coverage before this change, over the five tables carrying the column: 5 of 9
write statements bumped it. Nothing distinguished the four that didn't.
Add a
set_updated_at()trigger function and aBEFORE UPDATEtrigger pertable, applied through a
DOloop over anupdated_at_tablesarray — the sameshape, and for the same reason, as the RLS policy loop above it. Then delete
every hand-written
updated_at = now()so exactly one mechanism owns thecolumn. Adding a table is now one array entry.
BEFORE UPDATEand notBEFORE INSERT OR UPDATE: the column'sDEFAULT now()already stamps an inserted row, and an INSERT trigger would destroy the ability
to insert a deliberate value. An upsert's
ON CONFLICT DO UPDATEfires on theconflict path, which is what keeps every upsert's column live for free.
search_pathis pinned topg_catalogalone. The function is SECURITY INVOKER,so it would otherwise resolve names against the caller's
search_path; pinningmakes the body independent of it.
publicis deliberately NOT named — themigration is applied into a per-test isolation schema as often as into public,
so
publicwould pin to a schema that is not the one holding these tables.Left untouched:
issues.forge_updated_atandforge_repo_subscriptions.swept_updated_at. Those are forge-suppliedwatermarks, not this row's local mutation time, and their writers set them
deliberately (the
issuesupsert guards on the incoming value going forward).They are differently named precisely so this trigger cannot reach them.
Matt's ruling (2026-09-07):
created_at+updated_atis the general schemaconvention going forward, maintained by this trigger rather than by hand.
Verified: 3 pgtest proofs — the value advances on UPDATE while
created_atdoes not move (the assertion that catches a BEFORE INSERT mistake), the upsert
conflict path fires through the real
RecordAgentPlacement, and thesecretstable is armed. Red control: disabling only trigger creation fails all three,
each with
beforeandafteridentical, i.e. still the insert-time default.The pre-existing
forge_cursorstest assertingupdated_atadvances on anenable-flip passes unchanged — it previously passed via the hand-written
assignment and now passes via the trigger, corroborating the cutover.
Gates:
sqlc-drift,sql-migration-gate:check(squawk + sqruff, 0 issues),go build ./...,golangci-lint run ./internal/store/...(0 issues), and thefull
-tags pgteststore suite all pass.Co-authored-by: Matt Wilkinson matt@rigel.build