fix(db): make task_action_counts trigger follow action task_id changes - #416
Conversation
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 52 minutes and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends task action count tracking to account for task_id reassignment. The trigger now fires on both ChangesTask ID Reassignment Trigger and Migration Recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db/db.go (1)
442-451:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the backfill probe and recount atomic.
Lines 443-450 split the emptiness check and the
INSERT ... SELECTinto separate autocommit statements. A concurrent action insert can repopulatetask_action_countsbetween those calls, and the plain recount insert then collides with the(task_id, status)PK and abortsMigrate(). Wrap the probe + recount inwithTxRetry(or make the recount an upsert) so live migrations cannot fail on concurrent writes.Suggested fix
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 { - return nil - } - if _, err := db.Exec(recountTaskActionCountsSQL); err != nil { - return fmt.Errorf("insert: %w", err) - } - return nil + 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 + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/db.go` around lines 442 - 451, The probe-and-recount in backfillTaskActionCounts is not atomic and can race with concurrent inserts; wrap the emptiness check and the INSERT (recountTaskActionCountsSQL) inside a transactional retry using withTxRetry so both the SELECT EXISTS and Exec run in the same transaction (use the tx passed into the closure and call tx.QueryRow/tx.Exec) or alternatively convert recountTaskActionCountsSQL into an upsert; modify backfillTaskActionCounts to call withTxRetry and perform the probe + insert inside that closure, referencing backfillTaskActionCounts, withTxRetry, and recountTaskActionCountsSQL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@db/db.go`:
- Around line 442-451: The probe-and-recount in backfillTaskActionCounts is not
atomic and can race with concurrent inserts; wrap the emptiness check and the
INSERT (recountTaskActionCountsSQL) inside a transactional retry using
withTxRetry so both the SELECT EXISTS and Exec run in the same transaction (use
the tx passed into the closure and call tx.QueryRow/tx.Exec) or alternatively
convert recountTaskActionCountsSQL into an upsert; modify
backfillTaskActionCounts to call withTxRetry and perform the probe + insert
inside that closure, referencing backfillTaskActionCounts, withTxRetry, and
recountTaskActionCountsSQL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d91aba46-4f79-46e8-8129-a96b5cee7ce3
📒 Files selected for processing (5)
.goldenrules-rule17-allowlistdb/db.godb/schema.sqldb/task_action_counts_test.godocs/golden-rules.md
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 <noreply@anthropic.com>
|
@coderabbitai The backfill probe + recount race is fixed in 8afde13 — both statements now run inside withTxRetry, matching rebuildTaskActionCounts. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
task_action_counts維持トリガーtrg_actions_count_updateが action のtask_id付け替えに追従していなかった問題を修正する。task #914 / action #6262。バグの実害
2026-06-10、pending action の task 796 → 797 移動の痕跡が Turso 本番 DB に残留した。
残留したのは task 796 の幽霊カウント (pending +1) と task 797 の負カウント (pending -1) の 2 行。
これにより task #796 の close が「1 pending/running/dispatched action(s)」エラーで阻害された (実 actions は 0 件)。
原因は旧トリガーの定義
AFTER UPDATE OF status ... WHEN OLD.status != NEW.statusにある。この定義は
tq action update --taskの発行するUPDATE actions SET task_id = ?では発火しない。Changes
db/schema.sql— トリガーをAFTER UPDATE OF task_id, status+WHEN OLD.task_id != NEW.task_id OR OLD.status != NEW.statusへ拡張。body は元から (OLD.task_id, OLD.status) decrement → (NEW.task_id, NEW.status) upsert increment なので変更不要db/db.go— schema.sql はCREATE TRIGGER IF NOT EXISTSのため、既存 DB に新定義が入らない。Migrate()は WHEN 句マーカーOLD.task_id != NEW.task_idを欠く旧定義を sqlite_master から検出してDROP TRIGGER IF EXISTSする。直後の schema 実行が新定義を再作成する。旧定義の居た DB は過去ドリフトの可能性があるため、task_action_countsを actions から 1 トランザクションで全再構築する (rebuildTaskActionCounts)docs/golden-rules.md— Rule 18 の「task_id is immutable」前提を削除し、トリガー列挙とマイグレーション修復経路を現状へ更新.goldenrules-rule17-allowlist— db.go の行ずれ更新 + マイグレーション時スキャン 3 件 (sqlite_master 参照、一度きりの rebuild) を追加db/task_action_counts_test.go)設計判断
OLD.task_id単体だと旧定義 body (WHERE task_id = OLD.task_id) にも含まれ誤判定するため、WHEN 句固有の比較式にしている。schema.sql の将来の再フォーマットでマーカーが壊れた場合はTestTaskActionCounts_MigrateRecreatesStaleTriggerの DDL アサーションが CI で落ちて検知されるVerification
ユニットテスト + lint:
E2E (fresh build バイナリ + 一時 DB で本番事故シナリオを再現):
マイグレーション経路の E2E も実施した。
旧トリガーを sqlite3 で仕込んだ DB に
UPDATE actions SET task_id = 2を発行し、ドリフトを再現した。その後、新バイナリで
task listを 1 回実行した。結果、sqlite_master のトリガーは新定義となった。
counts は
2|pending|1のみへ再構築され、task 1 の close も成功した。デプロイ後の確認
本番 Turso DB は旧トリガーを持つため、新バイナリの初回 Migrate で再作成 + rebuild が一度走る。適用後に次で確認する:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests