fix(agenthub): TSK-0007 — FTS5 duplicate-on-update + claimTask race-guard
- core/index.ts: the FTS5 'search' table has no UNIQUE on id, so INSERT OR REPLACE appended a new row every update and the search JOIN returned an id N times. Now delete-then-insert by id -> exactly one FTS row per entity. - taskService.claimTask: race-guarded — only an OPEN task can be claimed; same-agent re-claim is an idempotent no-op; a task claimed by another agent or past open is refused. getTask+updateTask are sync, so within the single-thread event loop the read-check-write is effectively atomic (first claim wins). - routes.ts (spec 'nicht brechen'): PATCH in_progress surfaces a lost/non-open claim as a clean 400 instead of 500, so the board drag reverts gracefully and a second agent can't clobber the first's claim. - tests: +coreCorrectness.test.ts (no FTS dup after updates; concurrent claims -> one wins; idempotent re-claim; refuse non-open) + server.test.ts route guard Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
f0c7be25f4
commit
08667637e6
@ -108,10 +108,12 @@ export class Index {
|
||||
insert.run(params);
|
||||
|
||||
if (fts) {
|
||||
const search = this.db.prepare(`
|
||||
INSERT OR REPLACE INTO search (id, title, content) VALUES (@id, @title, @content)
|
||||
`);
|
||||
search.run(params);
|
||||
// The FTS5 `search` table has no UNIQUE key on `id` (it's a plain indexed
|
||||
// column), so `INSERT OR REPLACE` would NOT replace — it would append a new
|
||||
// row on every update, and the search JOIN would then return the same id
|
||||
// multiple times. Delete-then-insert keeps exactly one FTS row per id.
|
||||
this.db.prepare('DELETE FROM search WHERE id = @id').run({ id: params.id });
|
||||
this.db.prepare('INSERT INTO search (id, title, content) VALUES (@id, @title, @content)').run(params);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -59,7 +59,29 @@ export function updateTask(cwd: string, id: string, patch: Partial<Task>): Task
|
||||
return updated;
|
||||
}
|
||||
|
||||
/**
|
||||
* Claim a task for an agent (open → in_progress). Race-guarded: only an OPEN
|
||||
* task can be claimed. A re-claim by the SAME agent (retried request) is an
|
||||
* idempotent no-op; a task already claimed by someone else — or past `open`
|
||||
* (review/done/cancelled) — is refused, so two agents can't claim the same task
|
||||
* and a claim can't clobber another agent's in-progress work.
|
||||
*
|
||||
* `getTask`+`updateTask` are synchronous, so within the single-threaded server
|
||||
* event loop this read-check-write is effectively atomic: of two concurrent
|
||||
* claims on the same open task, the first completes the write and the second
|
||||
* then sees `in_progress` and is refused.
|
||||
*/
|
||||
export function claimTask(cwd: string, id: string, agentName: string): Task {
|
||||
const { task } = getTask(cwd, id);
|
||||
// Idempotent: the same agent re-claiming its own in-progress task is a no-op.
|
||||
if (task.status === 'in_progress' && task.claimedBy === agentName) {
|
||||
return task;
|
||||
}
|
||||
if (task.status !== 'open') {
|
||||
throw new Error(
|
||||
`Task ${id} cannot be claimed — status is "${task.status}"${task.claimedBy ? ` (claimed by ${task.claimedBy})` : ''}.`,
|
||||
);
|
||||
}
|
||||
return updateTask(cwd, id, { status: 'in_progress', assignedTo: agentName, claimedBy: agentName });
|
||||
}
|
||||
|
||||
|
||||
@ -340,7 +340,14 @@ export async function registerRoutes(app: FastifyInstance, cwd: string): Promise
|
||||
{
|
||||
const current = getTask(cwd, id).task;
|
||||
const agent = patch.assignedTo?.trim() || current.assignedTo || 'manual';
|
||||
// claimTask is race-guarded (open-only). Surface a lost race / non-open
|
||||
// claim as a clean 400 so the board drag reverts gracefully instead of
|
||||
// 500-ing, and a second agent can't clobber the first's claim.
|
||||
try {
|
||||
task = claimTask(cwd, id, agent);
|
||||
} catch (err) {
|
||||
return badRequest(reply, err instanceof Error ? err.message : 'Cannot claim task');
|
||||
}
|
||||
}
|
||||
break;
|
||||
case 'done':
|
||||
|
||||
66
tests/coreCorrectness.test.ts
Normal file
66
tests/coreCorrectness.test.ts
Normal file
@ -0,0 +1,66 @@
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { mkdtempSync, rmSync } from 'fs';
|
||||
import { tmpdir } from 'os';
|
||||
import { join } from 'path';
|
||||
import { init } from '../src/cli/commands/init.js';
|
||||
import { createTask, claimTask, reviewTask } from '../src/core/services/taskService.js';
|
||||
import { searchMemory } from '../src/core/services/memoryService.js';
|
||||
|
||||
describe('core correctness (TSK-0007)', () => {
|
||||
let cwd: string;
|
||||
|
||||
beforeEach(() => {
|
||||
cwd = mkdtempSync(join(tmpdir(), 'ah-core-'));
|
||||
init(cwd, { projectName: 'core-test', yes: true });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// ── FTS5 duplicate-on-update ──────────────────────────────────────────────
|
||||
it('search returns no duplicates after an entity is updated repeatedly', () => {
|
||||
const t = createTask(cwd, { title: 'Zephyr indexing widget', role: 'implementer' });
|
||||
// Each mutation re-upserts into the FTS index.
|
||||
claimTask(cwd, t.id, 'kimi');
|
||||
reviewTask(cwd, t.id);
|
||||
|
||||
const hits = searchMemory(cwd, 'Zephyr');
|
||||
const forTask = hits.filter((r) => r.id === t.id);
|
||||
expect(forTask).toHaveLength(1); // exactly one, not one-per-update
|
||||
});
|
||||
|
||||
// ── claimTask race-guard ──────────────────────────────────────────────────
|
||||
it('only one of two concurrent claims on the same open task wins', () => {
|
||||
const t = createTask(cwd, { title: 'contended', role: 'implementer' });
|
||||
|
||||
const winners: string[] = [];
|
||||
let refused = 0;
|
||||
for (const agent of ['kimi', 'codex']) {
|
||||
try {
|
||||
const claimed = claimTask(cwd, t.id, agent);
|
||||
winners.push(claimed.assignedTo ?? '');
|
||||
} catch {
|
||||
refused += 1;
|
||||
}
|
||||
}
|
||||
expect(winners).toHaveLength(1);
|
||||
expect(refused).toBe(1);
|
||||
expect(winners[0]).toBe('kimi'); // the first to act wins
|
||||
});
|
||||
|
||||
it('a re-claim by the same agent is an idempotent no-op', () => {
|
||||
const t = createTask(cwd, { title: 'mine', role: 'implementer' });
|
||||
claimTask(cwd, t.id, 'kimi');
|
||||
const again = claimTask(cwd, t.id, 'kimi'); // no throw
|
||||
expect(again.status).toBe('in_progress');
|
||||
expect(again.claimedBy).toBe('kimi');
|
||||
});
|
||||
|
||||
it('refuses to claim a task that is not open', () => {
|
||||
const t = createTask(cwd, { title: 'x', role: 'implementer' });
|
||||
claimTask(cwd, t.id, 'kimi');
|
||||
reviewTask(cwd, t.id); // now in review
|
||||
expect(() => claimTask(cwd, t.id, 'codex')).toThrow(/cannot be claimed/i);
|
||||
});
|
||||
});
|
||||
@ -69,6 +69,18 @@ describe('server routes', () => {
|
||||
expect(JSON.parse(res.payload).status).toBe('review');
|
||||
});
|
||||
|
||||
it('a second claim of the same task returns a clean 400 (race guard, board does not 500) — TSK-0007', async () => {
|
||||
await app.inject({ method: 'POST', url: '/tasks', payload: { title: 'A', role: 'implementer' } });
|
||||
const first = await app.inject({ method: 'PATCH', url: '/tasks/TSK-0001', payload: { status: 'in_progress', assignedTo: 'kimi' } });
|
||||
expect(first.statusCode).toBe(200);
|
||||
// Another agent tries to claim the now in-progress task.
|
||||
const second = await app.inject({ method: 'PATCH', url: '/tasks/TSK-0001', payload: { status: 'in_progress', assignedTo: 'codex' } });
|
||||
expect(second.statusCode).toBe(400);
|
||||
// The original claim is intact — not clobbered.
|
||||
const still = JSON.parse((await app.inject({ method: 'GET', url: '/tasks/TSK-0001' })).payload) as { task: { claimedBy?: string } };
|
||||
expect(still.task.claimedBy).toBe('kimi');
|
||||
});
|
||||
|
||||
it('PATCH /tasks/:id → review accepts reviewer separately from assignedTo', async () => {
|
||||
await app.inject({ method: 'POST', url: '/tasks', payload: { title: 'A', role: 'implementer', assignedTo: 'codex' } });
|
||||
const res = await app.inject({ method: 'PATCH', url: '/tasks/TSK-0001', payload: { status: 'review', reviewer: 'claude' } });
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user