From 08667637e6cd46e04c98b40f7a55137350ee2e4a Mon Sep 17 00:00:00 2001 From: chahinebrini Date: Sun, 12 Jul 2026 01:24:34 +0200 Subject: [PATCH] =?UTF-8?q?fix(agenthub):=20TSK-0007=20=E2=80=94=20FTS5=20?= =?UTF-8?q?duplicate-on-update=20+=20claimTask=20race-guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- src/core/index.ts | 10 +++-- src/core/services/taskService.ts | 22 +++++++++++ src/server/routes.ts | 9 ++++- tests/coreCorrectness.test.ts | 66 ++++++++++++++++++++++++++++++++ tests/server.test.ts | 12 ++++++ 5 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 tests/coreCorrectness.test.ts diff --git a/src/core/index.ts b/src/core/index.ts index 3a88118..71e0d47 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -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); } } diff --git a/src/core/services/taskService.ts b/src/core/services/taskService.ts index 43e64cd..7920135 100644 --- a/src/core/services/taskService.ts +++ b/src/core/services/taskService.ts @@ -59,7 +59,29 @@ export function updateTask(cwd: string, id: string, patch: Partial): 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 }); } diff --git a/src/server/routes.ts b/src/server/routes.ts index 6931ce0..0efd463 100644 --- a/src/server/routes.ts +++ b/src/server/routes.ts @@ -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'; - task = claimTask(cwd, id, agent); + // 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': diff --git a/tests/coreCorrectness.test.ts b/tests/coreCorrectness.test.ts new file mode 100644 index 0000000..1d1e362 --- /dev/null +++ b/tests/coreCorrectness.test.ts @@ -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); + }); +}); diff --git a/tests/server.test.ts b/tests/server.test.ts index b52263b..9fcb5d7 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -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' } });