diff --git a/internal/backup/db_restore.go b/internal/backup/db_restore.go index 9132902b..fdf2d1d0 100644 --- a/internal/backup/db_restore.go +++ b/internal/backup/db_restore.go @@ -79,8 +79,12 @@ func CheckActiveConnections(ctx context.Context, dsn string) (int, error) { defer db.Close() var count int + // Exclude the gateway's own pool connections, tagged application_name='goclaw' + // (see pg.PoolApplicationName). Otherwise a running gateway blocks its own + // restore on a fresh server (issue #1338). External clients are still counted. query := `SELECT COUNT(*) FROM pg_stat_activity - WHERE datname = $1 AND pid <> pg_backend_pid()` + WHERE datname = $1 AND pid <> pg_backend_pid() + AND application_name <> 'goclaw'` if err := db.QueryRowContext(ctx, query, creds.DBName).Scan(&count); err != nil { return 0, fmt.Errorf("query pg_stat_activity: %w", err) } diff --git a/internal/backup/tenant_discover_pg.go b/internal/backup/tenant_discover_pg.go index 13fd0a17..4e87da21 100644 --- a/internal/backup/tenant_discover_pg.go +++ b/internal/backup/tenant_discover_pg.go @@ -53,6 +53,8 @@ func ValidateTableRegistry(ctx context.Context, db *sql.DB) []string { "pairing_requests": true, "paired_devices": true, "channel_pending_messages": true, "cron_run_logs": true, "team_user_grants": true, + // Ephemeral hook/webhook run history — not restored (regenerated at runtime). + "hook_executions": true, "webhook_calls": true, } var warnings []string diff --git a/internal/backup/tenant_tables.go b/internal/backup/tenant_tables.go index 4ffd2eb3..c68158ce 100644 --- a/internal/backup/tenant_tables.go +++ b/internal/backup/tenant_tables.go @@ -19,6 +19,8 @@ type TableDef struct { HasTenantID bool // direct tenant_id column ScopeColumn string // explicit filter column when not tenant_id (e.g. tenants.id) ParentJoin string // JOIN clause for tables without direct tenant_id + OrderBy string // deterministic export ordering; defaults to "id" when empty. + // Tables without an `id` column (composite PKs) MUST set this to real columns. } // TenantTables returns all tenant-scoped tables in FK dependency order (parents first). @@ -35,7 +37,7 @@ func TenantTables() []TableDef { {Name: "agents", Tier: 2, HasTenantID: true}, {Name: "sessions", Tier: 2, HasTenantID: true}, {Name: "api_keys", Tier: 2, HasTenantID: true}, - {Name: "config_secrets", Tier: 2, HasTenantID: true}, + {Name: "config_secrets", Tier: 2, HasTenantID: true, OrderBy: "key"}, // PK (key, tenant_id) — no id column {Name: "skills", Tier: 2, HasTenantID: true}, {Name: "mcp_servers", Tier: 2, HasTenantID: true}, {Name: "secure_cli_binaries", Tier: 2, HasTenantID: true}, @@ -43,11 +45,13 @@ func TenantTables() []TableDef { {Name: "channel_instances", Tier: 2, HasTenantID: true}, {Name: "agent_teams", Tier: 2, HasTenantID: true}, {Name: "llm_providers", Tier: 2, HasTenantID: true}, + {Name: "hooks", Tier: 2, HasTenantID: true}, + {Name: "tenant_hook_budget", Tier: 2, HasTenantID: true, OrderBy: "tenant_id"}, // PK tenant_id — no id column // Tier 3: FK to Tier 2 {Name: "agent_context_files", Tier: 3, HasTenantID: true}, {Name: "user_context_files", Tier: 3, HasTenantID: true}, - {Name: "user_agent_profiles", Tier: 3, HasTenantID: true}, + {Name: "user_agent_profiles", Tier: 3, HasTenantID: true, OrderBy: "agent_id, user_id"}, // PK (agent_id, user_id) — no id column {Name: "user_agent_overrides", Tier: 3, HasTenantID: true}, {Name: "episodic_summaries", Tier: 3, HasTenantID: true}, {Name: "memory_documents", Tier: 3, HasTenantID: true}, @@ -59,7 +63,7 @@ func TenantTables() []TableDef { {Name: "agent_evolution_suggestions", Tier: 3, HasTenantID: true}, {Name: "channel_contacts", Tier: 3, HasTenantID: true}, {Name: "subagent_tasks", Tier: 3, HasTenantID: true}, - {Name: "agent_team_members", Tier: 3, HasTenantID: true}, + {Name: "agent_team_members", Tier: 3, HasTenantID: true, OrderBy: "team_id, agent_id"}, // PK (team_id, agent_id) — no id column {Name: "agent_links", Tier: 3, HasTenantID: true}, {Name: "agent_shares", Tier: 3, HasTenantID: true}, {Name: "agent_config_permissions", Tier: 3, HasTenantID: true}, @@ -71,9 +75,18 @@ func TenantTables() []TableDef { {Name: "mcp_user_credentials", Tier: 3, HasTenantID: true}, {Name: "secure_cli_agent_grants", Tier: 3, HasTenantID: true}, {Name: "secure_cli_user_credentials", Tier: 3, HasTenantID: true}, - {Name: "system_configs", Tier: 3, HasTenantID: true}, - {Name: "builtin_tool_tenant_configs", Tier: 3, HasTenantID: true}, - {Name: "skill_tenant_configs", Tier: 3, HasTenantID: true}, + {Name: "system_configs", Tier: 3, HasTenantID: true, OrderBy: "key"}, // PK (key, tenant_id) — no id column + {Name: "builtin_tool_tenant_configs", Tier: 3, HasTenantID: true, OrderBy: "tool_name"}, // PK (tool_name, tenant_id) — no id column + {Name: "skill_tenant_configs", Tier: 3, HasTenantID: true, OrderBy: "skill_id"}, // PK (skill_id, tenant_id) — no id column + {Name: "webhooks", Tier: 3, HasTenantID: true}, // FK -> agents, channel_instances + // hook_agents has no tenant_id — filter via JOIN hooks; composite PK (hook_id, agent_id). + { + Name: "hook_agents", + Tier: 3, + HasTenantID: false, + ParentJoin: "hook_agents vl JOIN hooks h ON vl.hook_id = h.id WHERE h.tenant_id = $1", + OrderBy: "vl.hook_id, vl.agent_id", + }, // Tier 4: FK to Tier 3 {Name: "kg_relations", Tier: 4, HasTenantID: true}, @@ -105,7 +118,13 @@ func (t TableDef) tenantFilterColumn() string { func (t TableDef) exportQuery() (string, error) { if t.ParentJoin != "" { - return fmt.Sprintf("SELECT vl.* FROM %s ORDER BY vl.id", t.ParentJoin), nil + // ParentJoin aliases the child table as `vl`. Order by OrderBy when set + // (required for composite-PK children like hook_agents that have no id). + orderBy := t.OrderBy + if orderBy == "" { + orderBy = "vl.id" + } + return fmt.Sprintf("SELECT vl.* FROM %s ORDER BY %s", t.ParentJoin, orderBy), nil } column := t.tenantFilterColumn() @@ -113,7 +132,11 @@ func (t TableDef) exportQuery() (string, error) { return "", fmt.Errorf("table %s: no tenant filter defined", t.Name) } - return fmt.Sprintf("SELECT * FROM %s WHERE %s = $1 ORDER BY id", t.Name, column), nil + orderBy := t.OrderBy + if orderBy == "" { + orderBy = "id" + } + return fmt.Sprintf("SELECT * FROM %s WHERE %s = $1 ORDER BY %s", t.Name, column, orderBy), nil } func (t TableDef) deleteQuery() (string, error) { diff --git a/internal/backup/tenant_tables_test.go b/internal/backup/tenant_tables_test.go index f9180593..79e6cafa 100644 --- a/internal/backup/tenant_tables_test.go +++ b/internal/backup/tenant_tables_test.go @@ -1,6 +1,76 @@ package backup -import "testing" +import ( + "strings" + "testing" +) + +// TestExportQueryNeverOrdersByMissingIdColumn guards issue #1076/#1338: several +// tenant-scoped tables have no `id` column (config_secrets PK is (key,tenant_id), +// agent_team_members PK is (team_id,agent_id), tenant_hook_budget PK is tenant_id). +// exportQuery() previously hardcoded `ORDER BY id`, so exporting them failed with +// SQLSTATE 42703 "column id does not exist". Their registry entries must set an +// OrderBy that references real columns. +func TestExportQueryNeverOrdersByMissingIdColumn(t *testing.T) { + idless := map[string]bool{ + "config_secrets": true, + "agent_team_members": true, + "tenant_hook_budget": true, + "user_agent_profiles": true, + "system_configs": true, + "builtin_tool_tenant_configs": true, + "skill_tenant_configs": true, + } + lookup := make(map[string]TableDef) + for _, tbl := range TenantTables() { + lookup[tbl.Name] = tbl + } + for name := range idless { + tbl, ok := lookup[name] + if !ok { + t.Errorf("%s missing from backup registry", name) + continue + } + q, err := tbl.exportQuery() + if err != nil { + t.Errorf("%s exportQuery error: %v", name, err) + continue + } + if strings.Contains(q, "ORDER BY id") { + t.Errorf("%s export orders by missing id column: %q", name, q) + } + } +} + +// TestHookWebhookFamilyRegistered guards issue #1076: hooks + budget + webhook +// config tables were missing from the backup registry, so their data was silently +// dropped on tenant backup/restore. The junction hook_agents (no tenant_id) must +// export via a ParentJoin through hooks and order by its composite PK. +func TestHookWebhookFamilyRegistered(t *testing.T) { + lookup := make(map[string]TableDef) + for _, tbl := range TenantTables() { + lookup[tbl.Name] = tbl + } + for _, name := range []string{"hooks", "tenant_hook_budget", "webhooks", "hook_agents"} { + if _, ok := lookup[name]; !ok { + t.Errorf("%s missing from backup registry", name) + } + } + ha, ok := lookup["hook_agents"] + if !ok { + return + } + q, err := ha.exportQuery() + if err != nil { + t.Fatalf("hook_agents exportQuery error: %v", err) + } + if !strings.Contains(q, "JOIN hooks") { + t.Errorf("hook_agents export must join hooks for tenant scope: %q", q) + } + if strings.Contains(q, "ORDER BY vl.id") { + t.Errorf("hook_agents has no id column; export must not order by vl.id: %q", q) + } +} func TestTenantTablesIncludesTenantUsersAndTenantScope(t *testing.T) { tables := TenantTables() diff --git a/internal/store/pg/pool.go b/internal/store/pg/pool.go index 65d3bd14..b2251f8a 100644 --- a/internal/store/pg/pool.go +++ b/internal/store/pg/pool.go @@ -5,15 +5,29 @@ import ( "fmt" "log/slog" - _ "github.com/jackc/pgx/v5/stdlib" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/stdlib" ) +// PoolApplicationName tags every gateway pool connection via the PostgreSQL +// application_name runtime parameter. Pre-restore safety checks use it to tell +// the gateway's own connections apart from external clients (issue #1338). +const PoolApplicationName = "goclaw" + // OpenDB creates a database/sql connection to Postgres using pgx driver. func OpenDB(dsn string) (*sql.DB, error) { - db, err := sql.Open("pgx", dsn) + config, err := pgx.ParseConfig(dsn) if err != nil { - return nil, fmt.Errorf("open postgres: %w", err) + return nil, fmt.Errorf("parse postgres dsn: %w", err) } + if config.RuntimeParams == nil { + config.RuntimeParams = map[string]string{} + } + // Respect a caller-supplied application_name; otherwise tag as the gateway. + if config.RuntimeParams["application_name"] == "" { + config.RuntimeParams["application_name"] = PoolApplicationName + } + db := stdlib.OpenDB(*config) db.SetMaxOpenConns(25) db.SetMaxIdleConns(10) diff --git a/tests/integration/backup_active_conns_test.go b/tests/integration/backup_active_conns_test.go new file mode 100644 index 00000000..ce17248f --- /dev/null +++ b/tests/integration/backup_active_conns_test.go @@ -0,0 +1,70 @@ +//go:build integration + +package integration + +import ( + "context" + "database/sql" + "os" + "testing" + + _ "github.com/jackc/pgx/v5/stdlib" + + "github.com/nextlevelbuilder/goclaw/internal/backup" + "github.com/nextlevelbuilder/goclaw/internal/store/pg" +) + +// TestBackup_CheckActiveConnections_ExcludesGatewayPool is a regression test for +// issue #1338: restoring on a fresh server failed with "N active DB connection(s) +// detected" because the gateway's own pool connections were counted as active +// clients. Gateway pool connections are tagged application_name='goclaw' and must +// be excluded; genuinely external connections must still be counted. +func TestBackup_CheckActiveConnections_ExcludesGatewayPool(t *testing.T) { + testDB(t) // skips if PG unavailable + dsn := os.Getenv("TEST_DATABASE_URL") + if dsn == "" { + dsn = defaultTestDSN + } + ctx := context.Background() + + base, err := backup.CheckActiveConnections(ctx, dsn) + if err != nil { + t.Fatalf("baseline CheckActiveConnections: %v", err) + } + + // A gateway-tagged pool connection must NOT be counted. + pool, err := pg.OpenDB(dsn) + if err != nil { + t.Fatalf("open gateway pool: %v", err) + } + defer pool.Close() + var one int + if err := pool.QueryRowContext(ctx, "SELECT 1").Scan(&one); err != nil { + t.Fatalf("warm gateway pool: %v", err) + } + + after, err := backup.CheckActiveConnections(ctx, dsn) + if err != nil { + t.Fatalf("CheckActiveConnections after gateway pool: %v", err) + } + if after != base { + t.Errorf("gateway pool connection was counted as active: base=%d after=%d", base, after) + } + + // A non-gateway connection MUST be counted. + external, err := sql.Open("pgx", dsn) + if err != nil { + t.Fatalf("open external conn: %v", err) + } + defer external.Close() + if err := external.QueryRowContext(ctx, "SELECT 1").Scan(&one); err != nil { + t.Fatalf("warm external conn: %v", err) + } + withExternal, err := backup.CheckActiveConnections(ctx, dsn) + if err != nil { + t.Fatalf("CheckActiveConnections with external: %v", err) + } + if withExternal <= after { + t.Errorf("external connection not counted: after=%d withExternal=%d", after, withExternal) + } +} diff --git a/tests/integration/backup_export_registry_test.go b/tests/integration/backup_export_registry_test.go new file mode 100644 index 00000000..426d48b9 --- /dev/null +++ b/tests/integration/backup_export_registry_test.go @@ -0,0 +1,32 @@ +//go:build integration + +package integration + +import ( + "bytes" + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/backup" +) + +// TestBackup_ExportAllRegistryTables_NoSQLError is an end-to-end regression for +// issue #1076/#1338: exporting config_secrets (and other composite-PK tables) +// failed with SQLSTATE 42703 "column id does not exist" because exportQuery +// hardcoded ORDER BY id. PostgreSQL validates the ORDER BY column at plan time, +// so the export fails even for an empty tenant. Running ExportTable over every +// registered table proves each query is valid against the real schema — including +// the hook_agents ParentJoin. +func TestBackup_ExportAllRegistryTables_NoSQLError(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + + for _, tbl := range backup.TenantTables() { + t.Run(tbl.Name, func(t *testing.T) { + var buf bytes.Buffer + if _, err := backup.ExportTable(context.Background(), db, tbl, tenantID, &buf); err != nil { + t.Errorf("ExportTable(%s): %v", tbl.Name, err) + } + }) + } +}