fix: safely collect runtime revisions in a leader-elected runnable - #2742

GC deletes the Substrate ActorTemplate before a DB delete that can silently no-op

Introduced here? The race is pre-existing, but this PR rewrites both the function and the query and converts loud FK errors into silent no-ops (the new checkpoint guard plus the release step). Right moment to fix the ordering.

How a user hits it: someone deletes an AgentTemplate at the same moment another user (or CI) creates an instance of it. The create request reads the revision just before retirement; GC deletes the ActorTemplate in Substrate; the instance insert commits; the guarded DB DELETE matches 0 rows and returns nil (:exec discards RowsAffected). The instance is stuck in CREATING forever — the create workflow (workflow.go:107) needs the now-deleted ActorTemplate by name — with no error and no self-heal, since a referenced revision never reappears in the unreferenced list.

Fix: make DeleteUnreferencedRuntimeRevision :execrows, delete the DB row first, and only call DeleteActorTemplate when a row was actually removed. The DB delete is the guarded, reversible half; let it decide.

Fixed in 06ad033 with a durable deletion claim before the Substrate call. Reference acquisition and claiming serialize on the revision row; the row and runtime UID remain until compute deletion succeeds. PostgreSQL tests cover both race orderings, including an instance that selected its revision before retirement, and controller tests cover failed/lost delete responses and restart. Finalization also checks the UID against delayed collectors.

A transient compile failure now silently destroys the pair's last-good revision

Introduced here? Partially. On main a compile failure also deleted the ActorTemplate, but the RESTRICT FK made the revision-row delete fail loudly and the row survived. With ReleaseRetiredRuntimeRevisionReferences, the destruction completes silently, and reactivation cannot recover (the upsert's ON CONFLICT never touches latest_successful_revision).

How a user hits it: edit a running agent's AgentTemplate to reference a ModelConfig/Secret/MCP server that does not exist yet — a typo, or applying the template before its Secret. Any CompileAgentTemplate error yields state.Revision == nil (only Failure is set), so this branch retires the pair; GC then releases the retired pointer and deletes the revision row and ActorTemplate. Fix the typo a minute later: the pair reactivates with latest_successful_revision = NULL, so every CreateAgentInstance returns NotFound until a whole new golden snapshot bakes. Note the asymmetry: post-compile failures (state.Failure != nil with a revision) keep the pair active and preserve the pointer — only compile-stage failures get the destructive treatment.

Fix: do not retire on compile failure — retire here only when state.Failure == nil, so a bad ref behaves like every other failure path: pair stays active, last-good preserved, GC blocked.

Fixed: compile failure preserves the current template/harness UID pair and its last-good runtime, while still retiring a replaced UID at the same name. Controller/PostgreSQL coverage exercises both cases; the real Substrate lifecycle E2E also invokes last-good inputs after an invalid edit.

The release UPDATE commits even when the DELETE no-ops — and the whole mechanism is hand-rolled ON DELETE SET NULL

Introduced here? Yes — ReleaseRetiredRuntimeRevisionReferences is new in this PR.

How a user hits it: a revision is listed as unreferenced, but an instance or checkpoint reference lands before the delete runs (this PR's own TestRuntimeRevisionCollectionPreservesInstanceAndCheckpoint sits in exactly this state on every assertRetained()). The DELETE no-ops but the retired pair's latest_successful_revision has already been nulled and the tx commits. If that pair is later reactivated (re-apply the same template), instance creation NotFounds until a new snapshot bakes — even though the revision row still exists.

Fix: delete this query, the withTx wrapper, and the dbgen method; change the FK in migrations/core/000001_initial.sql:56 to ON DELETE SET NULL. It fires only when the row is actually deleted, and it is safe: the DELETE's own guards still protect active pairs, and the only reader (GetLatestRuntimeRevisionForInstance) inner-joins with retired_at IS NULL. One word of schema replaces ~30 lines.

Fixed: finalization must find a claimed revision with the expected runtime UID before releasing any retired success pointers. A skipped finalization leaves those pointers intact; clearing and deletion remain one transaction with RESTRICT preserved. The regression test retires, attempts an unclaimed finalization, reactivates, and successfully creates from last-good.

The unguarded retire inside this tx rewrites the active row — and every historical retired row — on every reconcile, including the 1 Hz pending poll

Introduced here? Yes — main ran the upsert as a single statement.

How a user hits it: create 20 AgentTemplates. While their snapshots bake, pollPendingTemplates re-enqueues each pending pair every second, and each reconcile runs BEGIN / retire-UPDATE / upsert / COMMIT. RetireAgentTemplateHarnessPair has no UID exclusion and no retired_at IS NULL filter, and SET updated_at = NOW() is unconditional — so every pass writes N+1 dead tuples (N = lifetime UID generations at that name; pair rows are never deleted). Shows up as Postgres bloat and autovacuum churn scaling with template count, worst when Substrate is slow and pairs sit pending.

Fix: use a dedicated query inside this tx: same UPDATE plus AND retired_at IS NULL AND (agent_template_uid, harness_uid) IS DISTINCT FROM ($4, $5). Semantics preserved — the upsert reactivates that exact UID anyway — and steady state writes zero retire tuples. Do not change the shared RetireAgentTemplateHarnessPair; the reconciler's two other call sites need the current UID retired.

Fixed: the replacement retirement query now updates only active rows with different template/harness UIDs. The current UID is never retired during its upsert, and already-retired history is not rewritten by pending-template polls.

The three-clause "unreferenced" predicate is now copy-pasted between List and Delete

Introduced here? The duplication predates the PR, but this diff is the first time it bit: the checkpoint clause had to be hand-added to both copies (four counting dbgen).

How it bites: the next table that references runtime_revision gets added to one query only. Missing from Delete is FK-backstopped; missing from List hands GC a revision whose ActorTemplate then gets destroyed out from under a live reference (see the ordering comment on cleanupUnreferencedRevisions), or an eternal list/no-op-delete GC loop that nobody notices because neither call errors.

Fix: make one predicate authoritative — DELETE FROM runtime_revision WHERE revision = $1 AND revision IN (<the List query>), or a unreferenced_runtime_revision view in the migration.

Consolidated eligibility into the unreferenced_runtime_revision SQL view, used by both candidate discovery and claiming. Finalization operates on an already-claimed row; reference triggers prevent new references after the claim.

Follow-up (not this PR): FAILED / abandoned-CREATING checkpoints are undeletable and now silently pin their revision and ActorTemplate forever

Introduced here? No — the checkpoint state machine and the RESTRICT FK are pre-existing, and for READY checkpoints this guard is a genuine fix (pre-PR their ActorTemplate got destroyed and GC error-looped). What changes: the pin used to scream via FK violations; now it is silent indefinite retention.

How a user hits it: checkpoint an instance and the snapshot tagging step fails once (S3/Substrate hiccup) → checkpoint lands in FAILED. It is invisible to Get/List (READY filter), BeginDelete rejects it (state IN ('READY','DELETING')), the sole DELETE requires DELETING, and there is no reaper or cascade from instance deletion — so its prepared_revision pins the revision and ActorTemplate for the life of the database. A crash between reserve and finalize is worse: the partial unique index on state='CREATING' blocks all future checkpoints on that instance.

Fix (follow-up issue): allow BeginDelete on FAILED rows and add a reaper for stale CREATING rows. Do not add a state filter to this guard alone — that would resurrect the FK violation.

Tracked separately in #2746. FAILED checkpoints currently cannot enter deletion, and abandoned CREATING reservations need coordinated recovery (the original request can resume them today). GC continues to retain every checkpoint row until its lifecycle owner safely cleans it up; filtering by state would bypass that invariant.

Nit: this branch and the state == nil branch above now do the identical retire-then-cleanup, 8 lines apart with divergent error strings. Merge into one shared exit — this falls out naturally if compile failures stop retiring (see the comment below on line 255).

Simplified alongside the compile-failure fix: the pair is built once, and invalid compilation only retires replaced identities. The branch that retired the current pair on compile failure is gone.

Nit: this test starts its own Postgres container inside the test body (dbtest.StartT + raw pgxpool.New) instead of the database package's TestMain + database.Connect pattern, and the "first"/"replacement" loop fully tears down between iterations — the second pass repeats the first from an empty DB and does not test replacement-over-an-active-pair (TestRuntimeRevisionPairReplacement in the database package covers that). Suggest one iteration, or dropping the test in favor of the database-package coverage.

Replaced the repeated empty deletion cycle with distinct failure scenarios: invalid edits versus replacement UIDs, plus deletion failing before removal or succeeding with its response lost, followed by retry through a new reconciler/store. The integration test now uses database.Connect; it retains its own PostgreSQL fixture because the controller and database tests are separate packages.