mika kuns
b656a241a2
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.
88 lines
3.6 KiB
C#
88 lines
3.6 KiB
C#
using ClaudeDo.Data.Models;
|
|
using Microsoft.Data.Sqlite;
|
|
using Microsoft.EntityFrameworkCore;
|
|
|
|
namespace ClaudeDo.Data;
|
|
|
|
// Allocates TaskEntity.Number and inserts the task. Never MAX(number)+1 -- that would reissue a
|
|
// deleted task's number. Allocation is a single atomic UPDATE...RETURNING against the
|
|
// app_settings singleton row, which SQLite's single-writer model already serializes.
|
|
//
|
|
// The allocation and the insert are deliberately two separate statements, each committed on its
|
|
// own, rather than one shared transaction: a shared transaction would roll the counter bump back
|
|
// together with a failed insert, so a retry would hand out the exact same (still colliding)
|
|
// number every time and the bounded retry below would be pointless. Committing the bump
|
|
// unconditionally means a retry after a collision always advances to a fresh number, at the cost
|
|
// of burning the collided number as an unused gap -- which is fine, gaps are expected.
|
|
public static class TaskNumberAllocator
|
|
{
|
|
private const int MaxAttempts = 5;
|
|
|
|
public static async Task AddWithNumberAsync(
|
|
ClaudeDoDbContext context, TaskEntity entity, CancellationToken ct = default)
|
|
{
|
|
for (var attempt = 1; ; attempt++)
|
|
{
|
|
var settings = await context.AppSettings.FromSqlRaw(
|
|
"""
|
|
UPDATE app_settings SET next_task_number = next_task_number + 1
|
|
WHERE id = {0}
|
|
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);
|
|
|
|
try
|
|
{
|
|
await context.SaveChangesAsync(ct);
|
|
return;
|
|
}
|
|
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);
|
|
}
|