diff --git a/eslint.config.mjs b/eslint.config.mjs index 999f5486..df4fbccb 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -126,18 +126,34 @@ export default tseslint.config( "no-restricted-syntax": [ "error", { - // Plain string literals and normal template heads. + // Plain string literals: any `UPDATE tasks SET ...status = ...` + // regardless of column ordering. The previous variant only + // caught `SET status` as the first column, letting writes + // like `SET checkout_run_id = ..., status = 'todo'` slip + // through silently. selector: - "Literal[value=/\\b[Uu][Pp][Dd][Aa][Tt][Ee]\\s+tasks\\s+[Ss][Ee][Tt]\\s+status\\b/]", + "Literal[value=/\\b[Uu][Pp][Dd][Aa][Tt][Ee]\\s+tasks\\s+[Ss][Ee][Tt]\\s[^;]*\\bstatus\\s*=/]", message: "B2 Phase 1: write task.status through advanceTaskPhase (or tag with `// eslint-disable-next-line no-restricted-syntax` + dual-write rationale for grandfathered sites).", }, { selector: - "TemplateElement[value.raw=/\\b[Uu][Pp][Dd][Aa][Tt][Ee]\\s+tasks\\s+[Ss][Ee][Tt]\\s+status\\b/]", + "TemplateElement[value.raw=/\\b[Uu][Pp][Dd][Aa][Tt][Ee]\\s+tasks\\s+[Ss][Ee][Tt]\\s[^;]*\\bstatus\\s*=/]", message: "B2 Phase 1: write task.status through advanceTaskPhase (or tag with `// eslint-disable-next-line no-restricted-syntax` + dual-write rationale for grandfathered sites).", }, + { + // Dynamic-SQL bypass: `UPDATE tasks SET ${...}` where the + // first interpolation is a JS-built column list. The literal + // head ends with `SET ` (trailing space) and `status` never + // shows up in the template itself. Match the head-only shape + // so this helper must carry an explicit eslint-disable with + // a dual-write rationale, same as the grandfathered sites. + selector: + "TemplateElement[value.raw=/\\b[Uu][Pp][Dd][Aa][Tt][Ee]\\s+tasks\\s+[Ss][Ee][Tt]\\s*$/]", + message: + "B2 Phase 1: dynamic `UPDATE tasks SET ${...}` helpers bypass the literal-SET guard; route through advanceTaskPhase or tag with `// eslint-disable-next-line no-restricted-syntax` + dual-write rationale.", + }, ], }, } diff --git a/src/main/database-base.ts b/src/main/database-base.ts index 353f4787..93196654 100644 --- a/src/main/database-base.ts +++ b/src/main/database-base.ts @@ -246,26 +246,52 @@ export class AppDatabaseBase { const ts = nowIso(); let interruptedCount = 0; this.transaction(() => { - const result = this.db - .prepare("update runs set status = 'interrupted', error_message = 'AgentCompany restarted before the run could report completion.', finished_at = ?, updated_at = ? where status = 'running'") - .run(ts, ts); - interruptedCount = Number(result.changes); - if (interruptedCount > 0) { - this.db.prepare( - // eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: crash-recovery sweep; trigger mirrors status→phase, step 3/5 already drops shadow leases below - `update tasks set status = 'blocked', updated_at = ? where id in (select task_id from runs where status = 'interrupted' and finished_at = ?) and status in ('in_progress', 'todo')`, - ).run(ts, ts); + // Capture the run ids this sweep intends to interrupt BEFORE + // mutating anything. The old code scoped the downstream task + + // lease cleanup with `WHERE finished_at = ?` against the freshly + // stamped timestamp — safe in isolation, but ISO-millisecond + // ties with an earlier interrupted run are possible in principle + // and would clean unrelated leases. Pinning the set to explicit + // ids removes that dependency on timestamp uniqueness entirely. + const staleRuns = this.db + .prepare("SELECT id FROM runs WHERE status = 'running'") + .all() as Array<{ id: string }>; + if (staleRuns.length === 0) { + // Nothing to interrupt; still let agent/workspace recovery run + // so a boot that crashed while workers were already idle can + // still reset agent rows parked at `status = 'running'`. + this.db.prepare("update agents set status = 'idle', updated_at = ? where status = 'running'").run(ts); + this.workspacesMod.releaseAllWorkspaceLocks(); + return; } - this.db.prepare("update tasks set active_run_id = null where active_run_id in (select id from runs where status = 'interrupted')").run(); - // B2 Phase 1 step 3/5 — dual-write: drop shadow leases whose run - // has been interrupted. Uses the freshly finished_at timestamp to - // scope the deletion to this sweep's run set, matching the legacy - // path's "just-interrupted" semantics. + const runIds = staleRuns.map((r) => r.id); + const placeholders = runIds.map(() => "?").join(","); this.db .prepare( - "DELETE FROM task_leases WHERE run_id IN (SELECT id FROM runs WHERE status = 'interrupted' AND finished_at = ?)", + `update runs set status = 'interrupted', error_message = 'AgentCompany restarted before the run could report completion.', finished_at = ?, updated_at = ? where id in (${placeholders})`, ) - .run(ts); + .run(ts, ts, ...runIds); + interruptedCount = runIds.length; + this.db + .prepare( + // eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: crash-recovery sweep scoped to the exact run ids captured above. The trigger mirrors status→phase; the DELETE below drops shadow leases for the same id set. + `update tasks set status = 'blocked', updated_at = ? where id in (select task_id from runs where id in (${placeholders})) and status in ('in_progress', 'todo')`, + ) + .run(ts, ...runIds); + this.db + .prepare( + `update tasks set active_run_id = null where active_run_id in (${placeholders})`, + ) + .run(...runIds); + // B2 Phase 1 step 3/5 — dual-write: drop shadow leases whose run + // has been interrupted. Scoped by explicit run id list so two + // sweeps with identical ISO-millisecond `finished_at` stamps can + // never alias each other's lease cleanup. + this.db + .prepare( + `DELETE FROM task_leases WHERE run_id IN (${placeholders})`, + ) + .run(...runIds); this.db.prepare("update agents set status = 'idle', updated_at = ? where status = 'running'").run(ts); this.workspacesMod.releaseAllWorkspaceLocks(); }); @@ -280,7 +306,12 @@ export class AppDatabaseBase { ); const ts = nowIso(); for (const task of stale) { - this.db.prepare(`UPDATE tasks SET checkout_run_id = NULL, execution_locked_at = NULL, status = 'todo', updated_at = ? WHERE id = ?`).run(ts, task.id); + this.db + .prepare( + // eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: stale-lock recovery bounces the task back to todo and drops the shadow lease on the next statement; trigger mirrors status→phase. + `UPDATE tasks SET checkout_run_id = NULL, execution_locked_at = NULL, status = 'todo', updated_at = ? WHERE id = ?`, + ) + .run(ts, task.id); // B2 Phase 1 step 3/5 — dual-write: drop the shadow lease whose // legacy counterpart was just torn down. Matches the legacy // path's "clear everything for this stuck task" semantics; the diff --git a/src/main/database/tasks-crud.ts b/src/main/database/tasks-crud.ts index 9cb1fdc8..fa522ff9 100644 --- a/src/main/database/tasks-crud.ts +++ b/src/main/database/tasks-crud.ts @@ -70,6 +70,7 @@ export function buildTaskCrudMethods( if (existing) { getDb() .prepare( + // eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: saveTask upsert path writes the full row (including status) as a single atomic UPDATE. The status→phase trigger keeps the shadow phase column in sync; no lease is ever held by saveTask callers (lease writes live behind checkoutTask). "update tasks set project_id = ?, goal_id = ?, parent_id = ?, title = ?, description = ?, assignee_agent_id = ?, workspace_id = ?, priority = ?, status = ?, updated_at = ? where id = ? and company_id = ?", ) .run( diff --git a/src/main/database/tasks-status.ts b/src/main/database/tasks-status.ts index 7ba9e7c3..4ca6fcd3 100644 --- a/src/main/database/tasks-status.ts +++ b/src/main/database/tasks-status.ts @@ -43,12 +43,26 @@ function writeCheckoutLease( export function buildTaskStatusMethods( getDb: () => DatabaseSync, readAll: ReadAll, - _transaction: Transaction, + transaction: Transaction, deps: { addComment: (input: { companyId: string; taskId: string; authorAgentId: string | null; authorName: string; body: string }) => void; addActivity: (input: { companyId: string; actor: string; action: string; entityType: string; entityId: string; detail: string }) => void; }, ) { + // Terminal phases and "returned to queue" phases release any existing + // lease: once a task is done/cancelled/blocked or bounced back to + // todo/backlog the execution grant no longer has a referent. We mirror + // that in `task_leases` explicitly — the status→phase trigger only + // touches the phase column and leaves the lease row behind, producing + // an orphan that the dev-mode consistency probe surfaces as a + // divergence. Production would silently drift instead. + const STATUSES_THAT_CLEAR_LEASE = new Set([ + "done", + "cancelled", + "blocked", + "todo", + "backlog", + ]); function incrementActiveSprintVelocity(companyId: string, delta: number): void { const ts = nowIso(); const today = ts.slice(0, 10); @@ -150,9 +164,27 @@ export function buildTaskStatusMethods( timestampSets.push("active_run_id = NULL"); } - getDb() - .prepare(`UPDATE tasks SET ${timestampSets.join(", ")} WHERE id = ?`) - .run(...timestampValues, taskId); + const shouldClearLease = STATUSES_THAT_CLEAR_LEASE.has(newStatus); + transaction(() => { + getDb() + .prepare( + // eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: advanceTaskStatus is the sanctioned dynamic-SQL helper; timestampSets[0] is always the literal "status = ?". Status→phase trigger plus the explicit DELETE FROM task_leases below keep the shadow surface coherent. + `UPDATE tasks SET ${timestampSets.join(", ")} WHERE id = ?`, + ) + .run(...timestampValues, taskId); + if (shouldClearLease) { + // Drop any live lease row in the same transaction so no + // caller observes the intermediate state of "task in terminal + // or backlog phase, lease still present". `releaseTaskLock` + // clears the lease on the happy path (normal run completion), + // but terminal transitions can come from review timeouts, + // retries, automation rules, etc. without ever going through + // that helper. + getDb() + .prepare("DELETE FROM task_leases WHERE task_id = ?") + .run(taskId); + } + }); if (comment) { deps.addComment({ @@ -205,9 +237,9 @@ export function buildTaskStatusMethods( // The `active_run_id IS NULL` guard preserves the original CAS // contract for the busy case: a task whose active_run_id still // points at a live run is refused here, exactly as before. - const result = getDb() .prepare( + // eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: checkoutTask CAS writes status='in_progress' as part of the atomic lock acquisition. The status→phase trigger mirrors to the new phase column and writeCheckoutLease below writes the shadow lease in the same call. `UPDATE tasks SET checkout_run_id = ?, execution_locked_at = ?, status = 'in_progress', updated_at = ? WHERE id = ? diff --git a/tests/unit/tasks-phase.test.ts b/tests/unit/tasks-phase.test.ts index b11f8d21..ed249e2c 100644 --- a/tests/unit/tasks-phase.test.ts +++ b/tests/unit/tasks-phase.test.ts @@ -280,6 +280,116 @@ describe("tasks-phase helpers", () => { }); }); + describe("advanceTaskStatus lease cleanup", () => { + // B2 Phase 1 follow-up (Linus review ask #1): advanceTaskStatus + // used to write only the status column, leaving any lease row + // behind as an orphan. The helpers below seed a lease directly, + // then drive status transitions and assert the shadow row lifecycle. + function seedLease(taskId: string, runId: string): void { + const acquired = new Date().toISOString(); + const expires = new Date(Date.now() + 60_000).toISOString(); + (db as unknown as { + db: { prepare: (sql: string) => { run: (...args: unknown[]) => unknown } }; + }).db + .prepare( + `INSERT OR REPLACE INTO task_leases + (task_id, run_id, acquired_at, expires_at, checkout_run_id) + VALUES (?, ?, ?, ?, ?)`, + ) + .run(taskId, runId, acquired, expires, runId); + } + + function readLease(taskId: string): { run_id: string } | undefined { + return (db as unknown as { + db: { prepare: (sql: string) => { get: (...args: unknown[]) => unknown } }; + }).db + .prepare("SELECT run_id FROM task_leases WHERE task_id = ?") + .get(taskId) as { run_id: string } | undefined; + } + + it("drops the lease when the task transitions to done", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + const runId = createTestRun(db, companyId, taskId, agentId, workspaceId); + + db.tasks.advanceTaskStatus(taskId, "in_progress"); + seedLease(taskId, runId); + expect(readLease(taskId)?.run_id).toBe(runId); + + db.tasks.advanceTaskStatus(taskId, "done"); + expect(readLease(taskId)).toBeUndefined(); + }); + + it("drops the lease when the task transitions to cancelled", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + const runId = createTestRun(db, companyId, taskId, agentId, workspaceId); + + db.tasks.advanceTaskStatus(taskId, "in_progress"); + seedLease(taskId, runId); + + db.tasks.advanceTaskStatus(taskId, "cancelled"); + expect(readLease(taskId)).toBeUndefined(); + }); + + it("drops the lease when the task transitions to blocked", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + const runId = createTestRun(db, companyId, taskId, agentId, workspaceId); + + db.tasks.advanceTaskStatus(taskId, "in_progress"); + seedLease(taskId, runId); + + db.tasks.advanceTaskStatus(taskId, "blocked"); + expect(readLease(taskId)).toBeUndefined(); + }); + + it("drops the lease when the task is bounced back to todo", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + const runId = createTestRun(db, companyId, taskId, agentId, workspaceId); + + db.tasks.advanceTaskStatus(taskId, "in_progress"); + seedLease(taskId, runId); + + db.tasks.advanceTaskStatus(taskId, "todo"); + expect(readLease(taskId)).toBeUndefined(); + }); + + it("leaves the lease intact when the task transitions to in_review", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + // in_review guard requires either a reports_to or requires_user_review. + db.tasks.setTaskRequiresUserReview( + createTestTask(db, companyId, agentId, workspaceId), + true, + ); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + db.tasks.setTaskRequiresUserReview(taskId, true); + const runId = createTestRun(db, companyId, taskId, agentId, workspaceId); + + db.tasks.advanceTaskStatus(taskId, "in_progress"); + seedLease(taskId, runId); + + db.tasks.advanceTaskStatus(taskId, "in_review"); + expect(readLease(taskId)?.run_id).toBe(runId); + }); + }); + describe("releaseTaskLease", () => { it("releases the lease when the runId matches", () => { const companyId = createTestCompany(db);