fix(worker,data): honor wait_for_task_change's NotFound contract, harden TaskNumberAllocator
wait_for_task_change resolved #<number>/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.
This commit is contained in:
@@ -1,4 +1,5 @@
|
|||||||
using ClaudeDo.Data.Models;
|
using ClaudeDo.Data.Models;
|
||||||
|
using Microsoft.Data.Sqlite;
|
||||||
using Microsoft.EntityFrameworkCore;
|
using Microsoft.EntityFrameworkCore;
|
||||||
|
|
||||||
namespace ClaudeDo.Data;
|
namespace ClaudeDo.Data;
|
||||||
@@ -29,6 +30,16 @@ public static class TaskNumberAllocator
|
|||||||
RETURNING *
|
RETURNING *
|
||||||
""", AppSettingsEntity.SingletonId).AsNoTracking().ToListAsync(ct);
|
""", 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;
|
entity.Number = settings[0].NextTaskNumber - 1;
|
||||||
context.Tasks.Add(entity);
|
context.Tasks.Add(entity);
|
||||||
|
|
||||||
@@ -37,10 +48,40 @@ public static class TaskNumberAllocator
|
|||||||
await context.SaveChangesAsync(ct);
|
await context.SaveChangesAsync(ct);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
catch (DbUpdateException) when (attempt < MaxAttempts)
|
catch (DbUpdateException ex)
|
||||||
{
|
{
|
||||||
context.Entry(entity).State = EntityState.Detached;
|
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);
|
||||||
}
|
}
|
||||||
|
|||||||
+13
-1
@@ -109,7 +109,19 @@ public sealed class TaskWaitMcpTools
|
|||||||
var tasks = new TaskRepository(ctx);
|
var tasks = new TaskRepository(ctx);
|
||||||
var resolved = new string[ids.Length];
|
var resolved = new string[ids.Length];
|
||||||
for (var i = 0; i < ids.Length; i++)
|
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 #<number>/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;
|
return resolved;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -131,6 +131,35 @@ public sealed class TaskNumberAllocatorTests : IDisposable
|
|||||||
Assert.All(numbers, n => Assert.True(n > 0));
|
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<DbUpdateException>(() => 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]
|
[Fact]
|
||||||
public async Task GetByNumberAsync_finds_the_task_and_returns_null_for_unknown_numbers()
|
public async Task GetByNumberAsync_finds_the_task_and_returns_null_for_unknown_numbers()
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -253,6 +253,52 @@ public sealed class TaskWaitMcpToolsTests : IDisposable
|
|||||||
Assert.True(sw.Elapsed >= TimeSpan.FromMilliseconds(900), $"took {sw.Elapsed}");
|
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]
|
[Fact]
|
||||||
public void MaxTimeoutSeconds_StaysComfortablyUnderMcpToolTimeout()
|
public void MaxTimeoutSeconds_StaysComfortablyUnderMcpToolTimeout()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user