From f189bdd096d1602089dbd0defcc270ee51a12a64 Mon Sep 17 00:00:00 2001 From: MH4GF Date: Fri, 12 Jun 2026 10:19:04 +0900 Subject: [PATCH 1/3] fix(db): make task_action_counts trigger follow action task_id changes trg_actions_count_update fired only on status changes, so reassigning an action to another task (`tq action update --task` issues `UPDATE actions SET task_id = ?`) left a ghost count on the old task and no count on the new one. The stale ghost row then blocked closing the old task via the UpdateTaskFields close guard, which reads GetTaskActionCount, even though the task had zero live actions. - Extend the trigger to `AFTER UPDATE OF task_id, status` with `WHEN OLD.task_id != NEW.task_id OR OLD.status != NEW.status`; the body already decrements (OLD.task_id, OLD.status) and upserts (NEW.task_id, NEW.status), so it handles task moves as-is. - schema.sql uses CREATE TRIGGER IF NOT EXISTS and cannot replace the old definition on existing DBs. Migrate() now drops the stale trigger before the schema exec recreates it. Staleness is detected by the WHEN-clause marker `OLD.task_id != NEW.task_id` in sqlite_master; the bare substring `OLD.task_id` would misdetect because the old trigger body also contains it in its UPDATE ... WHERE clause. - A DB that carried the old trigger may hold drift it leaked, so the same migration rebuilds task_action_counts from actions in one transaction. The known production incident (ghost pending count after moving an action between tasks, blocking task close) heals on first run of the new binary. The crash window between DROP and rebuild is accepted: it is milliseconds long and at most once per DB. - Update golden-rules Rule 18 notes: drop the "task_id is immutable" assumption (a production path does reassign it) and document the migration/rebuild path. Refresh rule-17 allowlist line numbers and add the three new migration-time scans (sqlite_master lookup and the one-shot rebuild). Co-Authored-By: Claude Fable 5 --- .goldenrules-rule17-allowlist | 19 ++-- db/db.go | 43 +++++++++ db/schema.sql | 4 +- db/task_action_counts_test.go | 160 ++++++++++++++++++++++++++++++++++ docs/golden-rules.md | 4 +- 5 files changed, 219 insertions(+), 11 deletions(-) diff --git a/.goldenrules-rule17-allowlist b/.goldenrules-rule17-allowlist index 6ea1fee3..b39000b1 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:427-430 — rebuildTaskActionCounts: one-shot full rebuild on trigger migration +db/db.go:427 SCAN task_action_counts +db/db.go:430 SCAN actions +db/db.go:442 SCAN CONSTANT +db/db.go:442 SCAN task_action_counts +db/db.go:448 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 f778812b..28c93af6 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,6 +403,37 @@ 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 trg_actions_count_update"); err != nil { + return false, fmt.Errorf("drop stale trigger: %w", err) + } + return true, nil +} + +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, "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("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. diff --git a/db/schema.sql b/db/schema.sql index 61b174af..6a8369c4 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 81cd068f..488ff3e9 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 7c9f3376..1d981ce3 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. From 049739f391158b44a19efae17e1d1b3715eba1c8 Mon Sep 17 00:00:00 2001 From: MH4GF Date: Fri, 12 Jun 2026 10:46:15 +0900 Subject: [PATCH 2/3] fix(db): make stale-trigger drop race-safe and dedupe recount SQL - Use DROP TRIGGER IF EXISTS in dropStaleCountUpdateTrigger. Two tq processes migrating the same DB can both read the stale definition from sqlite_master; without IF EXISTS the slower one fails Migrate with "no such trigger". With IF EXISTS every interleaving converges on the new trigger plus a rebuilt counts table. - Extract the INSERT ... SELECT ... GROUP BY recount statement shared by rebuildTaskActionCounts and backfillTaskActionCounts into recountTaskActionCountsSQL so the two paths cannot drift. - Refresh rule-17 allowlist line numbers for the shifted db.go lines. Co-Authored-By: Claude Fable 5 --- .goldenrules-rule17-allowlist | 12 ++++++------ db/db.go | 8 +++++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/.goldenrules-rule17-allowlist b/.goldenrules-rule17-allowlist index b39000b1..c8d16d1d 100644 --- a/.goldenrules-rule17-allowlist +++ b/.goldenrules-rule17-allowlist @@ -25,12 +25,12 @@ 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:427-430 — rebuildTaskActionCounts: one-shot full rebuild on trigger migration -db/db.go:427 SCAN task_action_counts -db/db.go:430 SCAN actions -db/db.go:442 SCAN CONSTANT -db/db.go:442 SCAN task_action_counts -db/db.go:448 SCAN actions +# 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:444 SCAN CONSTANT +db/db.go:444 SCAN task_action_counts +db/db.go:450 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 28c93af6..102eb0df 100644 --- a/db/db.go +++ b/db/db.go @@ -415,19 +415,21 @@ func (db *DB) dropStaleCountUpdateTrigger() (bool, error) { if strings.Contains(sqlText, "OLD.task_id != NEW.task_id") { return false, nil } - if _, err := db.Exec("DROP TRIGGER trg_actions_count_update"); err != 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, "INSERT INTO task_action_counts (task_id, status, count) SELECT task_id, status, COUNT(*) FROM actions GROUP BY task_id, status"); err != nil { + if _, err := tx.ExecContext(ctx, recountTaskActionCountsSQL); err != nil { return fmt.Errorf("recount: %w", err) } return nil @@ -445,7 +447,7 @@ func (db *DB) backfillTaskActionCounts() error { if hasRows != 0 { 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 { + if _, err := db.Exec(recountTaskActionCountsSQL); err != nil { return fmt.Errorf("insert: %w", err) } return nil From 8afde133dbd9c92f7789a06f942254901216d952 Mon Sep 17 00:00:00 2001 From: MH4GF Date: Fri, 12 Jun 2026 13:25:47 +0900 Subject: [PATCH 3/3] fix(db): run backfill probe and recount in one transaction backfillTaskActionCounts checked emptiness and ran the recount INSERT as separate autocommit statements. A concurrent action insert between them repopulates task_action_counts via the insert trigger, and the recount then collides with the (task_id, status) primary key, aborting Migrate. Wrap both statements in withTxRetry so the probe and recount are atomic, matching rebuildTaskActionCounts. Refresh rule-17 allowlist line numbers for the shifted db.go lines. Co-Authored-By: Claude Fable 5 --- .goldenrules-rule17-allowlist | 6 +++--- db/db.go | 23 +++++++++++++---------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/.goldenrules-rule17-allowlist b/.goldenrules-rule17-allowlist index c8d16d1d..f950b7e4 100644 --- a/.goldenrules-rule17-allowlist +++ b/.goldenrules-rule17-allowlist @@ -28,9 +28,9 @@ 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:444 SCAN CONSTANT -db/db.go:444 SCAN task_action_counts -db/db.go:450 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 102eb0df..56740452 100644 --- a/db/db.go +++ b/db/db.go @@ -440,17 +440,20 @@ func (db *DB) rebuildTaskActionCounts() error { // 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(recountTaskActionCountsSQL); err != nil { - return fmt.Errorf("insert: %w", err) - } - return nil + }) } func (db *DB) Close() error {