From 7e61da9f314e9ad2918ffc93d8feb32b085c2ca0 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 7 Sep 2026 14:29:58 -0400 Subject: [PATCH] refactor(store): maintain updated_at with a trigger, not by hand (RIG-3495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- go/internal/store/db/agent_config.sql.go | 2 +- go/internal/store/db/agent_placements.sql.go | 3 +- go/internal/store/db/forge_cursors.sql.go | 4 +- go/internal/store/db/model_registry.sql.go | 2 +- go/internal/store/migrations/0001_init.sql | 72 +++++++ go/internal/store/queries/agent_config.sql | 2 +- .../store/queries/agent_placements.sql | 3 +- go/internal/store/queries/forge_cursors.sql | 4 +- go/internal/store/queries/model_registry.sql | 2 +- go/internal/store/updated_at_pgtest_test.go | 189 ++++++++++++++++++ 10 files changed, 271 insertions(+), 12 deletions(-) create mode 100644 go/internal/store/updated_at_pgtest_test.go diff --git a/go/internal/store/db/agent_config.sql.go b/go/internal/store/db/agent_config.sql.go index 4653abe3..d53a5c18 100644 --- a/go/internal/store/db/agent_config.sql.go +++ b/go/internal/store/db/agent_config.sql.go @@ -39,7 +39,7 @@ const putAgentConfig = `-- name: PutAgentConfig :exec INSERT INTO agent_config_bundle (singleton, version, bundle) VALUES (TRUE, $1, $2) ON CONFLICT (singleton) -DO UPDATE SET version = EXCLUDED.version, bundle = EXCLUDED.bundle, updated_at = now() +DO UPDATE SET version = EXCLUDED.version, bundle = EXCLUDED.bundle ` type PutAgentConfigParams struct { diff --git a/go/internal/store/db/agent_placements.sql.go b/go/internal/store/db/agent_placements.sql.go index 72825ee6..637709bb 100644 --- a/go/internal/store/db/agent_placements.sql.go +++ b/go/internal/store/db/agent_placements.sql.go @@ -84,8 +84,7 @@ INSERT INTO agent_placements (agent_account_id, runner_id, container_name) VALUES ($1, $2, $3) ON CONFLICT (agent_account_id) DO UPDATE SET runner_id = EXCLUDED.runner_id, - container_name = EXCLUDED.container_name, - updated_at = now() + container_name = EXCLUDED.container_name ` type RecordAgentPlacementParams struct { diff --git a/go/internal/store/db/forge_cursors.sql.go b/go/internal/store/db/forge_cursors.sql.go index c2c0ef1e..1ed9a632 100644 --- a/go/internal/store/db/forge_cursors.sql.go +++ b/go/internal/store/db/forge_cursors.sql.go @@ -152,7 +152,7 @@ func (q *Queries) LoadForgeRepoWatermark(ctx context.Context, arg LoadForgeRepoW const setForgeRepoSubscriptionEnabled = `-- name: SetForgeRepoSubscriptionEnabled :execrows UPDATE forge_repo_subscriptions - SET enabled = $4, updated_at = now() + SET enabled = $4 WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3 ` @@ -178,7 +178,7 @@ func (q *Queries) SetForgeRepoSubscriptionEnabled(ctx context.Context, arg SetFo const storeForgeRepoWatermark = `-- name: StoreForgeRepoWatermark :execrows UPDATE forge_repo_subscriptions - SET swept_updated_at = $4, list_etag = $5, updated_at = now() + SET swept_updated_at = $4, list_etag = $5 WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3 ` diff --git a/go/internal/store/db/model_registry.sql.go b/go/internal/store/db/model_registry.sql.go index 899c4785..1d30d047 100644 --- a/go/internal/store/db/model_registry.sql.go +++ b/go/internal/store/db/model_registry.sql.go @@ -61,7 +61,7 @@ func (q *Queries) InsertModelRegistry(ctx context.Context, registry []byte) (int const updateModelRegistry = `-- name: UpdateModelRegistry :one UPDATE model_registry - SET registry = $1, version = version + 1, updated_at = now() + SET registry = $1, version = version + 1 WHERE singleton = TRUE AND version = $2 RETURNING version ` diff --git a/go/internal/store/migrations/0001_init.sql b/go/internal/store/migrations/0001_init.sql index 30b540d8..b34f8919 100644 --- a/go/internal/store/migrations/0001_init.sql +++ b/go/internal/store/migrations/0001_init.sql @@ -961,3 +961,75 @@ BEGIN $f$, t); END LOOP; END $$; + +-- ── updated_at maintenance (RIG-3495) ─────────────────────────────────────── +-- created_at + updated_at is the schema convention for every table from here +-- on, and updated_at is maintained by THIS trigger — never by a hand-written +-- `updated_at = now()` in a query file. +-- +-- Hand-maintenance is the failure mode, not a hypothetical one: secrets.updated_at +-- rotted exactly that way. The column was declared here, read by DeclaredSecrets +-- and surfaced on SecretDeclaration.UpdatedAt, but no write statement in +-- queries/secrets.sql ever set it — so the value could only ever equal +-- created_at, and every caller reading it was reading a lie. A per-statement +-- convention that is invisible at the point a NEW write statement is added +-- cannot survive; a trigger is enforced by the table, so a write path added +-- later inherits it without anyone remembering. +-- +-- The rule for a new table: give it created_at + updated_at with the usual +-- DEFAULT now(), add its name to updated_at_tables below, and set updated_at +-- NOWHERE else. There must be exactly one mechanism. +-- +-- NOT covered, deliberately: issues.forge_updated_at and +-- forge_repo_subscriptions.swept_updated_at. Those are forge-supplied +-- watermarks — the remote's mutation time and the sweep high-water mark — not +-- this row's local mutation time, and their writers set them explicitly +-- (issues.sql's upsert even guards on the incoming value going forward). They +-- are differently named precisely so this trigger cannot reach them. +-- +-- search_path safety: the function is SECURITY INVOKER (the default, and what +-- we want — a trigger doing NEW.updated_at = now() needs no elevated rights), +-- but it still runs with whatever search_path the CALLING session has set, so +-- an unqualified name in the body could be resolved against a schema the caller +-- controls. Pinning search_path on the function makes the body's resolution +-- independent of the caller. pg_catalog alone is enough and is the right pin +-- here: the body resolves exactly one name, now(), which lives in pg_catalog — +-- and this migration is applied into a per-test isolation schema as often as +-- into public (internal/pgshare hands each test its own schema via search_path), +-- so naming `public` would pin to a schema that is NOT the one holding these +-- tables. NEW/OLD are parser-level, not search_path-resolved. +CREATE FUNCTION set_updated_at() RETURNS TRIGGER + LANGUAGE plpgsql + SET search_path = pg_catalog +AS $$ +BEGIN + NEW.updated_at = now(); + RETURN NEW; +END $$; + +-- One BEFORE UPDATE ... FOR EACH ROW trigger per table carrying updated_at. +-- BEFORE (not AFTER) because the trigger mutates the row being written, and +-- UPDATE only (not INSERT) because the column's DEFAULT now() already stamps an +-- inserted row — a BEFORE INSERT here would merely re-derive the same value +-- while destroying the ability to insert a row with a deliberate updated_at. +-- An INSERT ... ON CONFLICT DO UPDATE fires this on the conflict path, which is +-- what makes every upsert in queries/ keep the column live for free. Done in a +-- DO loop for the same reason the RLS block above is: so the identical trigger +-- is never copy-pasted per table, and adding a table is one array entry. +DO $$ +DECLARE + t text; + updated_at_tables text[] := ARRAY[ + 'secrets', + 'agent_placements', + 'agent_config_bundle', + 'model_registry', + 'forge_repo_subscriptions' + ]; +BEGIN + FOREACH t IN ARRAY updated_at_tables LOOP + EXECUTE format( + 'CREATE TRIGGER set_updated_at BEFORE UPDATE ON %I + FOR EACH ROW EXECUTE FUNCTION set_updated_at()', t); + END LOOP; +END $$; diff --git a/go/internal/store/queries/agent_config.sql b/go/internal/store/queries/agent_config.sql index 15d47613..23fc6ac6 100644 --- a/go/internal/store/queries/agent_config.sql +++ b/go/internal/store/queries/agent_config.sql @@ -8,7 +8,7 @@ INSERT INTO agent_config_bundle (singleton, version, bundle) VALUES (TRUE, $1, $2) ON CONFLICT (singleton) -DO UPDATE SET version = EXCLUDED.version, bundle = EXCLUDED.bundle, updated_at = now(); +DO UPDATE SET version = EXCLUDED.version, bundle = EXCLUDED.bundle; -- name: CurrentAgentConfig :one SELECT version, bundle FROM agent_config_bundle WHERE singleton = TRUE; diff --git a/go/internal/store/queries/agent_placements.sql b/go/internal/store/queries/agent_placements.sql index be2bc4e2..4c28f5d0 100644 --- a/go/internal/store/queries/agent_placements.sql +++ b/go/internal/store/queries/agent_placements.sql @@ -8,8 +8,7 @@ INSERT INTO agent_placements (agent_account_id, runner_id, container_name) VALUES ($1, $2, $3) ON CONFLICT (agent_account_id) DO UPDATE SET runner_id = EXCLUDED.runner_id, - container_name = EXCLUDED.container_name, - updated_at = now(); + container_name = EXCLUDED.container_name; -- name: AgentForContainer :one SELECT agent_account_id FROM agent_placements WHERE container_name = $1; diff --git a/go/internal/store/queries/forge_cursors.sql b/go/internal/store/queries/forge_cursors.sql index 2b29ab97..c187cc96 100644 --- a/go/internal/store/queries/forge_cursors.sql +++ b/go/internal/store/queries/forge_cursors.sql @@ -13,7 +13,7 @@ WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3; -- name: StoreForgeRepoWatermark :execrows UPDATE forge_repo_subscriptions - SET swept_updated_at = $4, list_etag = $5, updated_at = now() + SET swept_updated_at = $4, list_etag = $5 WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3; -- name: EnsureForgeRepoSubscription :exec @@ -40,5 +40,5 @@ ORDER BY repo ASC; -- name: SetForgeRepoSubscriptionEnabled :execrows UPDATE forge_repo_subscriptions - SET enabled = $4, updated_at = now() + SET enabled = $4 WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3; diff --git a/go/internal/store/queries/model_registry.sql b/go/internal/store/queries/model_registry.sql index 18dec1bf..18293874 100644 --- a/go/internal/store/queries/model_registry.sql +++ b/go/internal/store/queries/model_registry.sql @@ -25,7 +25,7 @@ RETURNING version; -- ErrVersionConflict. -- name: UpdateModelRegistry :one UPDATE model_registry - SET registry = $1, version = version + 1, updated_at = now() + SET registry = $1, version = version + 1 WHERE singleton = TRUE AND version = $2 RETURNING version; diff --git a/go/internal/store/updated_at_pgtest_test.go b/go/internal/store/updated_at_pgtest_test.go new file mode 100644 index 00000000..83441829 --- /dev/null +++ b/go/internal/store/updated_at_pgtest_test.go @@ -0,0 +1,189 @@ +//go:build pgtest + +package store + +// The set_updated_at trigger convention (RIG-3495), proven against real +// Postgres. updated_at is maintained by ONE mechanism — the BEFORE UPDATE +// trigger 0001_init.sql installs on every table carrying the column — and no +// query file sets it by hand. These tests are the enforcement of that: they +// fail if the trigger is missing, which is exactly what happens if someone +// re-adds a table without an updated_at_tables entry, or drops the block. +// +// Three properties, each a distinct failure mode: +// +// 1. A plain UPDATE advances updated_at and leaves created_at alone. The +// created_at half is not padding: it is what catches a BEFORE INSERT OR +// UPDATE trigger, which would look correct on every update assertion while +// silently making created_at meaningless. +// 2. The INSERT ... ON CONFLICT DO UPDATE conflict path fires it. This is the +// case most likely to be wrong, since the trigger is declared on UPDATE and +// an upsert reads as an insert. Driven through the real store method +// (RecordAgentPlacement), not raw SQL, so the covered thing is the path +// production takes. +// 3. secrets.updated_at is live — the rot this change fixes. +// +// now() is TRANSACTION time in Postgres, so two writes inside one transaction +// observe the identical value. Every write below is its own statement on the +// pool (its own implicit transaction), which is what makes a strict > correct. +// A time.Sleep would only mask a wrong assertion; there is none here. +// +// context.Background is the test root (the pgtest-suite convention, sibling +// forge_cursors_pgtest_test.go). + +import ( + "context" + "testing" + "time" +) + +// TestUpdatedAtTriggerAdvancesOnUpdate proves property 1 on +// forge_repo_subscriptions: SetForgeRepoSubscriptionEnabled is a plain UPDATE +// that no longer sets updated_at itself, so an advanced value can only have +// come from the trigger — and created_at must not move with it. +func TestUpdatedAtTriggerAdvancesOnUpdate(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + + sub := ForgeRepoSubscription{ + Provider: ForgeProviderGitHub, + Host: "github.com", + Repo: "a/b", + Enabled: true, + } + if err := s.EnsureForgeRepoSubscription(ctx, sub); err != nil { + t.Fatalf("EnsureForgeRepoSubscription: %v", err) + } + createdBefore, updatedBefore := repoSubStamps(t, s, sub) + if !updatedBefore.Equal(createdBefore) { + t.Fatalf("on insert updated_at %v != created_at %v; both default now() in one statement", + updatedBefore, createdBefore) + } + + // Separate statement => separate transaction => a strictly later now(). + if err := s.SetForgeRepoSubscriptionEnabled(ctx, sub.Provider, sub.Host, sub.Repo, false); err != nil { + t.Fatalf("SetForgeRepoSubscriptionEnabled: %v", err) + } + createdAfter, updatedAfter := repoSubStamps(t, s, sub) + + if !updatedAfter.After(updatedBefore) { + t.Errorf("updated_at did not advance on UPDATE: before=%v after=%v (set_updated_at trigger missing?)", + updatedBefore, updatedAfter) + } + if !createdAfter.Equal(createdBefore) { + t.Errorf("created_at moved on UPDATE: before=%v after=%v (trigger fires on INSERT too?)", + createdBefore, createdAfter) + } +} + +// TestUpdatedAtTriggerFiresOnUpsertConflict proves property 2: an +// INSERT ... ON CONFLICT DO UPDATE takes the UPDATE path on conflict and so +// fires the BEFORE UPDATE trigger. RecordAgentPlacement is that upsert, and its +// query no longer carries a hand-written updated_at. +func TestUpdatedAtTriggerFiresOnUpsertConflict(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + owner := mustUser(t, s, "placement-owner") + agent := mustAgent(t, s, owner.ID, "placed") + + if err := s.RecordAgentPlacement(ctx, agent.ID, "runner-1", "compass-agent-1"); err != nil { + t.Fatalf("RecordAgentPlacement (insert): %v", err) + } + createdBefore, updatedBefore := placementStamps(t, s, agent.ID) + + // Same agent_account_id => the ON CONFLICT DO UPDATE arm. + if err := s.RecordAgentPlacement(ctx, agent.ID, "runner-2", "compass-agent-1"); err != nil { + t.Fatalf("RecordAgentPlacement (conflict): %v", err) + } + createdAfter, updatedAfter := placementStamps(t, s, agent.ID) + + if !updatedAfter.After(updatedBefore) { + t.Errorf("updated_at did not advance on the upsert conflict path: before=%v after=%v", + updatedBefore, updatedAfter) + } + if !createdAfter.Equal(createdBefore) { + t.Errorf("created_at moved on the upsert conflict path: before=%v after=%v", + createdBefore, createdAfter) + } +} + +// TestSecretsUpdatedAtIsLive proves property 3 — the specific rot RIG-3495 +// fixes. secrets.updated_at is declared, read by DeclaredSecrets, and surfaced +// on SecretDeclaration.UpdatedAt, but queries/secrets.sql has only an INSERT and +// a DELETE: no write path ever set the column, so its value could never differ +// from created_at and every reader was reading a lie. +// +// The store therefore still has no update method for a secret, and this test +// does NOT invent one. It asserts what the store's own surface can show (a +// freshly declared row has updated_at == created_at), and then drives a bare +// UPDATE on the table to prove the trigger is ARMED on secrets — so the column +// becomes correct for free the moment a re-declare/rotate path is added, rather +// than needing whoever adds it to remember. +func TestSecretsUpdatedAtIsLive(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + actor := mustUser(t, s, "secrets-owner") + + if err := s.DeclareSecret(ctx, actor.ID, "DATABASE_URL", SecretDeliveryEnv, SecretKindGeneric, "", ""); err != nil { + t.Fatalf("DeclareSecret: %v", err) + } + createdBefore, updatedBefore := secretStamps(t, s, "DATABASE_URL") + if !updatedBefore.Equal(createdBefore) { + t.Fatalf("on declare updated_at %v != created_at %v; both default now() in one statement", + updatedBefore, createdBefore) + } + + // A mutation of the row's own data, in its own transaction. The statement + // deliberately does NOT mention updated_at: only the trigger can move it. + if _, err := s.pool.Exec(ctx, + `UPDATE secrets SET delivery = $2 WHERE name = $1`, + "DATABASE_URL", int32(SecretDeliveryFile), + ); err != nil { + t.Fatalf("update secret delivery: %v", err) + } + createdAfter, updatedAfter := secretStamps(t, s, "DATABASE_URL") + + if !updatedAfter.After(updatedBefore) { + t.Errorf("secrets.updated_at did not advance on UPDATE: before=%v after=%v (the rot is not fixed)", + updatedBefore, updatedAfter) + } + if !createdAfter.Equal(createdBefore) { + t.Errorf("secrets.created_at moved on UPDATE: before=%v after=%v", + createdBefore, createdAfter) + } +} + +// repoSubStamps reads a forge repo subscription's (created_at, updated_at). +func repoSubStamps(t *testing.T, s *Store, sub ForgeRepoSubscription) (created, updated time.Time) { + t.Helper() + if err := s.pool.QueryRow(context.Background(), + `SELECT created_at, updated_at FROM forge_repo_subscriptions + WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3`, + int16(sub.Provider), sub.Host, sub.Repo, + ).Scan(&created, &updated); err != nil { + t.Fatalf("read forge_repo_subscriptions stamps: %v", err) + } + return created, updated +} + +// placementStamps reads an agent placement's (created_at, updated_at). +func placementStamps(t *testing.T, s *Store, agent AccountID) (created, updated time.Time) { + t.Helper() + if err := s.pool.QueryRow(context.Background(), + `SELECT created_at, updated_at FROM agent_placements WHERE agent_account_id = $1`, + string(agent), + ).Scan(&created, &updated); err != nil { + t.Fatalf("read agent_placements stamps: %v", err) + } + return created, updated +} + +// secretStamps reads a declared secret's (created_at, updated_at). +func secretStamps(t *testing.T, s *Store, name string) (created, updated time.Time) { + t.Helper() + if err := s.pool.QueryRow(context.Background(), + `SELECT created_at, updated_at FROM secrets WHERE name = $1`, name, + ).Scan(&created, &updated); err != nil { + t.Fatalf("read secrets stamps: %v", err) + } + return created, updated +}