fix(db): strip implicit data mutations from startup
`init()` ran three implicit data-repair queries on every boot: `reconcileOrphanedTasks`, `reconcileUnreviewableTasks`, and a secrets-table migration whose fallback glued legacy rows onto a synthetic zero UUID when no current company existed. The first two papered over state-machine leaks at init time — hiding the real bug and making startup observably mutate production data; the third silently connected ciphertext to a non-existent company with no UI visibility and no deletion path. Root fix: the startup path is now allowed exactly one mutation — `interruptStaleRuns`, which is crash recovery (running-at-boot = worker died mid-turn) and cannot be expressed anywhere else. The dead-letter `in_review` case moves to the transition point: `tasks-status.guardInReviewHasReviewer` refuses to advance a task into `in_review` when its assignee is a top-level agent (`reports_to IS NULL`) and the task is not flagged for user review. Callers either pre-flag the task (the CEO-completion path in `automation-run-completion.advanceToInReview` now does this) or the transition is rejected. No more silent resurrection at boot; the failure is visible and actionable at source. The orphan-task reconcile disappears entirely — the `finishRun` guard already enforces the invariant at the one place a run can leave the in-progress state, and `recoverStaleLocks` handles stuck executions at runtime. The secrets migration now throws when neither `settings.current_company_id` nor any company row exists, rather than binding legacy ciphertext to `00000000-0000-0000-0000-000000000000`. Legacy rows stay on the old table until the operator creates a real company, at which point the upgrade succeeds deterministically. Tests rewritten: the reconciliation regression suite now asserts the transition-time guard and the lone `interruptStaleRuns` behavior. `resolve-workspace.test.ts` catches up with the exclusive-by-default lock policy committed earlier.
This commit is contained in:
ZacharyZhang-NY committed
1 parent
cdf783c0b8
commit
7f23f40e87
6 files changed
+94
-208
No files matched your search
@@ -420,124 +420,45 @@ describe("CAS checkout", () => {
|
||||
expect(db.getTask(taskId).status).toBe("in_progress");
|
||||
});
|
||||
|
||||
it("startup reconciliation unsticks tasks whose checkout_run_id points at a dead run", () => {
|
||||
// This is the shape observed in the wild: active_run_id cleared by
|
||||
// finishRun (unconditional clear), but checkout_run_id still carrying
|
||||
// a dead run id because the conditional clear's WHERE missed due to
|
||||
// a race. Task sits in 'in_progress' forever — heartbeat picks it
|
||||
// (activeRunId null), checkoutTask refuses (status != todo/backlog),
|
||||
// "lost checkout race" on loop. Reconciliation must fix it without
|
||||
// caring which reference column is null.
|
||||
it("advanceTaskStatus('in_review') rejects a CEO self-review that has no reviewer", () => {
|
||||
// Regression guard for the CEO dead-letter case: a task whose
|
||||
// assignee is a top-level agent (reports_to IS NULL) and that is
|
||||
// not flagged for user review has no reachable reviewer. Previous
|
||||
// versions silently resurrected these tasks at every DB init; the
|
||||
// root fix rejects the transition at source so callers either
|
||||
// escalate to the human or leave the task in a reclaimable state.
|
||||
const companyId = createTestCompany(db);
|
||||
createTestConnector(db);
|
||||
const workspaceId = createTestWorkspace(db, companyId);
|
||||
const agentId = createTestAgent(db, companyId, workspaceId);
|
||||
const taskId = createTestTask(db, companyId, null, workspaceId);
|
||||
|
||||
const { id: deadRunId } = db.createRun({
|
||||
companyId,
|
||||
taskId,
|
||||
agentId,
|
||||
workspaceId,
|
||||
connectorId: "openai-codex",
|
||||
});
|
||||
db.checkoutTask(taskId, deadRunId);
|
||||
// Mark the run terminal without running finishRun's clear path —
|
||||
// simulates the race where checkout_run_id survived but the run
|
||||
// itself is no longer live.
|
||||
(db as unknown as RawDb).db
|
||||
.prepare("UPDATE runs SET status = 'cancelled' WHERE id = ?")
|
||||
.run(deadRunId);
|
||||
(db as unknown as RawDb).db
|
||||
.prepare("UPDATE tasks SET active_run_id = NULL WHERE id = ?")
|
||||
.run(taskId);
|
||||
|
||||
// Pre-condition: task is stuck.
|
||||
const stuck = (db as unknown as RawDb).db
|
||||
.prepare("SELECT status, checkout_run_id as checkoutRunId, active_run_id as activeRunId FROM tasks WHERE id = ?")
|
||||
.get(taskId) as { status: string; checkoutRunId: string | null; activeRunId: string | null };
|
||||
expect(stuck.status).toBe("in_progress");
|
||||
expect(stuck.checkoutRunId).toBe(deadRunId);
|
||||
expect(stuck.activeRunId).toBeNull();
|
||||
|
||||
// Re-initialise: reconciliation should run.
|
||||
db.close();
|
||||
db = new AppDatabase(tempDir);
|
||||
db.init();
|
||||
|
||||
expect(db.lastOrphanResetCount).toBe(1);
|
||||
const reclaimed = db.getTask(taskId);
|
||||
expect(reclaimed.status).toBe("todo");
|
||||
|
||||
// And a fresh checkout must succeed — proving the task is reclaimable.
|
||||
const { id: newRunId } = db.createRun({
|
||||
companyId,
|
||||
taskId,
|
||||
agentId,
|
||||
workspaceId,
|
||||
connectorId: "openai-codex",
|
||||
});
|
||||
expect(db.checkoutTask(taskId, newRunId)).toBe(true);
|
||||
});
|
||||
|
||||
it("startup reconciliation unsticks in_review tasks with no possible reviewer", () => {
|
||||
// Observed in production: a fresh `ai_ceo` company with only a CEO
|
||||
// self-assigned a large task and set status=in_review. The reviewer
|
||||
// would be the assignee's manager (walk up reports_to), but the CEO
|
||||
// has no manager. With `requires_user_review = 0` there is no human
|
||||
// fallback either. The heartbeat falls into a loop re-proposing
|
||||
// `approve_ceo_strategy` every cycle without ever hiring or moving
|
||||
// the task. Demoting to 'todo' breaks the loop.
|
||||
const companyId = createTestCompany(db);
|
||||
createTestConnector(db);
|
||||
const workspaceId = createTestWorkspace(db, companyId);
|
||||
// Top-level agent (CEO): reports_to stays NULL.
|
||||
const ceoId = createTestAgent(db, companyId, workspaceId, "CEO");
|
||||
const taskId = createTestTask(db, companyId, ceoId, workspaceId);
|
||||
// Manually wedge the task into the dead-letter shape.
|
||||
(db as unknown as RawDb).db
|
||||
.prepare("UPDATE tasks SET status = 'in_review', requires_user_review = 0 WHERE id = ?")
|
||||
.run(taskId);
|
||||
|
||||
expect(db.getTask(taskId).status).toBe("in_review");
|
||||
|
||||
// Re-init triggers reconciliation.
|
||||
db.close();
|
||||
db = new AppDatabase(tempDir);
|
||||
db.init();
|
||||
|
||||
expect(db.lastUnreviewableResetCount).toBe(1);
|
||||
expect(() => db.advanceTaskStatus(taskId, "in_review")).toThrow(/no reviewer/);
|
||||
// Task status must remain unchanged after the rejected transition.
|
||||
expect(db.getTask(taskId).status).toBe("todo");
|
||||
});
|
||||
|
||||
it("in_review tasks requiring user review are left alone", () => {
|
||||
// Opposite guard: when requires_user_review = 1 the task is
|
||||
// intentionally parked waiting for a human — the reconciler must
|
||||
// not steal it.
|
||||
it("advanceTaskStatus('in_review') accepts a CEO assignee flagged requires_user_review=1", () => {
|
||||
// The same task becomes legitimate when the human is the reviewer.
|
||||
const companyId = createTestCompany(db);
|
||||
createTestConnector(db);
|
||||
const workspaceId = createTestWorkspace(db, companyId);
|
||||
const ceoId = createTestAgent(db, companyId, workspaceId, "CEO");
|
||||
const taskId = createTestTask(db, companyId, ceoId, workspaceId);
|
||||
(db as unknown as RawDb).db
|
||||
.prepare("UPDATE tasks SET status = 'in_review', requires_user_review = 1 WHERE id = ?")
|
||||
.run(taskId);
|
||||
db.setTaskRequiresUserReview(taskId, true);
|
||||
|
||||
db.close();
|
||||
db = new AppDatabase(tempDir);
|
||||
db.init();
|
||||
|
||||
expect(db.lastUnreviewableResetCount).toBe(0);
|
||||
db.advanceTaskStatus(taskId, "in_review");
|
||||
expect(db.getTask(taskId).status).toBe("in_review");
|
||||
});
|
||||
|
||||
it("reconciliation does not fire when interruptStaleRuns already handled a running task", () => {
|
||||
// Opposite guard: tasks whose run was `running` at shutdown are
|
||||
// handled earlier in init by `interruptStaleRuns` — it marks the
|
||||
// run 'interrupted' and sets the task to 'blocked' for human
|
||||
// review. By the time reconciliation runs, the task is no longer
|
||||
// 'in_progress', so the cleanup's WHERE clause skips it. The user
|
||||
// sees the task surfaced in the recovery UI, not silently reset.
|
||||
it("interruptStaleRuns surfaces shutdown-era running runs without startup task rewrites", () => {
|
||||
// `interruptStaleRuns` is the ONLY mutation the startup path may
|
||||
// perform: any run at 'running' on boot means a crash, so the run
|
||||
// flips to 'interrupted' and the task surfaces as 'blocked' for
|
||||
// operator attention. No other reconciliation runs at init; the
|
||||
// task-status machine (see guardInReviewHasReviewer) and the
|
||||
// runtime `finishRun` guards are now responsible for every other
|
||||
// data-repair concern.
|
||||
const companyId = createTestCompany(db);
|
||||
createTestConnector(db);
|
||||
const workspaceId = createTestWorkspace(db, companyId);
|
||||
@@ -558,10 +479,7 @@ describe("CAS checkout", () => {
|
||||
db = new AppDatabase(tempDir);
|
||||
db.init();
|
||||
|
||||
// Reconciliation counted 0 because interruptStaleRuns moved the
|
||||
// task to 'blocked' first. Both guards collaborate — neither
|
||||
// steals a task out from under an actively-working run.
|
||||
expect(db.lastOrphanResetCount).toBe(0);
|
||||
expect(db.lastInterruptedCount).toBe(1);
|
||||
expect(db.getTask(taskId).status).toBe("blocked");
|
||||
});
|
||||
});
|
||||
@@ -16,10 +16,12 @@ import { AppDatabase } from "../../src/main/database";
|
||||
import { resolveWorkspaceWithLockMode } from "../../src/main/orchestration/resolve-workspace";
|
||||
|
||||
/**
|
||||
* Regression tests for the workspace lock policy introduced in v0.4.168+:
|
||||
* only `task.workspaceId` grants exclusive access. Agent-, project-, and
|
||||
* company-scoped resolutions are all `shared` so teammates can run in
|
||||
* parallel instead of queuing on a single workspace lock.
|
||||
* Regression tests for the current workspace lock policy: every
|
||||
* resolved workspace is held exclusively for the duration of the run.
|
||||
* Pi's coding tools always include bash/write/edit, so there is no
|
||||
* read-only category where concurrent runs are safe; teammates
|
||||
* sharing one repo queue through the wakeup table instead of racing
|
||||
* the filesystem.
|
||||
*
|
||||
* See `resolve-workspace.ts` for the full rationale.
|
||||
*/
|
||||
@@ -75,7 +77,7 @@ describe("resolveWorkspaceWithLockMode", () => {
|
||||
expect(result.workspace?.id).toBe(workspaceId);
|
||||
});
|
||||
|
||||
it("agent-bound workspace → shared (normal case: teammates share a repo)", () => {
|
||||
it("agent-bound workspace → exclusive (single writer per repo)", () => {
|
||||
const { companyId, workspaceId } = seedCompany(db);
|
||||
const result = resolveWorkspaceWithLockMode(
|
||||
db,
|
||||
@@ -84,10 +86,10 @@ describe("resolveWorkspaceWithLockMode", () => {
|
||||
companyId,
|
||||
);
|
||||
expect(result.source).toBe("agent");
|
||||
expect(result.lockMode).toBe("shared");
|
||||
expect(result.lockMode).toBe("exclusive");
|
||||
});
|
||||
|
||||
it("project-bound workspace → shared (team collaboration space)", () => {
|
||||
it("project-bound workspace → exclusive (single writer per repo)", () => {
|
||||
const { companyId } = seedCompany(db);
|
||||
const projectId = db.saveProject({
|
||||
companyId,
|
||||
@@ -95,9 +97,6 @@ describe("resolveWorkspaceWithLockMode", () => {
|
||||
description: "",
|
||||
status: "active",
|
||||
});
|
||||
// Create a second workspace bound to the project so the resolver's
|
||||
// project-primary branch actually fires (the default seeded workspace
|
||||
// has no project_id set).
|
||||
db.saveWorkspace({
|
||||
companyId,
|
||||
name: "Project repo",
|
||||
@@ -115,10 +114,10 @@ describe("resolveWorkspaceWithLockMode", () => {
|
||||
companyId,
|
||||
);
|
||||
expect(result.source).toBe("project");
|
||||
expect(result.lockMode).toBe("shared");
|
||||
expect(result.lockMode).toBe("exclusive");
|
||||
});
|
||||
|
||||
it("company-fallback → shared (unchanged legacy behavior)", () => {
|
||||
it("company-fallback → exclusive (single writer per repo)", () => {
|
||||
const { companyId } = seedCompany(db);
|
||||
const result = resolveWorkspaceWithLockMode(
|
||||
db,
|
||||
@@ -127,7 +126,7 @@ describe("resolveWorkspaceWithLockMode", () => {
|
||||
companyId,
|
||||
);
|
||||
expect(result.source).toBe("company_fallback");
|
||||
expect(result.lockMode).toBe("shared");
|
||||
expect(result.lockMode).toBe("exclusive");
|
||||
});
|
||||
|
||||
it("task workspace wins over agent workspace even when both are set", () => {
|
||||
|
||||
Reference in new issue
Block a user