From b656a241a21e6355c931f0ef3747993374e0b197 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Tue, 11 Aug 2026 16:57:23 +0200 Subject: [PATCH] fix(worker,data): honor wait_for_task_change's NotFound contract, harden TaskNumberAllocator wait_for_task_change resolved #/bare-number ids up front via TaskIdResolver, which throws for an unknown number -- breaking the tool's own documented promise that an unknown id reports status "NotFound" instead of failing the whole call. Resolve per id and fall back to the original id on a resolution failure so CheckOnceAsync can still report it. TaskNumberAllocator.AddWithNumberAsync indexed into the app_settings UPDATE...RETURNING result without checking for an empty result, throwing on a missing singleton row; it also caught any DbUpdateException as a number collision, burning up to 5 numbers on an unrelated failure (e.g. FK violation) before the real error surfaced. Now recreates the missing row and only retries on the actual unique-index collision (SQLite error 19 on tasks.number), rethrowing everything else immediately. Audited the other TaskIdResolver.ResolveAsync/ResolveManyAsync call sites (ExternalMcpService, HandoffMcpTools, ConfigMcpTools, RunHistoryMcpTools, AttachmentMcpTools, LifecycleMcpTools, BatchMcpTools): none of their tool descriptions promise a found/NotFound flag for the id itself (BatchMcpTools.BatchGetTasks already handles this correctly via its own per-id try/catch; PreviewMergeSet promises a per-task "error" field, not a found/NotFound flag; the rest are single-id tools that already throw on a missing task downstream) -- left throwing behavior as-is. --- src/ClaudeDo.Data/TaskNumberAllocator.cs | 43 ++++++++++++++++- .../External/TaskWaitMcpTools.cs | 14 +++++- .../TaskNumberAllocatorTests.cs | 29 ++++++++++++ .../External/TaskWaitMcpToolsTests.cs | 46 +++++++++++++++++++ 4 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/ClaudeDo.Data/TaskNumberAllocator.cs b/src/ClaudeDo.Data/TaskNumberAllocator.cs index b51edf7c..2e54f940 100644 --- a/src/ClaudeDo.Data/TaskNumberAllocator.cs +++ b/src/ClaudeDo.Data/TaskNumberAllocator.cs @@ -1,4 +1,5 @@ using ClaudeDo.Data.Models; +using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; namespace ClaudeDo.Data; @@ -29,6 +30,16 @@ public static class TaskNumberAllocator RETURNING * """, AppSettingsEntity.SingletonId).AsNoTracking().ToListAsync(ct); + if (settings.Count == 0) + { + // The singleton row is missing (e.g. a corrupted/hand-edited DB) -- recreate it, + // as its own separate insert/commit, and retry the UPDATE above rather than + // indexing into an empty result. Doesn't count against MaxAttempts: it's not a + // number collision. + await EnsureSingletonRowAsync(context, ct); + continue; + } + entity.Number = settings[0].NextTaskNumber - 1; context.Tasks.Add(entity); @@ -37,10 +48,40 @@ public static class TaskNumberAllocator await context.SaveChangesAsync(ct); return; } - catch (DbUpdateException) when (attempt < MaxAttempts) + catch (DbUpdateException ex) { context.Entry(entity).State = EntityState.Detached; + if (attempt < MaxAttempts && IsNumberCollision(ex)) + continue; + throw; } } } + + private static async Task EnsureSingletonRowAsync(ClaudeDoDbContext context, CancellationToken ct) + { + var row = new AppSettingsEntity { Id = AppSettingsEntity.SingletonId }; + context.AppSettings.Add(row); + try + { + await context.SaveChangesAsync(ct); + } + catch (DbUpdateException) + { + // A concurrent process already inserted the singleton -- fine, the next UPDATE + // attempt above will find it. + } + finally + { + context.Entry(row).State = EntityState.Detached; + } + } + + // Distinguishes the expected unique-index collision on idx_tasks_number (worth retrying with + // a fresh number) from any other DbUpdateException -- an FK violation, a NOT NULL violation, + // etc. -- which must surface immediately instead of silently burning up to MaxAttempts numbers + // on a failure a retry can never fix. + private static bool IsNumberCollision(DbUpdateException ex) => + ex.InnerException is SqliteException { SqliteErrorCode: 19 } sqliteEx + && sqliteEx.Message.Contains("tasks.number", StringComparison.Ordinal); } diff --git a/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs index 7079672a..4f2b4ce7 100644 --- a/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs +++ b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs @@ -109,7 +109,19 @@ public sealed class TaskWaitMcpTools var tasks = new TaskRepository(ctx); var resolved = new string[ids.Length]; for (var i = 0; i < ids.Length; i++) - resolved[i] = await TaskIdResolver.ResolveAsync(tasks, ids[i], ct); + { + try + { + resolved[i] = await TaskIdResolver.ResolveAsync(tasks, ids[i], ct); + } + catch (InvalidOperationException) + { + // A #/bare-number id with no matching task -- keep the original, + // unresolvable id so CheckOnceAsync's lookup misses and reports "NotFound", + // per this tool's documented contract, instead of failing the whole call. + resolved[i] = ids[i]; + } + } return resolved; } diff --git a/tests/ClaudeDo.Data.Tests/TaskNumberAllocatorTests.cs b/tests/ClaudeDo.Data.Tests/TaskNumberAllocatorTests.cs index 93be3071..fc341443 100644 --- a/tests/ClaudeDo.Data.Tests/TaskNumberAllocatorTests.cs +++ b/tests/ClaudeDo.Data.Tests/TaskNumberAllocatorTests.cs @@ -131,6 +131,35 @@ public sealed class TaskNumberAllocatorTests : IDisposable Assert.All(numbers, n => Assert.True(n > 0)); } + [Fact] + public async Task AddAsync_recreates_a_missing_app_settings_row_instead_of_crashing() + { + await SeedListAsync(); + await _ctx.Database.ExecuteSqlRawAsync("DELETE FROM app_settings"); + + var task = NewTask("l1"); + await new TaskRepository(_ctx).AddAsync(task); + + Assert.True(task.Number > 0); + } + + [Fact] + public async Task AddAsync_noncollision_DbUpdateException_surfaces_immediately_without_burning_numbers() + { + await SeedListAsync(); + var badTask = NewTask("no-such-list"); + + await Assert.ThrowsAsync(() => new TaskRepository(_ctx).AddAsync(badTask)); + + // The failed attempt above should have burned exactly one number (attempt #1, no + // collision-driven retries) -- confirm the next successful insert isn't several numbers + // further along. + var goodTask = NewTask("l1"); + await new TaskRepository(_ctx).AddAsync(goodTask); + + Assert.Equal(2, goodTask.Number); + } + [Fact] public async Task GetByNumberAsync_finds_the_task_and_returns_null_for_unknown_numbers() { diff --git a/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs index 80d400f2..846d7955 100644 --- a/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs @@ -253,6 +253,52 @@ public sealed class TaskWaitMcpToolsTests : IDisposable Assert.True(sw.Elapsed >= TimeSpan.FromMilliseconds(900), $"took {sw.Elapsed}"); } + [Fact] + public async Task WaitForTaskChange_UnknownNumberHashForm_ReturnsImmediatelyAsNotFound() + { + var sut = BuildSut(); + var sw = Stopwatch.StartNew(); + + var result = await sut.WaitForTaskChange(["#999999"], timeoutSeconds: 30, cancellationToken: CancellationToken.None); + + sw.Stop(); + Assert.False(result.TimedOut); + Assert.Equal("NotFound", Assert.Single(result.Changed).Status); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(2), $"took {sw.Elapsed}"); + } + + [Fact] + public async Task WaitForTaskChange_UnknownBareNumber_ReturnsImmediatelyAsNotFound() + { + var sut = BuildSut(); + var sw = Stopwatch.StartNew(); + + var result = await sut.WaitForTaskChange(["999999"], timeoutSeconds: 30, cancellationToken: CancellationToken.None); + + sw.Stop(); + Assert.False(result.TimedOut); + Assert.Equal("NotFound", Assert.Single(result.Changed).Status); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(2), $"took {sw.Elapsed}"); + } + + [Fact] + public async Task WaitForTaskChange_MixOfValidAndUnknownNumber_ReportsBothCorrectly() + { + var task = await SeedTaskAsync(TaskStatus.WaitingForReview); + var sut = BuildSut(); + var sw = Stopwatch.StartNew(); + + var result = await sut.WaitForTaskChange( + [$"#{task.Number}", "#999999"], timeoutSeconds: 30, cancellationToken: CancellationToken.None); + + sw.Stop(); + Assert.False(result.TimedOut); + Assert.Equal(2, result.Changed.Count); + Assert.Contains(result.Changed, c => c.TaskId == task.Id && c.Status == "WaitingForReview"); + Assert.Contains(result.Changed, c => c.TaskId == "#999999" && c.Status == "NotFound"); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(2), $"took {sw.Elapsed}"); + } + [Fact] public void MaxTimeoutSeconds_StaysComfortablyUnderMcpToolTimeout() {