diff --git a/.goldenrules-rule17-allowlist b/.goldenrules-rule17-allowlist index 6ea1fee..f950b7e 100644 --- a/.goldenrules-rule17-allowlist +++ b/.goldenrules-rule17-allowlist @@ -19,13 +19,18 @@ # O(N), shows as SCAN in EXPLAIN but not actionable via index) db/action.go:399 SCAN actions -db/db.go:167 SCAN tasks -db/db.go:184 SCAN actions -db/db.go:336 SCAN actions -db/db.go:352 SCAN schedules -db/db.go:399 SCAN CONSTANT -db/db.go:399 SCAN task_action_counts -db/db.go:405 SCAN actions +db/db.go:173 SCAN tasks +db/db.go:190 SCAN actions +db/db.go:348 SCAN actions +db/db.go:364 SCAN schedules +# db/db.go:408 — dropStaleCountUpdateTrigger: sqlite_master lookup, no index possible +db/db.go:408 SCAN sqlite_master +# db/db.go:429-432 — rebuildTaskActionCounts: one-shot full rebuild on trigger migration +db/db.go:429 SCAN task_action_counts +db/db.go:432 SCAN actions +db/db.go:446 SCAN CONSTANT +db/db.go:446 SCAN task_action_counts +db/db.go:452 SCAN actions # db/event.go:76 — ListRecentEvents: LIMIT-bounded PK-order scan # (ORDER BY id DESC LIMIT ?), not a real full scan; no index will help. db/event.go:76 SCAN events diff --git a/db/db.go b/db/db.go index f778812..5674045 100644 --- a/db/db.go +++ b/db/db.go @@ -5,6 +5,7 @@ import ( "database/sql" _ "embed" "encoding/json" + "errors" "fmt" "strings" @@ -102,6 +103,11 @@ func (db *DB) hasColumn(table, column string) (bool, error) { } func (db *DB) Migrate() error { + staleCountTrigger, err := db.dropStaleCountUpdateTrigger() + if err != nil { + return fmt.Errorf("migrate task_action_counts trigger: %w", err) + } + if _, err := db.Exec(schemaSQL); err != nil { return err } @@ -319,6 +325,12 @@ func (db *DB) Migrate() error { return fmt.Errorf("migrate experimental_bg mode: %w", err) } + if staleCountTrigger { + if err := db.rebuildTaskActionCounts(); err != nil { + return fmt.Errorf("rebuild task_action_counts: %w", err) + } + } + if err := db.backfillTaskActionCounts(); err != nil { return fmt.Errorf("backfill task_action_counts: %w", err) } @@ -391,21 +403,57 @@ func (db *DB) backfillSearchFTS() error { return nil } +func (db *DB) dropStaleCountUpdateTrigger() (bool, error) { + var sqlText string + err := db.QueryRow("SELECT sql FROM sqlite_master WHERE type = 'trigger' AND name = 'trg_actions_count_update'").Scan(&sqlText) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("read trigger definition: %w", err) + } + if strings.Contains(sqlText, "OLD.task_id != NEW.task_id") { + return false, nil + } + if _, err := db.Exec("DROP TRIGGER IF EXISTS trg_actions_count_update"); err != nil { + return false, fmt.Errorf("drop stale trigger: %w", err) + } + return true, nil +} + +const recountTaskActionCountsSQL = "INSERT INTO task_action_counts (task_id, status, count) SELECT task_id, status, COUNT(*) FROM actions GROUP BY task_id, status" + +func (db *DB) rebuildTaskActionCounts() error { + ctx := context.Background() + return db.withTxRetry(ctx, "rebuildTaskActionCounts", func(tx *sql.Tx) error { + if _, err := tx.ExecContext(ctx, "DELETE FROM task_action_counts"); err != nil { + return fmt.Errorf("clear: %w", err) + } + if _, err := tx.ExecContext(ctx, recountTaskActionCountsSQL); err != nil { + return fmt.Errorf("recount: %w", err) + } + return nil + }) +} + // backfillTaskActionCounts populates task_action_counts from existing actions // rows on the first migration. Idempotent: if the table already has rows, // triggers have been keeping it in sync, so skip. func (db *DB) backfillTaskActionCounts() error { - var hasRows int - if err := db.QueryRow("SELECT EXISTS(SELECT 1 FROM task_action_counts LIMIT 1)").Scan(&hasRows); err != nil { - return fmt.Errorf("check rows: %w", err) - } - if hasRows != 0 { + ctx := context.Background() + return db.withTxRetry(ctx, "backfillTaskActionCounts", func(tx *sql.Tx) error { + var hasRows int + if err := tx.QueryRowContext(ctx, "SELECT EXISTS(SELECT 1 FROM task_action_counts LIMIT 1)").Scan(&hasRows); err != nil { + return fmt.Errorf("check rows: %w", err) + } + if hasRows != 0 { + return nil + } + if _, err := tx.ExecContext(ctx, recountTaskActionCountsSQL); err != nil { + return fmt.Errorf("insert: %w", err) + } return nil - } - if _, err := db.Exec("INSERT INTO task_action_counts (task_id, status, count) SELECT task_id, status, COUNT(*) FROM actions GROUP BY task_id, status"); err != nil { - return fmt.Errorf("insert: %w", err) - } - return nil + }) } func (db *DB) Close() error { diff --git a/db/schema.sql b/db/schema.sql index 61b174a..6a8369c 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -81,8 +81,8 @@ BEGIN END; CREATE TRIGGER IF NOT EXISTS trg_actions_count_update -AFTER UPDATE OF status ON actions -WHEN OLD.status != NEW.status +AFTER UPDATE OF task_id, status ON actions +WHEN OLD.task_id != NEW.task_id OR OLD.status != NEW.status BEGIN UPDATE task_action_counts SET count = count - 1 WHERE task_id = OLD.task_id AND status = OLD.status; diff --git a/db/task_action_counts_test.go b/db/task_action_counts_test.go index 81cd068..488ff3e 100644 --- a/db/task_action_counts_test.go +++ b/db/task_action_counts_test.go @@ -2,6 +2,7 @@ package db_test import ( "sort" + "strings" "testing" "github.com/MH4GF/tq/db" @@ -361,3 +362,162 @@ func TestTaskActionCounts_BackfillFromExisting(t *testing.T) { t.Errorf("backfill mismatch:\ngot=%v\nwant=%v", got, want) } } + +func TestTaskActionCounts_TaskIDChange(t *testing.T) { + d := testutil.NewTestDB(t) + testutil.SeedTestProjects(t, d) + task1, err := d.InsertTask(1, "task1", "{}", "") + if err != nil { + t.Fatalf("InsertTask task1: %v", err) + } + task2, err := d.InsertTask(1, "task2", "{}", "") + if err != nil { + t.Fatalf("InsertTask task2: %v", err) + } + a1, err := d.InsertAction("a1", task1, "{}", db.ActionStatusPending, nil, "") + if err != nil { + t.Fatalf("InsertAction: %v", err) + } + + steps := []struct { + name string + mutate func() error + want []countRow + }{ + { + name: "initial: pending on task1", + mutate: func() error { return nil }, + want: []countRow{{task1, db.ActionStatusPending, 1}}, + }, + { + name: "task_id only: task1 → task2", + mutate: func() error { + _, err := d.Exec("UPDATE actions SET task_id = ? WHERE id = ?", task2, a1) + return err + }, + want: []countRow{{task2, db.ActionStatusPending, 1}}, + }, + { + name: "task_id and status together: task2/pending → task1/running", + mutate: func() error { + _, err := d.Exec("UPDATE actions SET task_id = ?, status = ? WHERE id = ?", task1, db.ActionStatusRunning, a1) + return err + }, + want: []countRow{{task1, db.ActionStatusRunning, 1}}, + }, + } + + for _, st := range steps { + t.Run(st.name, func(t *testing.T) { + if err := st.mutate(); err != nil { + t.Fatalf("mutate: %v", err) + } + got := dumpCounts(t, d) + if !equalCountRows(got, st.want) { + t.Errorf("counts: got %v, want %v", got, st.want) + } + if want := dumpGroupBy(t, d); !equalCountRows(got, want) { + t.Errorf("counts diverge from GROUP BY actions:\ngroup_by=%v\ncounts=%v", want, got) + } + }) + } +} + +func TestTaskActionCounts_TaskReassignReleasesCloseGuard(t *testing.T) { + d := testutil.NewTestDB(t) + testutil.SeedTestProjects(t, d) + task1, err := d.InsertTask(1, "task1", "{}", "") + if err != nil { + t.Fatalf("InsertTask task1: %v", err) + } + task2, err := d.InsertTask(1, "task2", "{}", "") + if err != nil { + t.Fatalf("InsertTask task2: %v", err) + } + a1, err := d.InsertAction("a1", task1, "{}", db.ActionStatusPending, nil, "") + if err != nil { + t.Fatalf("InsertAction: %v", err) + } + if _, err := d.Exec("UPDATE actions SET task_id = ? WHERE id = ?", task2, a1); err != nil { + t.Fatalf("reassign: %v", err) + } + + if err := d.UpdateTask(task1, db.TaskStatusDone, "all actions reassigned"); err != nil { + t.Errorf("close task1 after reassign: %v", err) + } + err = d.UpdateTask(task2, db.TaskStatusDone, "should be blocked") + if err == nil || !strings.Contains(err.Error(), "pending/running/dispatched") { + t.Errorf("close task2 with active action: got %v, want close-guard error", err) + } +} + +const preTaskIDTriggerDDL = `CREATE TRIGGER trg_actions_count_update +AFTER UPDATE OF status ON actions +WHEN OLD.status != NEW.status +BEGIN + UPDATE task_action_counts SET count = count - 1 + WHERE task_id = OLD.task_id AND status = OLD.status; + INSERT INTO task_action_counts (task_id, status, count) + VALUES (NEW.task_id, NEW.status, 1) + ON CONFLICT(task_id, status) DO UPDATE SET count = count + 1; +END` + +func TestTaskActionCounts_MigrateRecreatesStaleTrigger(t *testing.T) { + d := testutil.NewTestDB(t) + testutil.SeedTestProjects(t, d) + task1, err := d.InsertTask(1, "task1", "{}", "") + if err != nil { + t.Fatalf("InsertTask task1: %v", err) + } + task2, err := d.InsertTask(1, "task2", "{}", "") + if err != nil { + t.Fatalf("InsertTask task2: %v", err) + } + if _, err := d.Exec("DROP TRIGGER trg_actions_count_update"); err != nil { + t.Fatalf("drop trigger: %v", err) + } + if _, err := d.Exec(preTaskIDTriggerDDL); err != nil { + t.Fatalf("create stale trigger: %v", err) + } + + a1, err := d.InsertAction("a1", task1, "{}", db.ActionStatusPending, nil, "") + if err != nil { + t.Fatalf("InsertAction: %v", err) + } + if _, err := d.Exec("UPDATE actions SET task_id = ? WHERE id = ?", task2, a1); err != nil { + t.Fatalf("reassign: %v", err) + } + + ghost := []countRow{{task1, db.ActionStatusPending, 1}} + if got := dumpCounts(t, d); !equalCountRows(got, ghost) { + t.Fatalf("stale trigger should leave ghost count: got %v, want %v", got, ghost) + } + err = d.UpdateTask(task1, db.TaskStatusDone, "blocked by ghost count") + if err == nil || !strings.Contains(err.Error(), "pending/running/dispatched") { + t.Fatalf("close task1 before Migrate: got %v, want close-guard error", err) + } + + if err := d.Migrate(); err != nil { + t.Fatalf("Migrate: %v", err) + } + + var triggerSQL string + if err := d.QueryRow("SELECT sql FROM sqlite_master WHERE type = 'trigger' AND name = 'trg_actions_count_update'").Scan(&triggerSQL); err != nil { + t.Fatalf("read trigger: %v", err) + } + if !strings.Contains(triggerSQL, "OLD.task_id != NEW.task_id") { + t.Errorf("trigger not recreated with task_id coverage: %s", triggerSQL) + } + + got := dumpCounts(t, d) + want := []countRow{{task2, db.ActionStatusPending, 1}} + if !equalCountRows(got, want) { + t.Errorf("counts after rebuild: got %v, want %v", got, want) + } + if groupBy := dumpGroupBy(t, d); !equalCountRows(got, groupBy) { + t.Errorf("counts diverge from GROUP BY actions:\ngroup_by=%v\ncounts=%v", groupBy, got) + } + if err := d.UpdateTask(task1, db.TaskStatusDone, "ghost count repaired"); err != nil { + t.Errorf("close task1 after Migrate: %v", err) + } +} diff --git a/docs/golden-rules.md b/docs/golden-rules.md index 7c9f337..1d981ce 100644 --- a/docs/golden-rules.md +++ b/docs/golden-rules.md @@ -162,8 +162,8 @@ Current status totals are captured after each rule as `current violations: N`. A **Rule 18 [enforced] — `tui/` and `dispatch/` MUST NOT contain `SELECT COUNT/SUM/AVG` string literals. Aggregate-driven counts on `actions` go through `db.Store.GetTaskActionCount`, which reads from the trigger-maintained `task_action_counts` table.** -- Why: `turso db inspect tq --queries` repeatedly surfaces `SELECT COUNT(*) FROM actions WHERE task_id = ? AND status IN (...)` in the top-N rows-read consumers. Turso bills per row read, so per-tick aggregate scans dominate quota. The `task_action_counts(task_id, status, count)` table is maintained by `AFTER INSERT / AFTER UPDATE OF status / AFTER DELETE` triggers on `actions`, so any task's status counts are a 1-row index lookup. This rule structurally bans hot-path code from re-introducing aggregate scans — it is a specialization of Rule 11 narrowed to the most expensive aggregates. -- Assumption: `actions.task_id` is immutable. Triggers do not handle `task_id` changes; no production path issues `UPDATE actions SET task_id = ?`. If that changes, add a trigger or migrate the data manually. +- Why: `turso db inspect tq --queries` repeatedly surfaces `SELECT COUNT(*) FROM actions WHERE task_id = ? AND status IN (...)` in the top-N rows-read consumers. Turso bills per row read, so per-tick aggregate scans dominate quota. The `task_action_counts(task_id, status, count)` table is maintained by `AFTER INSERT / AFTER UPDATE OF task_id, status / AFTER DELETE` triggers on `actions`, so any task's status counts are a 1-row index lookup. This rule structurally bans hot-path code from re-introducing aggregate scans — it is a specialization of Rule 11 narrowed to the most expensive aggregates. +- Task reassignment: `tq action update --task` issues `UPDATE actions SET task_id = ?`, so `trg_actions_count_update` fires on `task_id` changes as well as `status` changes, moving the count between tasks. DBs whose trigger predates `task_id` coverage are healed on `db.Migrate()`: the stale trigger is dropped, recreated from the schema, and `task_action_counts` is rebuilt from `actions` (`dropStaleCountUpdateTrigger` / `rebuildTaskActionCounts` in `db/db.go`). - Backfill: `db.Migrate()` runs an idempotent `INSERT INTO task_action_counts SELECT task_id, status, COUNT(*) FROM actions GROUP BY task_id, status` only when `task_action_counts` is empty (`backfillTaskActionCounts` in `db/db.go`). Subsequent runs see existing rows kept in sync by the triggers and skip the backfill. - Verify: Go test harness `internal/goldenrules/` scans `tui/`, `dispatch/` for `"...SELECT COUNT|SUM|AVG..."` string literals. Ceiling-based: violations below the ceiling pass, regressions fail. Run `go test ./internal/goldenrules/`. - Detection limits (same as Rule 11/16): line-split (`"SELECT " + "COUNT(*)"`) and cross-literal concatenation (`prefix + " COUNT(*)"`) bypass detection. Reviewers MUST reject rewrites that exploit these to hide hot-path aggregates.