fix(tasks): persist task_history row at task START to survive restart kills #1112

Merged

reviewer merged 2 commits from dev/1107 into main

2026-05-11 20:32:10 +00:00

dev commented

2026-05-11 20:05:37 +00:00

Collaborator

Closes #1107.

Summary

Start-time INSERT: startTask() inserts a status='running' row in task_history the moment a worker transitions to running (in onStart). A SIGKILL between dispatch and completion now leaves an audit row instead of a blank.
Finish-time UPDATE: persistTask() (called from onFinish) remains an upsert — it overwrites the running row with the terminal status, finished_at, token counts, and cost. No duplicate inserts.
Boot-time recovery: scanInterruptedAtBoot now calls markRunningAsInterrupted() first — every orphaned running row from a prior killed process is transitioned to interrupted with finished_at = now, making it visible to listUnresolvedInterruptedTasks and the existing session-invalidity sweep (sweepInvalidSessions already rejects any running row as non-success, so no ordering dependency).

Schema

Migration 0003_nullable_finished_at_tasks.sql rebuilds task_history (SQLite CREATE/INSERT/DROP/RENAME pattern) to make finished_at nullable for in-flight rows, and adds a status index for the boot-scan query.

Test plan

8 new unit tests in task-store.test.ts cover: start-time row content, idempotency, finish-time update (no double-insert), empty markRunningAsInterrupted, two-row transition, session-scan visibility, completed rows not re-transitioned.
All 3319 server tests pass (bun x turbo run test --filter=@claude-hooks/server).

Closes #1107. ## Summary - **Start-time INSERT**: `startTask()` inserts a `status='running'` row in `task_history` the moment a worker transitions to `running` (in `onStart`). A SIGKILL between dispatch and completion now leaves an audit row instead of a blank. - **Finish-time UPDATE**: `persistTask()` (called from `onFinish`) remains an upsert — it overwrites the `running` row with the terminal status, `finished_at`, token counts, and cost. No duplicate inserts. - **Boot-time recovery**: `scanInterruptedAtBoot` now calls `markRunningAsInterrupted()` first — every orphaned `running` row from a prior killed process is transitioned to `interrupted` with `finished_at = now`, making it visible to `listUnresolvedInterruptedTasks` and the existing session-invalidity sweep (`sweepInvalidSessions` already rejects any `running` row as non-success, so no ordering dependency). ## Schema Migration `0003_nullable_finished_at_tasks.sql` rebuilds `task_history` (SQLite `CREATE/INSERT/DROP/RENAME` pattern) to make `finished_at` nullable for in-flight rows, and adds a `status` index for the boot-scan query. ## Test plan - [ ] 8 new unit tests in `task-store.test.ts` cover: start-time row content, idempotency, finish-time update (no double-insert), empty `markRunningAsInterrupted`, two-row transition, session-scan visibility, completed rows not re-transitioned. - [ ] All 3319 server tests pass (`bun x turbo run test --filter=@claude-hooks/server`).

dev added 2 commits

2026-05-11 20:05:37 +00:00

fix(tasks): persist task_history row at task START to survive restart kills 8164f49c15

Closes #1107.

Adds a start-time INSERT so that a SIGKILL between dispatch and
completion leaves a `status='running'` audit row in `task_history`.
On next boot, `scanInterruptedAtBoot` transitions orphaned running
rows to `interrupted`, making them visible to the existing recovery
hint log and session-invalidity sweep.

Schema migration (0003): rebuilds `task_history` with nullable
`finished_at` (required for in-flight rows) and adds a `status`
index for the boot-time running-row scan.

Key changes:
- `startTask(input)` — inserts `status='running'` row at dispatch
  time; `onConflictDoNothing` so double-calls are safe.
- `markRunningAsInterrupted()` — transitions all `running` rows to
  `interrupted` at boot; returns affected rows for the log.
- `onStart` in `registry.ts` — calls `startTask` so every in-flight
  task has a DB record from the moment it begins.
- `scanInterruptedAtBoot` — calls `markRunningAsInterrupted` first,
  then the existing `listUnresolvedInterruptedTasks` scan.
- `listTasksForIssue` — now filters by terminal statuses to exclude
  in-flight `running` rows from the pipeline view.
- 8 unit tests covering start-time insert, idempotency, finish-time
  update, and boot-time interrupted transition.

Co-authored-by: Cursor <cursoragent@cursor.com>

fix(tasks): fix TS errors — add started_at to LatestTaskStatusRow, widen ZeroOutputSuccessRow.finished_at

qa / dockerfile (pull_request) Successful in 1m1s

Details

qa / i18n-string-check (pull_request) Successful in 1m56s

Details

qa / sql-layer-check (pull_request) Successful in 2m11s

Details

qa / db-schema (pull_request) Successful in 2m25s

Details

qa / qa-1 (pull_request) Successful in 21m53s

Details

qa / qa (pull_request) Successful in 0s

Details

482f9e94bb

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer

2026-05-11 20:29:39 +00:00

reviewer approved these changes

2026-05-11 20:32:00 +00:00

reviewer left a comment

Review — fix(tasks): persist task_history row at task START (#1107)

CI ✅ green · mergeable ✅

Overview

Adds a two-phase task_history lifecycle: a status='running' row is inserted the moment a worker starts (startTask / onStart), and the existing persistTask upsert overwrites it at completion. A SIGKILL between those two points now leaves an auditable row rather than a blank. Boot-time recovery is fixed by calling markRunningAsInterrupted() before the HTTP server starts, so orphaned rows flow straight into listUnresolvedInterruptedTasks and the session-invalidity sweep without any ordering dependency.

What works well

Idempotency via onConflictDoNothing in startTask is the right primitive — double-calls are silently safe.
Boot ordering is correct: markRunningAsInterrupted() → listUnresolvedInterruptedTasks() → HTTP server, all before any new startTask calls can fire. The TOCTOU window in the read-then-update pattern inside markRunningAsInterrupted is therefore unreachable in production.
listTasksForIssue filter (inArray(status, TERMINAL_STATUSES)) cleanly prevents in-flight rows from polluting the pipeline view. Good defensive move.
Migration uses the correct SQLite create/copy/drop/rename dance. The DROP TABLE IF EXISTS task_history_new guard at the top is good defensive practice against a half-applied previous run.
8 new unit tests cover the happy path, idempotency, finish-time update (no duplicate insert), empty/two-row markRunningAsInterrupted, session-scan visibility, and completed-row non-regression. Coverage is solid for the new code paths.
Comment update on hasInFlightTask accurately reflects the new DB-level coverage (in-flight tasks are now visible via startTask) while correctly noting that queued-but-not-yet-started tasks still require the in-memory check.

Minor observations (non-blocking)

finished_at: r.finished_at as number cast in listTasksForIssue — the cast is safe because the inArray(status, TERMINAL_STATUSES) filter guarantees every returned row has a non-null finished_at, but the assertion silently suppresses the nullability at the type level. A short inline comment — // safe: terminal rows always have finished_at set — would make the intent self-documenting for the next reader.
finished_at ?? 0 fallback in listUnresolvedInterruptedTasks — using the Unix epoch (1970-01-01) as a sentinel for a timestamp field is a code smell. It's practically unreachable (all interrupted rows have finished_at set, either historically under the old NOT NULL constraint or via markRunningAsInterrupted), but if it ever fires it silently sorts affected rows to the front of the recovery list. r.started_at or Date.now() would be less surprising as a fallback.
ZeroOutputSuccessRow.finished_at widened to number | null — the backing query (listZeroOutputSuccesses) filters exclusively on status='success' rows, which always carry a non-null finished_at. The type widening is over-broad here; keeping it number would be more precise and would save callers from having to null-guard a field that can never actually be null on that code path.
No test for listTasksForIssue excluding running rows — the eight new tests are good, but the filtering change in listTasksForIssue (which is a behaviour change, not just a type fix) has no dedicated coverage. A one-liner that inserts a running row and asserts it doesn't appear in listTasksForIssue results would close the gap.

None of these are blockers — the core logic is correct and the tests pass. Approving.

## Review — fix(tasks): persist task_history row at task START (#1107) CI ✅ green · mergeable ✅ ### Overview Adds a two-phase `task_history` lifecycle: a `status='running'` row is inserted the moment a worker starts (`startTask` / `onStart`), and the existing `persistTask` upsert overwrites it at completion. A SIGKILL between those two points now leaves an auditable row rather than a blank. Boot-time recovery is fixed by calling `markRunningAsInterrupted()` before the HTTP server starts, so orphaned rows flow straight into `listUnresolvedInterruptedTasks` and the session-invalidity sweep without any ordering dependency. ### What works well - **Idempotency via `onConflictDoNothing`** in `startTask` is the right primitive — double-calls are silently safe. - **Boot ordering** is correct: `markRunningAsInterrupted()` → `listUnresolvedInterruptedTasks()` → HTTP server, all before any new `startTask` calls can fire. The TOCTOU window in the read-then-update pattern inside `markRunningAsInterrupted` is therefore unreachable in production. - **`listTasksForIssue` filter** (`inArray(status, TERMINAL_STATUSES)`) cleanly prevents in-flight rows from polluting the pipeline view. Good defensive move. - **Migration** uses the correct SQLite create/copy/drop/rename dance. The `DROP TABLE IF EXISTS task_history_new` guard at the top is good defensive practice against a half-applied previous run. - **8 new unit tests** cover the happy path, idempotency, finish-time update (no duplicate insert), empty/two-row `markRunningAsInterrupted`, session-scan visibility, and completed-row non-regression. Coverage is solid for the new code paths. - **Comment update on `hasInFlightTask`** accurately reflects the new DB-level coverage (in-flight tasks are now visible via `startTask`) while correctly noting that queued-but-not-yet-started tasks still require the in-memory check. ### Minor observations (non-blocking) 1. **`finished_at: r.finished_at as number` cast in `listTasksForIssue`** — the cast is safe because the `inArray(status, TERMINAL_STATUSES)` filter guarantees every returned row has a non-null `finished_at`, but the assertion silently suppresses the nullability at the type level. A short inline comment — `// safe: terminal rows always have finished_at set` — would make the intent self-documenting for the next reader. 2. **`finished_at ?? 0` fallback in `listUnresolvedInterruptedTasks`** — using the Unix epoch (1970-01-01) as a sentinel for a timestamp field is a code smell. It's practically unreachable (all `interrupted` rows have `finished_at` set, either historically under the old NOT NULL constraint or via `markRunningAsInterrupted`), but if it ever fires it silently sorts affected rows to the front of the recovery list. `r.started_at` or `Date.now()` would be less surprising as a fallback. 3. **`ZeroOutputSuccessRow.finished_at` widened to `number | null`** — the backing query (`listZeroOutputSuccesses`) filters exclusively on `status='success'` rows, which always carry a non-null `finished_at`. The type widening is over-broad here; keeping it `number` would be more precise and would save callers from having to null-guard a field that can never actually be null on that code path. 4. **No test for `listTasksForIssue` excluding running rows** — the eight new tests are good, but the filtering change in `listTasksForIssue` (which is a behaviour change, not just a type fix) has no dedicated coverage. A one-liner that inserts a running row and asserts it doesn't appear in `listTasksForIssue` results would close the gap. None of these are blockers — the core logic is correct and the tests pass. Approving.

reviewer merged commit 057420f3db into main

2026-05-11 20:32:10 +00:00