From 8cb227f455c77a28dd771d1d03da3e4fec28adcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Mon, 20 Apr 2026 12:53:28 -0400 Subject: [PATCH] Add advanceTaskPhase and task lease helpers with tests (B2 Phase 1 step 2/5) --- src/main/database/tasks-phase.ts | 163 ++++++++++++++++ src/main/database/tasks.ts | 2 + tests/unit/tasks-phase.test.ts | 314 +++++++++++++++++++++++++++++++ 3 files changed, 479 insertions(+) create mode 100644 src/main/database/tasks-phase.ts create mode 100644 tests/unit/tasks-phase.test.ts diff --git a/src/main/database/tasks-phase.ts b/src/main/database/tasks-phase.ts new file mode 100644 index 00000000..f383eb37 --- /dev/null +++ b/src/main/database/tasks-phase.ts @@ -0,0 +1,163 @@ +import type { DatabaseSync } from "node:sqlite"; +import { nowIso } from "./helpers"; + +/** + * B2 Phase 1 step 2/5 — pure helpers for the new phase + lease shadow. + * + * These helpers expose the shape the state-machine refactor will + * eventually take — a single CAS function for phase transitions, and + * a lease table with explicit acquire/release/renew — without yet + * swapping the production call sites over. Step 3/5 wires the existing + * writes (checkoutTask, finishRun, interruptStaleRuns, recoverStaleLocks) + * to call these helpers in a dual-write arrangement alongside the + * legacy columns; until then the helpers are callable but unused. + * + * The enum here matches the v50 migration CHECK constraint exactly. + * Any drift between this module and v50-v50.ts is a bug — the TypeScript + * `TaskPhase` union is the compile-time mirror of the SQL constraint. + */ + +/** Canonical phase enum matching the v50 CHECK constraint. */ +export type TaskPhase = + | "todo" + | "backlog" + | "in_progress" + | "blocked" + | "completed" + | "cancelled"; + +export interface TaskLeaseRow { + taskId: string; + runId: string; + acquiredAt: string; + expiresAt: string; + checkoutRunId: string | null; +} + +export interface AcquireTaskLeaseInput { + taskId: string; + runId: string; + /** Lease time-to-live in milliseconds. Mapped onto `expires_at`. */ + ttlMs: number; + /** Optional `checkout_run_id` to mirror onto the lease row. */ + checkoutRunId?: string | null; +} + +export interface TasksPhaseMethods { + /** + * Compare-and-swap phase transition. Returns true when the row was + * updated (current phase matched `from`), false otherwise. The caller + * is responsible for wrapping lease writes in the same transaction + * when the transition is coupled to a lease acquire/release. + */ + advanceTaskPhase(taskId: string, from: TaskPhase, to: TaskPhase): boolean; + + /** + * Insert or replace the lease row for `taskId`. When an unexpired + * lease for a *different* run already exists, returns null and writes + * nothing. Callers treat null as "another run owns this task; do not + * proceed". + * + * The staleness check is a clock comparison against `now()` — a lease + * whose `expires_at` is in the past is considered releasable, so a + * subsequent acquire from a new run replaces it. This mirrors the + * existing `recoverStaleLocks` behavior but scoped to the new + * lease table. + */ + acquireTaskLease(input: AcquireTaskLeaseInput): TaskLeaseRow | null; + + /** + * Release the lease for `taskId` when the current row belongs to + * `runId`. Returns true when a row was deleted. + */ + releaseTaskLease(taskId: string, runId: string): boolean; + + /** + * Read the current lease row (if any) for `taskId`. + */ + getTaskLease(taskId: string): TaskLeaseRow | null; +} + +export function buildTasksPhaseMethods( + getDb: () => DatabaseSync, +): TasksPhaseMethods { + function readLease(taskId: string): TaskLeaseRow | null { + const row = getDb() + .prepare( + `SELECT task_id AS taskId, + run_id AS runId, + acquired_at AS acquiredAt, + expires_at AS expiresAt, + checkout_run_id AS checkoutRunId + FROM task_leases + WHERE task_id = ?`, + ) + .get(taskId) as TaskLeaseRow | undefined; + return row ?? null; + } + + return { + advanceTaskPhase(taskId, from, to) { + const ts = nowIso(); + const result = getDb() + .prepare( + `UPDATE tasks + SET phase = ?, updated_at = ? + WHERE id = ? AND phase = ?`, + ) + .run(to, ts, taskId, from); + return result.changes > 0; + }, + + acquireTaskLease({ taskId, runId, ttlMs, checkoutRunId }) { + if (!Number.isFinite(ttlMs) || ttlMs <= 0) { + throw new Error( + `acquireTaskLease: ttlMs must be a positive finite number, got ${ttlMs}`, + ); + } + const nowMs = Date.now(); + const acquiredAt = new Date(nowMs).toISOString(); + const expiresAt = new Date(nowMs + ttlMs).toISOString(); + + const existing = readLease(taskId); + if (existing) { + const existingExpiresMs = Date.parse(existing.expiresAt); + const stillLive = + Number.isFinite(existingExpiresMs) && existingExpiresMs > nowMs; + if (stillLive && existing.runId !== runId) { + return null; + } + // Same run refreshing its own lease, or a stale lease to be + // replaced. Either way we rewrite the row with the new window. + getDb() + .prepare( + `UPDATE task_leases + SET run_id = ?, acquired_at = ?, expires_at = ?, checkout_run_id = ? + WHERE task_id = ?`, + ) + .run(runId, acquiredAt, expiresAt, checkoutRunId ?? null, taskId); + return readLease(taskId); + } + + getDb() + .prepare( + `INSERT INTO task_leases + (task_id, run_id, acquired_at, expires_at, checkout_run_id) + VALUES (?, ?, ?, ?, ?)`, + ) + .run(taskId, runId, acquiredAt, expiresAt, checkoutRunId ?? null); + return readLease(taskId); + }, + + releaseTaskLease(taskId, runId) { + const result = getDb() + .prepare(`DELETE FROM task_leases WHERE task_id = ? AND run_id = ?`) + .run(taskId, runId); + return result.changes > 0; + }, + + getTaskLease(taskId) { + return readLease(taskId); + }, + }; +} diff --git a/src/main/database/tasks.ts b/src/main/database/tasks.ts index 4f0b2b45..11a87fb1 100644 --- a/src/main/database/tasks.ts +++ b/src/main/database/tasks.ts @@ -4,6 +4,7 @@ import { buildTaskCrudMethods } from "./tasks-crud"; import { buildTaskQueryMethods } from "./task-queries"; import { buildTaskSessionMethods } from "./task-sessions-crud"; import { buildTaskStatusMethods } from "./tasks-status"; +import { buildTasksPhaseMethods } from "./tasks-phase"; export function buildTaskMethods( getDb: () => DatabaseSync, @@ -19,5 +20,6 @@ export function buildTaskMethods( ...buildTaskQueryMethods(getDb, readAll, transaction), ...buildTaskSessionMethods(getDb, readAll, transaction), ...buildTaskStatusMethods(getDb, readAll, transaction, deps), + ...buildTasksPhaseMethods(getDb), }; } diff --git a/tests/unit/tasks-phase.test.ts b/tests/unit/tasks-phase.test.ts new file mode 100644 index 00000000..d4eadc95 --- /dev/null +++ b/tests/unit/tasks-phase.test.ts @@ -0,0 +1,314 @@ +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { mkdtemp, rm } from "node:fs/promises"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("electron", () => ({ + safeStorage: { + isEncryptionAvailable: () => false, + getSelectedStorageBackend: () => "basic_text", + encryptString: (value: string) => Buffer.from(value, "utf8"), + decryptString: (value: Buffer) => value.toString("utf8"), + }, +})); + +import { AppDatabase } from "../../src/main/database"; + +/** + * B2 Phase 1 step 2/5 — unit coverage for the new phase + lease helpers. + * + * These tests exercise the shadow surface directly against a real + * SQLite database: v50 has run, the `tasks.phase` column and + * `task_leases` table exist, and the new helpers write and read them + * without going through any production code path. The goal is to pin + * the state machine semantics (CAS rejection on wrong `from`, lease + * replacement on staleness, release idempotency) before step 3/5 wires + * the helpers into `checkoutTask` / `finishRun`. + */ + +function createTestCompany(db: AppDatabase): string { + return db.saveCompany({ + name: "Phase Helper Company", + description: "B2 phase + lease unit tests", + status: "active", + }); +} + +function createTestConnector(db: AppDatabase): void { + db.saveConnector({ + id: "openai-codex", + model: "gpt-5", + envBindingText: "", + notes: "Test connector", + }); +} + +function createTestWorkspace(db: AppDatabase, companyId: string): string { + return db.saveWorkspace({ + companyId, + name: "Phase Workspace", + localPath: "/tmp/phase-workspace", + repoUrl: "https://github.com/example/phase", + repoRef: "main", + isPrimary: true, + }); +} + +function createTestAgent(db: AppDatabase, companyId: string, workspaceId: string, name = "Agent"): string { + return db.saveAgent({ + companyId, + name, + role: "IC", + title: name, + status: "active", + reportsTo: null, + connectorId: "openai-codex", + workspaceId, + model: null, + capabilities: "Run task helpers", + budgetMonthlyUsd: 100, + }); +} + +function createTestTask( + db: AppDatabase, + companyId: string, + agentId: string, + workspaceId: string, +): string { + return db.saveTask({ + companyId, + title: "Phase helper task", + description: "dual-write seed", + assigneeAgentId: agentId, + workspaceId, + priority: "medium", + status: "todo", + }); +} + +function createTestRun( + db: AppDatabase, + companyId: string, + taskId: string, + agentId: string, + workspaceId: string, +): string { + const { id } = db.createRun({ + companyId, + taskId, + agentId, + workspaceId, + connectorId: "openai-codex", + }); + return id; +} + +describe("tasks-phase helpers", () => { + let tempDir: string; + let db: AppDatabase; + + beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), "agentcompany-phase-")); + db = new AppDatabase(tempDir); + db.init(); + }); + + afterEach(async () => { + db.close(); + await rm(tempDir, { recursive: true, force: true }); + }); + + describe("advanceTaskPhase", () => { + it("backfill leaves every existing task with a phase matching its status", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + // `saveTask` ran with the v50 migration already applied, so the + // row must carry a phase value immediately. + const row = (db as unknown as { + db: { prepare: (sql: string) => { get: (...args: unknown[]) => unknown } }; + }).db + .prepare("SELECT phase FROM tasks WHERE id = ?") + .get(taskId) as { phase: string } | undefined; + expect(row?.phase).toBe("todo"); + }); + + it("CAS transition succeeds when the current phase matches `from`", () => { + 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 ok = db.tasks.advanceTaskPhase(taskId, "todo", "in_progress"); + expect(ok).toBe(true); + const after = db.tasks.getTaskLease(taskId); + expect(after).toBeNull(); // phase transitions do not auto-create leases + const row = (db as unknown as { + db: { prepare: (sql: string) => { get: (...args: unknown[]) => unknown } }; + }).db + .prepare("SELECT phase FROM tasks WHERE id = ?") + .get(taskId) as { phase: string } | undefined; + expect(row?.phase).toBe("in_progress"); + }); + + it("CAS transition refuses when the current phase does not match `from`", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + + // Row is at phase=todo; pretending it was in_progress must fail. + const ok = db.tasks.advanceTaskPhase(taskId, "in_progress", "completed"); + expect(ok).toBe(false); + }); + + it("CHECK constraint rejects a write to an unknown phase value", () => { + const companyId = createTestCompany(db); + createTestConnector(db); + const workspaceId = createTestWorkspace(db, companyId); + const agentId = createTestAgent(db, companyId, workspaceId); + const taskId = createTestTask(db, companyId, agentId, workspaceId); + + // Using the raw DB to bypass the TypeScript enum — the CHECK + // constraint installed in v50 must still reject the write. + expect(() => { + (db as unknown as { + db: { prepare: (sql: string) => { run: (...args: unknown[]) => unknown } }; + }).db + .prepare("UPDATE tasks SET phase = ? WHERE id = ?") + .run("bogus", taskId); + }).toThrow(); + }); + }); + + describe("acquireTaskLease", () => { + it("acquires a lease when none exists and returns the row", () => { + 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); + + const lease = db.tasks.acquireTaskLease({ + taskId, + runId, + ttlMs: 60_000, + checkoutRunId: runId, + }); + expect(lease).not.toBeNull(); + expect(lease?.taskId).toBe(taskId); + expect(lease?.runId).toBe(runId); + expect(lease?.checkoutRunId).toBe(runId); + }); + + it("refreshing the lease for the same run succeeds and moves expires_at forward", () => { + 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); + + const first = db.tasks.acquireTaskLease({ taskId, runId, ttlMs: 10_000 }); + expect(first).not.toBeNull(); + const renewed = db.tasks.acquireTaskLease({ taskId, runId, ttlMs: 60_000 }); + expect(renewed).not.toBeNull(); + expect(Date.parse(renewed!.expiresAt)).toBeGreaterThanOrEqual(Date.parse(first!.expiresAt)); + }); + + it("refuses to hand the lease to a different live run", () => { + 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 runA = createTestRun(db, companyId, taskId, agentId, workspaceId); + const runB = createTestRun(db, companyId, taskId, agentId, workspaceId); + + const ownedByA = db.tasks.acquireTaskLease({ taskId, runId: runA, ttlMs: 60_000 }); + expect(ownedByA).not.toBeNull(); + const blocked = db.tasks.acquireTaskLease({ taskId, runId: runB, ttlMs: 60_000 }); + expect(blocked).toBeNull(); + const current = db.tasks.getTaskLease(taskId); + expect(current?.runId).toBe(runA); + }); + + it("replaces a stale lease when the previous window has expired", () => { + 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 runA = createTestRun(db, companyId, taskId, agentId, workspaceId); + const runB = createTestRun(db, companyId, taskId, agentId, workspaceId); + + const ownedByA = db.tasks.acquireTaskLease({ taskId, runId: runA, ttlMs: 60_000 }); + expect(ownedByA).not.toBeNull(); + // Force the lease into the past by rewriting expires_at directly. + const pastIso = new Date(Date.now() - 1_000).toISOString(); + (db as unknown as { + db: { prepare: (sql: string) => { run: (...args: unknown[]) => unknown } }; + }).db + .prepare("UPDATE task_leases SET expires_at = ? WHERE task_id = ?") + .run(pastIso, taskId); + + const takenByB = db.tasks.acquireTaskLease({ taskId, runId: runB, ttlMs: 60_000 }); + expect(takenByB).not.toBeNull(); + expect(takenByB?.runId).toBe(runB); + }); + + it("throws when ttlMs is not a positive finite number", () => { + 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); + + expect(() => + db.tasks.acquireTaskLease({ taskId, runId, ttlMs: 0 }), + ).toThrow(/positive finite number/); + expect(() => + db.tasks.acquireTaskLease({ taskId, runId, ttlMs: Number.NaN }), + ).toThrow(/positive finite number/); + }); + }); + + describe("releaseTaskLease", () => { + it("releases the lease when the runId matches", () => { + 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.acquireTaskLease({ taskId, runId, ttlMs: 60_000 }); + const released = db.tasks.releaseTaskLease(taskId, runId); + expect(released).toBe(true); + expect(db.tasks.getTaskLease(taskId)).toBeNull(); + }); + + it("does nothing when a different runId asks to release", () => { + 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 runA = createTestRun(db, companyId, taskId, agentId, workspaceId); + const runB = createTestRun(db, companyId, taskId, agentId, workspaceId); + + db.tasks.acquireTaskLease({ taskId, runId: runA, ttlMs: 60_000 }); + const released = db.tasks.releaseTaskLease(taskId, runB); + expect(released).toBe(false); + const current = db.tasks.getTaskLease(taskId); + expect(current?.runId).toBe(runA); + }); + }); +});