diff --git a/internal/store/pg/providers.go b/internal/store/pg/providers.go index bfe8b4d4..b76f82ab 100644 --- a/internal/store/pg/providers.go +++ b/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 { diff --git a/internal/store/sqlitestore/heartbeat_provider_fk_migration_test.go b/internal/store/sqlitestore/heartbeat_provider_fk_migration_test.go new file mode 100644 index 00000000..d3cc0a98 --- /dev/null +++ b/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 +} diff --git a/internal/store/sqlitestore/providers.go b/internal/store/sqlitestore/providers.go index 81d8149e..8679dfb3 100644 --- a/internal/store/sqlitestore/providers.go +++ b/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 { diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index 6d9ce401..49a15109 100644 --- a/internal/store/sqlitestore/schema.go +++ b/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. diff --git a/internal/store/sqlitestore/schema.sql b/internal/store/sqlitestore/schema.sql index 266353fc..05e8ddff 100644 --- a/internal/store/sqlitestore/schema.sql +++ b/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, diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index 83df8b2e..fc18492d 100644 --- a/internal/upgrade/version.go +++ b/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 diff --git a/migrations/000057_heartbeat_provider_fk_set_null.down.sql b/migrations/000057_heartbeat_provider_fk_set_null.down.sql new file mode 100644 index 00000000..a2991a9a --- /dev/null +++ b/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); diff --git a/migrations/000057_heartbeat_provider_fk_set_null.up.sql b/migrations/000057_heartbeat_provider_fk_set_null.up.sql new file mode 100644 index 00000000..bcec46ec --- /dev/null +++ b/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;