fix(worker): claim Running before creating run resources
TaskRunner.RunAsync created the worktree (PrepareRunDirectoryAsync) before claiming Running via StartRunningAsync. RunNow dispatches by task id with no atomic claim of their own, so a Queued task racing the queue picker's atomic SQL claim could hit WorktreeManager's branch-collision self-heal, which force-removes and recreates the winner's live worktree mid-run. Move the claim before any resource creation and bail out immediately when it's rejected. OverrideSlotService.RunNow also fast-rejects a task already Running in the DB (defense in depth). RunCancellationRegistry now refuses (and logs) a double registration instead of silently overwriting the first CTS, so a losing dispatch's cleanup can no longer unregister the winner's cancellation token.
This commit is contained in:
@@ -0,0 +1,89 @@
|
||||
using ClaudeDo.Data;
|
||||
using ClaudeDo.Data.Models;
|
||||
using ClaudeDo.Data.Repositories;
|
||||
using ClaudeDo.Worker.Config;
|
||||
using ClaudeDo.Worker.Hub;
|
||||
using ClaudeDo.Worker.Queue;
|
||||
using ClaudeDo.Worker.Runner;
|
||||
using ClaudeDo.Worker.Tests.Infrastructure;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using TaskStatus = ClaudeDo.Data.Models.TaskStatus;
|
||||
|
||||
namespace ClaudeDo.Worker.Tests.Queue;
|
||||
|
||||
/// OverrideSlotService.RunNow's own precheck — distinct from QueueService.EnsureNotInQueueSlot,
|
||||
/// which only guards a task already tracked in its in-memory queue-slot dict. This covers a
|
||||
/// task the DB already shows Running (e.g. the queue picker's atomic claim landed) regardless
|
||||
/// of local slot bookkeeping. The real race-safety net is TaskRunner.RunAsync claiming Running
|
||||
/// before creating any resources; this precheck just fails fast for the common case.
|
||||
public sealed class OverrideSlotServiceTests : IDisposable
|
||||
{
|
||||
private readonly DbFixture _db = new();
|
||||
private readonly string _tempDir;
|
||||
private readonly WorkerConfig _cfg;
|
||||
|
||||
public OverrideSlotServiceTests()
|
||||
{
|
||||
_tempDir = Path.Combine(Path.GetTempPath(), $"cd_override_{Guid.NewGuid():N}");
|
||||
Directory.CreateDirectory(_tempDir);
|
||||
_cfg = new WorkerConfig { SandboxRoot = _tempDir, LogRoot = _tempDir };
|
||||
}
|
||||
|
||||
public void Dispose() { _db.Dispose(); try { Directory.Delete(_tempDir, true); } catch { } }
|
||||
|
||||
private OverrideSlotService BuildService()
|
||||
{
|
||||
var dbFactory = _db.CreateFactory();
|
||||
var state = TaskStateServiceBuilder.Build(dbFactory).State;
|
||||
var wt = new WorktreeManager(new ClaudeDo.Data.Git.GitService(), dbFactory, _cfg, NullLogger<WorktreeManager>.Instance);
|
||||
var runner = new TaskRunner(new FakeClaudeProcess(), dbFactory, new HubBroadcaster(new CapturingHubContext()), wt,
|
||||
new ClaudeArgsBuilder(), _cfg, NullLogger<TaskRunner>.Instance, state, new TaskRunTokenRegistry(),
|
||||
new AttachmentStore(), new FakeSessionSkillSeeder(), new FakeTranscriptUsageReader());
|
||||
var runCancels = new RunCancellationRegistry(NullLogger<RunCancellationRegistry>.Instance);
|
||||
return new OverrideSlotService(dbFactory, runner, NullLogger<OverrideSlotService>.Instance, runCancels);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task RunNow_TaskAlreadyRunningInDb_ThrowsWithoutDispatching()
|
||||
{
|
||||
string listId = Guid.NewGuid().ToString(), taskId = Guid.NewGuid().ToString();
|
||||
using (var ctx = _db.CreateContext())
|
||||
{
|
||||
ctx.Lists.Add(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow });
|
||||
ctx.Tasks.Add(new TaskEntity
|
||||
{
|
||||
Id = taskId, ListId = listId, Title = "T", Status = TaskStatus.Running,
|
||||
StartedAt = DateTime.UtcNow, CreatedAt = DateTime.UtcNow,
|
||||
});
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var service = BuildService();
|
||||
|
||||
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => service.RunNow(taskId));
|
||||
Assert.Contains("already running", ex.Message);
|
||||
Assert.Null(service.CurrentSlot);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task RunNow_TaskQueued_DoesNotThrow()
|
||||
{
|
||||
string listId = Guid.NewGuid().ToString(), taskId = Guid.NewGuid().ToString();
|
||||
using (var ctx = _db.CreateContext())
|
||||
{
|
||||
ctx.Lists.Add(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow });
|
||||
ctx.Tasks.Add(new TaskEntity
|
||||
{
|
||||
Id = taskId, ListId = listId, Title = "T", Status = TaskStatus.Queued,
|
||||
CreatedAt = DateTime.UtcNow,
|
||||
});
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var service = BuildService();
|
||||
|
||||
// The dispatch itself races with FakeClaudeProcess's immediate completion in the
|
||||
// background, so this only asserts the precheck doesn't reject a claimable task.
|
||||
await service.RunNow(taskId);
|
||||
}
|
||||
}
|
||||
@@ -1,4 +1,5 @@
|
||||
using ClaudeDo.Worker.Queue;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
|
||||
namespace ClaudeDo.Worker.Tests.Queue;
|
||||
|
||||
@@ -7,9 +8,9 @@ public sealed class RunCancellationRegistryTests
|
||||
[Fact]
|
||||
public void TryCancel_RegisteredTask_CancelsAndReturnsTrue()
|
||||
{
|
||||
var sut = new RunCancellationRegistry();
|
||||
var sut = new RunCancellationRegistry(NullLogger<RunCancellationRegistry>.Instance);
|
||||
using var cts = new CancellationTokenSource();
|
||||
sut.Register("t1", cts);
|
||||
Assert.True(sut.Register("t1", cts));
|
||||
|
||||
Assert.True(sut.TryCancel("t1"));
|
||||
Assert.True(cts.IsCancellationRequested);
|
||||
@@ -18,30 +19,50 @@ public sealed class RunCancellationRegistryTests
|
||||
[Fact]
|
||||
public void TryCancel_UnknownTask_ReturnsFalse()
|
||||
{
|
||||
var sut = new RunCancellationRegistry();
|
||||
var sut = new RunCancellationRegistry(NullLogger<RunCancellationRegistry>.Instance);
|
||||
Assert.False(sut.TryCancel("nope"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Register_SecondCallForSameTask_RefusesAndKeepsFirstRegistration()
|
||||
{
|
||||
// A double-dispatch (e.g. RunNow racing the queue picker for the same task) must not
|
||||
// let the second registration silently clobber the first — that would leave CancelAsync
|
||||
// pointing at the wrong (or already-finished) CTS.
|
||||
var sut = new RunCancellationRegistry(NullLogger<RunCancellationRegistry>.Instance);
|
||||
using var first = new CancellationTokenSource();
|
||||
using var second = new CancellationTokenSource();
|
||||
|
||||
Assert.True(sut.Register("t1", first));
|
||||
Assert.False(sut.Register("t1", second));
|
||||
|
||||
Assert.True(sut.TryCancel("t1"));
|
||||
Assert.True(first.IsCancellationRequested);
|
||||
Assert.False(second.IsCancellationRequested);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Unregister_RemovesOnlyTheGivenRegistration()
|
||||
{
|
||||
var sut = new RunCancellationRegistry();
|
||||
using var stale = new CancellationTokenSource();
|
||||
using var current = new CancellationTokenSource();
|
||||
var sut = new RunCancellationRegistry(NullLogger<RunCancellationRegistry>.Instance);
|
||||
using var first = new CancellationTokenSource();
|
||||
|
||||
sut.Register("t1", stale);
|
||||
sut.Register("t1", current); // re-run replaced the registration
|
||||
sut.Unregister("t1", stale); // late cleanup of the old slot must not evict the new one
|
||||
sut.Register("t1", first);
|
||||
sut.Unregister("t1", first);
|
||||
|
||||
// Slot is free again once the run that held it cleans up properly, so a genuine
|
||||
// sequential re-run can register a fresh CTS under the same task id.
|
||||
using var second = new CancellationTokenSource();
|
||||
Assert.True(sut.Register("t1", second));
|
||||
|
||||
Assert.True(sut.TryCancel("t1"));
|
||||
Assert.True(current.IsCancellationRequested);
|
||||
Assert.False(stale.IsCancellationRequested);
|
||||
Assert.True(second.IsCancellationRequested);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryCancel_DisposedCts_ReturnsFalse()
|
||||
{
|
||||
var sut = new RunCancellationRegistry();
|
||||
var sut = new RunCancellationRegistry(NullLogger<RunCancellationRegistry>.Instance);
|
||||
var cts = new CancellationTokenSource();
|
||||
sut.Register("t1", cts);
|
||||
cts.Dispose();
|
||||
|
||||
Reference in New Issue
Block a user