fix(providers): heartbeat FK SET NULL + tenant-scoped tx-safe DeleteProvider (#1034)

internal/store/pg/providers.go

@@ -165,11 +165,47 @@ func (s *PGProviderStore) DeleteProvider(ctx context.Context, id uuid.UUID) erro
 	if err != nil {
 		return err
 	}
-	_, err = s.db.ExecContext(ctx,
+	tx, err := s.db.BeginTx(ctx, nil)
+	if err != nil {
+		return err
+	}
+	// Safe no-op after Commit (returns sql.ErrTxDone, ignored).
+	defer tx.Rollback()
+
+	// Defensive: disable heartbeats so the next scheduler tick after delete
+	// cannot fire stale config. FK ON DELETE SET NULL clears provider_id auto.
+	// Tenant-scope the UPDATE through agents to prevent cross-tenant side effects:
+	// even though provider IDs are UUIDs (globally unique), an attacker who guessed
+	// or leaked one could otherwise disable another tenant's heartbeats.
+	// IsCrossTenant (master scope) bypasses scoping for legitimate cross-tenant admin.
+	var updateQuery string
+	var updateArgs []any
+	if store.IsCrossTenant(ctx) {
+		updateQuery = "UPDATE agent_heartbeats SET enabled = false WHERE provider_id = $1"
+		updateArgs = []any{id}
+	} else {
+		tid := store.TenantIDFromContext(ctx)
+		updateQuery = `UPDATE agent_heartbeats SET enabled = false
+		               WHERE provider_id = $1
+		                 AND agent_id IN (SELECT id FROM agents WHERE tenant_id = $2)`
+		updateArgs = []any{id, tid}
+	}
+	res, err := tx.ExecContext(ctx, updateQuery, updateArgs...)
+	if err != nil {
+		return err
+	}
+	if n, _ := res.RowsAffected(); n > 0 {
+		slog.Warn("heartbeat.provider_cleared",
+			"provider_id", id, "heartbeats_disabled", n)
+	}
+
+	if _, err := tx.ExecContext(ctx,
 		"DELETE FROM llm_providers WHERE id = $1"+tClause,
 		append([]any{id}, tArgs...)...,
-	)
-	return err
+	); err != nil {
+		return err
+	}
+	return tx.Commit()
 }
 
 func (s *PGProviderStore) decryptKey(apiKey, providerName string) string {

internal/store/sqlitestore/heartbeat_provider_fk_migration_test.go

@@ -0,0 +1,154 @@
+//go:build sqlite || sqliteonly
+
+package sqlitestore
+
+import (
+	"database/sql"
+	"strings"
+	"testing"
+)
+
+// TestSQLiteSchemaUpgrade_25_to_26_HeartbeatFKSetNull verifies the v25→v26
+// rebuild of agent_heartbeats:
+//   - replaces FK clause with ON DELETE SET NULL
+//   - preserves existing rows (id values) under FK toggle
+//   - leaves heartbeat_run_logs FK references intact (resolved by table NAME)
+//   - re-enables PRAGMA foreign_keys=ON after the rebuild
+//   - successfully cascades a provider DELETE to NULL the heartbeat reference
+func TestSQLiteSchemaUpgrade_25_to_26_HeartbeatFKSetNull(t *testing.T) {
+	db := openTestDBAtVersion(t, 25)
+
+	// At v25 the schema.sql was applied (which now has the v26 FK clause), then
+	// version reset to 25. To simulate a real pre-v26 DB we must rebuild
+	// agent_heartbeats with the OLD FK clause (no ON DELETE), and toggle FK off
+	// for the rebuild itself.
+	mustExec(t, db, "PRAGMA foreign_keys=OFF")
+	mustExec(t, db, `CREATE TABLE agent_heartbeats_old (
+		id TEXT NOT NULL PRIMARY KEY,
+		agent_id TEXT NOT NULL UNIQUE REFERENCES agents(id) ON DELETE CASCADE,
+		enabled BOOLEAN NOT NULL DEFAULT 0,
+		interval_sec INT NOT NULL DEFAULT 1800,
+		prompt TEXT,
+		provider_id TEXT REFERENCES llm_providers(id),
+		model VARCHAR(200),
+		isolated_session BOOLEAN NOT NULL DEFAULT 1,
+		light_context BOOLEAN NOT NULL DEFAULT 0,
+		ack_max_chars INT NOT NULL DEFAULT 300,
+		max_retries INT NOT NULL DEFAULT 2,
+		active_hours_start VARCHAR(5),
+		active_hours_end VARCHAR(5),
+		timezone TEXT,
+		channel VARCHAR(50),
+		chat_id TEXT,
+		next_run_at TEXT,
+		last_run_at TEXT,
+		last_status VARCHAR(20),
+		last_error TEXT,
+		run_count INT NOT NULL DEFAULT 0,
+		suppress_count INT NOT NULL DEFAULT 0,
+		metadata TEXT DEFAULT '{}',
+		created_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
+		updated_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))
+	)`)
+	mustExec(t, db, `INSERT INTO agent_heartbeats_old SELECT * FROM agent_heartbeats`)
+	mustExec(t, db, `DROP TABLE agent_heartbeats`)
+	mustExec(t, db, `ALTER TABLE agent_heartbeats_old RENAME TO agent_heartbeats`)
+	mustExec(t, db, "PRAGMA foreign_keys=ON")
+
+	// Sanity: confirm we are now at the OLD FK shape.
+	if got := tableSQL(t, db, "agent_heartbeats"); strings.Contains(got, "ON DELETE SET NULL") {
+		t.Fatalf("expected OLD FK shape (no ON DELETE), got: %s", got)
+	}
+
+	// Seed: tenant + agent + provider + heartbeat referencing provider + a
+	// heartbeat_run_log referencing the heartbeat. After the rebuild, the
+	// run_log FK must still resolve to the renamed table.
+	tenantID := "11111111-1111-1111-1111-111111111111"
+	agentID := "22222222-2222-2222-2222-222222222222"
+	providerID := "33333333-3333-3333-3333-333333333333"
+	hbID := "44444444-4444-4444-4444-444444444444"
+	logID := "55555555-5555-5555-5555-555555555555"
+	mustExec(t, db, `INSERT INTO tenants (id, name, slug, status) VALUES (?, 'T', 't26', 'active')`, tenantID)
+	mustExec(t, db, `INSERT INTO agents (id, agent_key, display_name, status, tenant_id, owner_id, model, provider, agent_type) VALUES (?, 'a', 'A', 'active', ?, 'o', 'm', 'p', 'predefined')`, agentID, tenantID)
+	mustExec(t, db, `INSERT INTO llm_providers (id, name, provider_type, api_base, api_key, enabled, settings, tenant_id) VALUES (?, 'p26', 'openai-compat', 'http://x', 'k', 1, '{}', ?)`, providerID, tenantID)
+	mustExec(t, db, `INSERT INTO agent_heartbeats (id, agent_id, enabled, provider_id, model) VALUES (?, ?, 1, ?, 'gpt-4')`, hbID, agentID, providerID)
+	mustExec(t, db, `INSERT INTO heartbeat_run_logs (id, heartbeat_id, agent_id, status) VALUES (?, ?, ?, 'success')`, logID, hbID, agentID)
+
+	// Run the migration v25 → v26.
+	if err := EnsureSchema(db); err != nil {
+		t.Fatalf("EnsureSchema (v25→26) failed: %v", err)
+	}
+
+	// Verify schema version bumped.
+	var version int
+	db.QueryRow("SELECT version FROM schema_version LIMIT 1").Scan(&version)
+	if version != SchemaVersion {
+		t.Errorf("schema version = %d, want %d", version, SchemaVersion)
+	}
+
+	// Verify FK clause changed.
+	if got := tableSQL(t, db, "agent_heartbeats"); !strings.Contains(got, "ON DELETE SET NULL") {
+		t.Fatalf("expected FK ON DELETE SET NULL after migration, got: %s", got)
+	}
+
+	// Verify heartbeat row preserved (same id).
+	var preservedID string
+	if err := db.QueryRow(`SELECT id FROM agent_heartbeats WHERE id = ?`, hbID).Scan(&preservedID); err != nil {
+		t.Fatalf("heartbeat row missing after rebuild: %v", err)
+	}
+
+	// Verify heartbeat_run_log FK still resolves to the renamed table.
+	var logFKResolved string
+	if err := db.QueryRow(
+		`SELECT l.id FROM heartbeat_run_logs l JOIN agent_heartbeats h ON l.heartbeat_id = h.id WHERE l.id = ?`,
+		logID,
+	).Scan(&logFKResolved); err != nil {
+		t.Fatalf("heartbeat_run_logs FK broken after rebuild: %v", err)
+	}
+
+	// Verify foreign_keys pragma is back to ON.
+	var fkOn int
+	db.QueryRow("PRAGMA foreign_keys").Scan(&fkOn)
+	if fkOn != 1 {
+		t.Errorf("PRAGMA foreign_keys = %d after migration, want 1 (ON)", fkOn)
+	}
+
+	// Verify ON DELETE SET NULL semantics: deleting the provider sets
+	// heartbeat.provider_id NULL instead of erroring.
+	if _, err := db.Exec(`DELETE FROM llm_providers WHERE id = ?`, providerID); err != nil {
+		t.Fatalf("DELETE provider must succeed under SET NULL FK, got: %v", err)
+	}
+	var providerIDAfter *string
+	db.QueryRow(`SELECT provider_id FROM agent_heartbeats WHERE id = ?`, hbID).Scan(&providerIDAfter)
+	if providerIDAfter != nil {
+		t.Fatalf("expected provider_id NULL after provider DELETE, got %v", *providerIDAfter)
+	}
+
+	// Verify foreign_key_check finds no violations.
+	rows, err := db.Query("PRAGMA foreign_key_check")
+	if err != nil {
+		t.Fatalf("foreign_key_check: %v", err)
+	}
+	defer rows.Close()
+	if rows.Next() {
+		t.Fatal("foreign_key_check reported violations after migration")
+	}
+}
+
+func mustExec(t *testing.T, db *sql.DB, query string, args ...any) {
+	t.Helper()
+	if _, err := db.Exec(query, args...); err != nil {
+		t.Fatalf("exec %q: %v", query, err)
+	}
+}
+
+func tableSQL(t *testing.T, db *sql.DB, name string) string {
+	t.Helper()
+	var sqlText string
+	if err := db.QueryRow(
+		`SELECT sql FROM sqlite_master WHERE type='table' AND name=?`, name,
+	).Scan(&sqlText); err != nil {
+		t.Fatalf("read table sql for %q: %v", name, err)
+	}
+	return sqlText
+}

internal/store/sqlitestore/providers.go

@@ -168,12 +168,46 @@ func (s *SQLiteProviderStore) DeleteProvider(ctx context.Context, id uuid.UUID)
 	if err != nil {
 		return err
 	}
+	tx, err := s.db.BeginTx(ctx, nil)
+	if err != nil {
+		return err
+	}
+	// Safe no-op after Commit.
+	defer tx.Rollback()
+
+	// Defensive: disable heartbeats so the next scheduler tick after delete
+	// cannot fire stale config. FK ON DELETE SET NULL clears provider_id auto.
+	// Tenant-scope the UPDATE through agents to prevent cross-tenant side effects.
+	// IsCrossTenant (master scope) bypasses scoping for legitimate cross-tenant admin.
+	var updateQuery string
+	var updateArgs []any
+	if store.IsCrossTenant(ctx) {
+		updateQuery = "UPDATE agent_heartbeats SET enabled = 0 WHERE provider_id = ?"
+		updateArgs = []any{id}
+	} else {
+		tid := store.TenantIDFromContext(ctx)
+		updateQuery = `UPDATE agent_heartbeats SET enabled = 0
+		               WHERE provider_id = ?
+		                 AND agent_id IN (SELECT id FROM agents WHERE tenant_id = ?)`
+		updateArgs = []any{id, tid}
+	}
+	res, err := tx.ExecContext(ctx, updateQuery, updateArgs...)
+	if err != nil {
+		return err
+	}
+	if n, _ := res.RowsAffected(); n > 0 {
+		slog.Warn("heartbeat.provider_cleared",
+			"provider_id", id, "heartbeats_disabled", n)
+	}
+
 	args := append([]any{id}, tArgs...)
-	_, err = s.db.ExecContext(ctx,
+	if _, err := tx.ExecContext(ctx,
 		"DELETE FROM llm_providers WHERE id = ?"+tClause,
 		args...,
-	)
-	return err
+	); err != nil {
+		return err
+	}
+	return tx.Commit()
 }
 
 func (s *SQLiteProviderStore) decryptKey(apiKey, providerName string) string {

internal/store/sqlitestore/schema.go

@@ -16,7 +16,7 @@ var schemaSQL string
 
 // SchemaVersion is the current SQLite schema version.
 // Bump this when adding new migration steps below.
-const SchemaVersion = 25
+const SchemaVersion = 26
 
 // migrations maps version → SQL to apply when upgrading FROM that version.
 // schema.sql always represents the LATEST full schema (for fresh DBs).
@@ -501,6 +501,66 @@ CREATE TRIGGER IF NOT EXISTS trg_vault_docs_scope_consistency_upd
 	// SQLite lacks regex by default — skip backfill (desktop is single-user; cross-chat risk minimal).
 	24: `ALTER TABLE vault_documents ADD COLUMN chat_id TEXT;
 CREATE INDEX IF NOT EXISTS idx_vault_docs_team_chat ON vault_documents(team_id, chat_id) WHERE team_id IS NOT NULL;`,
+
+	// Version 25 → 26: change agent_heartbeats.provider_id FK to ON DELETE SET NULL
+	// (mirrors PG migration 000057). SQLite cannot ALTER FK clauses, so the table
+	// must be rebuilt. Explicit 25-column INSERT/SELECT to avoid silent column drift.
+	25: `-- Defensive: clear orphan provider_id refs before rebuild (idempotent).
+UPDATE agent_heartbeats
+   SET provider_id = NULL
+ WHERE provider_id IS NOT NULL
+   AND provider_id NOT IN (SELECT id FROM llm_providers);
+
+-- Rebuild table with ON DELETE SET NULL on provider_id FK.
+CREATE TABLE agent_heartbeats_new (
+    id                 TEXT NOT NULL PRIMARY KEY,
+    agent_id           TEXT NOT NULL UNIQUE REFERENCES agents(id) ON DELETE CASCADE,
+    enabled            BOOLEAN NOT NULL DEFAULT 0,
+    interval_sec       INT NOT NULL DEFAULT 1800,
+    prompt             TEXT,
+    provider_id        TEXT REFERENCES llm_providers(id) ON DELETE SET NULL,
+    model              VARCHAR(200),
+    isolated_session   BOOLEAN NOT NULL DEFAULT 1,
+    light_context      BOOLEAN NOT NULL DEFAULT 0,
+    ack_max_chars      INT NOT NULL DEFAULT 300,
+    max_retries        INT NOT NULL DEFAULT 2,
+    active_hours_start VARCHAR(5),
+    active_hours_end   VARCHAR(5),
+    timezone           TEXT,
+    channel            VARCHAR(50),
+    chat_id            TEXT,
+    next_run_at        TEXT,
+    last_run_at        TEXT,
+    last_status        VARCHAR(20),
+    last_error         TEXT,
+    run_count          INT NOT NULL DEFAULT 0,
+    suppress_count     INT NOT NULL DEFAULT 0,
+    metadata           TEXT DEFAULT '{}',
+    created_at         TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
+    updated_at         TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))
+);
+
+INSERT INTO agent_heartbeats_new (
+    id, agent_id, enabled, interval_sec, prompt, provider_id, model,
+    isolated_session, light_context, ack_max_chars, max_retries,
+    active_hours_start, active_hours_end, timezone, channel, chat_id,
+    next_run_at, last_run_at, last_status, last_error,
+    run_count, suppress_count, metadata, created_at, updated_at
+) SELECT
+    id, agent_id, enabled, interval_sec, prompt, provider_id, model,
+    isolated_session, light_context, ack_max_chars, max_retries,
+    active_hours_start, active_hours_end, timezone, channel, chat_id,
+    next_run_at, last_run_at, last_status, last_error,
+    run_count, suppress_count, metadata, created_at, updated_at
+  FROM agent_heartbeats;
+
+DROP TABLE agent_heartbeats;
+ALTER TABLE agent_heartbeats_new RENAME TO agent_heartbeats;
+
+-- Recreate the only index on agent_heartbeats (verified via grep).
+CREATE INDEX IF NOT EXISTS idx_heartbeats_due
+  ON agent_heartbeats(next_run_at)
+  WHERE enabled = 1 AND next_run_at IS NOT NULL;`,
 }
 
 // addHooksTables is the SQLite incremental migration for schema v19 → v20.
@@ -692,23 +752,57 @@ func EnsureSchema(db *sql.DB) error {
 			if !ok {
 				return fmt.Errorf("sqlite: missing migration for version %d → %d", v, v+1)
 			}
+			// Migrations that rebuild a table referenced by another table's FK
+			// require foreign_keys=OFF per SQLite altertable §7. The pragma is
+			// a no-op inside a transaction, so toggle it around BEGIN/COMMIT.
+			// v25 → v26: rebuilds agent_heartbeats; heartbeat_run_logs.heartbeat_id FKs into it.
+			needsFKOff := v == 25
+			if needsFKOff {
+				if _, err := db.Exec("PRAGMA foreign_keys=OFF"); err != nil {
+					return fmt.Errorf("disable FK before v%d: %w", v, err)
+				}
+			}
 			tx, txErr := db.Begin()
 			if txErr != nil {
+				if needsFKOff {
+					_, _ = db.Exec("PRAGMA foreign_keys=ON")
+				}
 				return fmt.Errorf("begin migration tx v%d: %w", v, txErr)
 			}
 			if _, err := tx.Exec(patch); err != nil {
 				tx.Rollback()
+				if needsFKOff {
+					_, _ = db.Exec("PRAGMA foreign_keys=ON")
+				}
 				return fmt.Errorf("apply migration v%d: %w", v, err)
 			}
 			if _, err := tx.Exec(
 				"UPDATE schema_version SET version = ? WHERE version = ?", v+1, v,
 			); err != nil {
 				tx.Rollback()
+				if needsFKOff {
+					_, _ = db.Exec("PRAGMA foreign_keys=ON")
+				}
 				return fmt.Errorf("update schema version v%d: %w", v, err)
 			}
 			if err := tx.Commit(); err != nil {
+				if needsFKOff {
+					_, _ = db.Exec("PRAGMA foreign_keys=ON")
+				}
 				return fmt.Errorf("commit migration v%d: %w", v, err)
 			}
+			if needsFKOff {
+				// Verify referential integrity after the rebuild.
+				if rows, qErr := db.Query("PRAGMA foreign_key_check"); qErr == nil {
+					if rows.Next() {
+						slog.Warn("sqlite: foreign_key_check reported violations after migration", "version", v+1)
+					}
+					rows.Close()
+				}
+				if _, err := db.Exec("PRAGMA foreign_keys=ON"); err != nil {
+					return fmt.Errorf("re-enable FK after v%d: %w", v, err)
+				}
+			}
 			// Post-SQL backfill hooks for migrations needing app-side logic.
 			// modernc.org/sqlite lacks regexp_replace, so the v15 → v16
 			// basename columns must be populated via a Go loop.

internal/store/sqlitestore/schema.sql

@@ -1272,7 +1272,7 @@ CREATE TABLE IF NOT EXISTS agent_heartbeats (
     enabled            BOOLEAN NOT NULL DEFAULT 0,
     interval_sec       INT NOT NULL DEFAULT 1800,
     prompt             TEXT,
-    provider_id        TEXT REFERENCES llm_providers(id),
+    provider_id        TEXT REFERENCES llm_providers(id) ON DELETE SET NULL,
     model              VARCHAR(200),
     isolated_session   BOOLEAN NOT NULL DEFAULT 1,
     light_context      BOOLEAN NOT NULL DEFAULT 0,

internal/upgrade/version.go

@@ -2,4 +2,4 @@ package upgrade
 
 // RequiredSchemaVersion is the schema migration version this binary requires.
 // Bump this whenever adding a new SQL migration file.
-const RequiredSchemaVersion uint = 56
+const RequiredSchemaVersion uint = 57

migrations/000057_heartbeat_provider_fk_set_null.down.sql

@@ -0,0 +1,19 @@
+-- Reverse: drop the SET NULL FK and restore implicit RESTRICT.
+-- Note: pre-existing NULL provider_id rows remain NULL (RESTRICT FK accepts NULL trivially).
+DO $$
+DECLARE
+  cname text;
+BEGIN
+  SELECT conname INTO cname
+    FROM pg_constraint
+   WHERE conrelid = 'agent_heartbeats'::regclass
+     AND confrelid = 'llm_providers'::regclass
+     AND contype = 'f';
+  IF cname IS NOT NULL THEN
+    EXECUTE format('ALTER TABLE agent_heartbeats DROP CONSTRAINT %I', cname);
+  END IF;
+END $$;
+
+ALTER TABLE agent_heartbeats
+  ADD CONSTRAINT agent_heartbeats_provider_id_fkey
+    FOREIGN KEY (provider_id) REFERENCES llm_providers(id);

migrations/000057_heartbeat_provider_fk_set_null.up.sql

@@ -0,0 +1,27 @@
+-- 1. Defensive orphan cleanup before changing FK semantics.
+--    Old FK was RESTRICT so orphans should be impossible, but historical
+--    schema drift could leave dangling provider_id references.
+UPDATE agent_heartbeats
+   SET provider_id = NULL
+ WHERE provider_id IS NOT NULL
+   AND provider_id NOT IN (SELECT id FROM llm_providers);
+
+-- 2. Drop existing FK by lookup (handles auto-gen name drift).
+DO $$
+DECLARE
+  cname text;
+BEGIN
+  SELECT conname INTO cname
+    FROM pg_constraint
+   WHERE conrelid = 'agent_heartbeats'::regclass
+     AND confrelid = 'llm_providers'::regclass
+     AND contype = 'f';
+  IF cname IS NOT NULL THEN
+    EXECUTE format('ALTER TABLE agent_heartbeats DROP CONSTRAINT %I', cname);
+  END IF;
+END $$;
+
+-- 3. Re-add with ON DELETE SET NULL.
+ALTER TABLE agent_heartbeats
+  ADD CONSTRAINT agent_heartbeats_provider_id_fkey
+    FOREIGN KEY (provider_id) REFERENCES llm_providers(id) ON DELETE SET NULL;