fix(worker): route done-toggle and dequeue through guarded TaskStateService transitions
The task-list done toggle (both islands) and RemoveFromQueue wrote TaskEntity.Status directly via EF, bypassing TaskStateService: no TaskUpdated broadcast, no guard against a concurrent picker claim (lost update), and no status-based filter. Added guarded MarkDoneAsync/UnmarkDoneAsync/DequeueToIdleAsync transitions plus matching hub methods (SetTaskDone/UnsetTaskDone/DequeueTask) and IWorkerClient wrappers; the three UI call sites now route through the hub with optimistic-then-revert row updates and ErrorReported on failure. RemoveFromQueueAsync dequeues each queued child individually through the same guarded path instead of cascading via a raw EF update. Also closes two hub guard gaps: UpdateListConfig's delete branch now preserves a list's SerializeOnFileOverlap flag instead of dropping it, and SubmitTaskForReview's Idle/Failed status gate now runs before either mutation branch so a Done/Cancelled task can't get committed or stamped and then rejected.
This commit is contained in:
mika kuns
@@ -0,0 +1,101 @@
|
||||
using ClaudeDo.Data;
|
||||
using ClaudeDo.Data.Models;
|
||||
using ClaudeDo.Data.Repositories;
|
||||
using ClaudeDo.Worker.Hub;
|
||||
using ClaudeDo.Worker.Tests.Infrastructure;
|
||||
using Xunit;
|
||||
|
||||
namespace ClaudeDo.Worker.Tests.Hub;
|
||||
|
||||
/// UpdateListConfig's "all fields blank -> delete the row" branch used to delete unconditionally,
|
||||
/// silently dropping SerializeOnFileOverlap -- a flag with no UI/hub affordance of its own (set
|
||||
/// only via set_list_config or directly against ListConfigEntity).
|
||||
public sealed class ListConfigHubTests : IDisposable
|
||||
{
|
||||
private readonly DbFixture _db = new();
|
||||
|
||||
public void Dispose() => _db.Dispose();
|
||||
|
||||
private WorkerHub CreateHub()
|
||||
{
|
||||
var factory = _db.CreateFactory();
|
||||
var broadcaster = new HubBroadcaster(new CapturingHubContext());
|
||||
var hub = new WorkerHub(
|
||||
null!, null!, null!, null!, broadcaster, factory,
|
||||
null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, null!,
|
||||
null!, new ClaudeDo.Worker.Online.OnlineInboxConfig(), new ClaudeDo.Worker.Online.OnlineTokenStore(),
|
||||
new ClaudeDo.Worker.Runner.PendingQuestionRegistry(), null!);
|
||||
hub.Clients = new FakeHubCallerClients(new RecordingClientProxy());
|
||||
hub.Context = new FakeHubCallerContext();
|
||||
return hub;
|
||||
}
|
||||
|
||||
// Each helper opens (and disposes) its own short-lived context/repository -- ListRepository's
|
||||
// GetConfigAsync doesn't AsNoTracking(), so reusing one long-lived instance across a hub call
|
||||
// that writes via a *different* context would return a stale, identity-mapped entity.
|
||||
|
||||
private async Task<string> SeedListAsync()
|
||||
{
|
||||
var listId = Guid.NewGuid().ToString();
|
||||
await using var ctx = _db.CreateContext();
|
||||
await new ListRepository(ctx).AddAsync(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow });
|
||||
return listId;
|
||||
}
|
||||
|
||||
private async Task SeedConfigAsync(string listId, string? model = null, bool serializeOnFileOverlap = false)
|
||||
{
|
||||
await using var ctx = _db.CreateContext();
|
||||
await new ListRepository(ctx).SetConfigAsync(new ListConfigEntity
|
||||
{
|
||||
ListId = listId, Model = model, SerializeOnFileOverlap = serializeOnFileOverlap,
|
||||
});
|
||||
}
|
||||
|
||||
private async Task<ListConfigEntity?> GetConfigAsync(string listId)
|
||||
{
|
||||
await using var ctx = _db.CreateContext();
|
||||
return await new ListRepository(ctx).GetConfigAsync(listId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task UpdateListConfig_AllBlank_NoUnrelatedSettings_DeletesRow()
|
||||
{
|
||||
var hub = CreateHub();
|
||||
var listId = await SeedListAsync();
|
||||
await SeedConfigAsync(listId, model: "opus");
|
||||
|
||||
await hub.UpdateListConfig(new UpdateListConfigDto(listId, null, null, null));
|
||||
|
||||
Assert.Null(await GetConfigAsync(listId));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task UpdateListConfig_AllBlank_WithSerializeOnFileOverlap_KeepsFlag_RowSurvives()
|
||||
{
|
||||
var hub = CreateHub();
|
||||
var listId = await SeedListAsync();
|
||||
await SeedConfigAsync(listId, model: "opus", serializeOnFileOverlap: true);
|
||||
|
||||
await hub.UpdateListConfig(new UpdateListConfigDto(listId, null, null, null));
|
||||
|
||||
var config = await GetConfigAsync(listId);
|
||||
Assert.NotNull(config);
|
||||
Assert.True(config!.SerializeOnFileOverlap);
|
||||
Assert.Null(config.Model);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task UpdateListConfig_WithModel_UpsertsNormally_PreservesSerializeOnFileOverlap()
|
||||
{
|
||||
var hub = CreateHub();
|
||||
var listId = await SeedListAsync();
|
||||
await SeedConfigAsync(listId, serializeOnFileOverlap: true);
|
||||
|
||||
await hub.UpdateListConfig(new UpdateListConfigDto(listId, "opus", null, null));
|
||||
|
||||
var config = await GetConfigAsync(listId);
|
||||
Assert.NotNull(config);
|
||||
Assert.Equal("opus", config!.Model);
|
||||
Assert.True(config.SerializeOnFileOverlap);
|
||||
}
|
||||
}
|
||||
@@ -214,6 +214,51 @@ public sealed class MergeHelperTaskHubTests : IDisposable
|
||||
var hub = CreateHub();
|
||||
await Assert.ThrowsAsync<HubException>(() => hub.SubmitTaskForReview(task.Id));
|
||||
}
|
||||
|
||||
// The status gate (Idle or Failed) must run before either mutation branch below, so a
|
||||
// Done/Cancelled task can never get its worktree committed or its HandlerHeadCommit stamped
|
||||
// and then rejected by SubmitInteractiveForReviewAsync, stranding the work.
|
||||
|
||||
[Fact]
|
||||
public async Task SubmitTaskForReview_DoneTask_WorktreePath_ThrowsBeforeCommit_WorktreeUntouched()
|
||||
{
|
||||
var listId = await SeedListAsync(Path.GetTempPath());
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Done);
|
||||
var worktree = new WorktreeEntity
|
||||
{
|
||||
TaskId = task.Id,
|
||||
Path = Path.Combine(Path.GetTempPath(), $"cd_no_such_worktree_{Guid.NewGuid():N}"),
|
||||
BranchName = "claudedo/does-not-matter",
|
||||
BaseCommit = "abc123",
|
||||
State = WorktreeState.Active,
|
||||
CreatedAt = DateTime.UtcNow,
|
||||
};
|
||||
await new WorktreeRepository(_ctx).AddAsync(worktree);
|
||||
|
||||
var hub = CreateHub();
|
||||
var ex = await Assert.ThrowsAsync<HubException>(() => hub.SubmitTaskForReview(task.Id));
|
||||
Assert.Contains("Idle or Failed", ex.Message);
|
||||
|
||||
var reloadedWorktree = await new WorktreeRepository(_ctx).GetByTaskIdAsync(task.Id);
|
||||
Assert.Null(reloadedWorktree!.HeadCommit);
|
||||
var reloadedTask = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Equal(TaskStatus.Done, reloadedTask!.Status);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubmitTaskForReview_CancelledTask_HandlerPath_ThrowsBeforeStamp_HandlerHeadCommitUntouched()
|
||||
{
|
||||
var listId = await SeedListAsync(Path.GetTempPath());
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Cancelled, handlerBaseCommit: "abc123");
|
||||
|
||||
var hub = CreateHub();
|
||||
var ex = await Assert.ThrowsAsync<HubException>(() => hub.SubmitTaskForReview(task.Id));
|
||||
Assert.Contains("Idle or Failed", ex.Message);
|
||||
|
||||
var reloaded = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Null(reloaded!.HandlerHeadCommit);
|
||||
Assert.Equal(TaskStatus.Cancelled, reloaded.Status);
|
||||
}
|
||||
}
|
||||
|
||||
// RecordingClientProxy / FakeHubCallerClients / FakeHubCallerContext are defined once for the
|
||||
|
||||
@@ -0,0 +1,160 @@
|
||||
using ClaudeDo.Data;
|
||||
using ClaudeDo.Data.Models;
|
||||
using ClaudeDo.Data.Repositories;
|
||||
using ClaudeDo.Worker.Hub;
|
||||
using ClaudeDo.Worker.Tests.Infrastructure;
|
||||
using Microsoft.AspNetCore.SignalR;
|
||||
using Xunit;
|
||||
using TaskStatus = ClaudeDo.Data.Models.TaskStatus;
|
||||
|
||||
namespace ClaudeDo.Worker.Tests.Hub;
|
||||
|
||||
/// Covers the guarded "manual done toggle" (SetTaskDone/UnsetTaskDone) and "remove from queue"
|
||||
/// (DequeueTask) hub methods added to replace the UI's raw EF writes -- each checks the expected
|
||||
/// starting status server-side so a concurrent picker claim can't be silently overwritten.
|
||||
public sealed class TaskDoneDequeueHubTests : IDisposable
|
||||
{
|
||||
private readonly DbFixture _db = new();
|
||||
private readonly ClaudeDoDbContext _ctx;
|
||||
private readonly TaskRepository _tasks;
|
||||
private readonly ListRepository _lists;
|
||||
private readonly RecordingClientProxy _proxy = new();
|
||||
|
||||
public TaskDoneDequeueHubTests()
|
||||
{
|
||||
_ctx = _db.CreateContext();
|
||||
_tasks = new TaskRepository(_ctx);
|
||||
_lists = new ListRepository(_ctx);
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
_ctx.Dispose();
|
||||
_db.Dispose();
|
||||
}
|
||||
|
||||
private WorkerHub CreateHub()
|
||||
{
|
||||
var factory = _db.CreateFactory();
|
||||
var built = TaskStateServiceBuilder.Build(factory);
|
||||
var hub = new WorkerHub(
|
||||
null!, null!, null!, null!, null!, factory, null!, null!, null!,
|
||||
null!, null!, null!, null!, null!, null!, null!, built.State, null!, null!,
|
||||
null!, new ClaudeDo.Worker.Online.OnlineInboxConfig(), new ClaudeDo.Worker.Online.OnlineTokenStore(),
|
||||
new ClaudeDo.Worker.Runner.PendingQuestionRegistry(), null!);
|
||||
hub.Clients = new FakeHubCallerClients(_proxy);
|
||||
hub.Context = new FakeHubCallerContext();
|
||||
return hub;
|
||||
}
|
||||
|
||||
private async Task<string> SeedListAsync()
|
||||
{
|
||||
var listId = Guid.NewGuid().ToString();
|
||||
await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow });
|
||||
return listId;
|
||||
}
|
||||
|
||||
private async Task<TaskEntity> SeedTaskAsync(
|
||||
string listId, TaskStatus status, string? blockedByTaskId = null, string title = "T")
|
||||
{
|
||||
var task = new TaskEntity
|
||||
{
|
||||
Id = Guid.NewGuid().ToString(),
|
||||
ListId = listId,
|
||||
Title = title,
|
||||
Status = status,
|
||||
BlockedByTaskId = blockedByTaskId,
|
||||
CreatedAt = DateTime.UtcNow,
|
||||
};
|
||||
await _tasks.AddAsync(task);
|
||||
return task;
|
||||
}
|
||||
|
||||
// ── SetTaskDone ──
|
||||
|
||||
[Fact]
|
||||
public async Task SetTaskDone_FromIdle_TransitionsToDone()
|
||||
{
|
||||
var listId = await SeedListAsync();
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Idle);
|
||||
|
||||
var hub = CreateHub();
|
||||
await hub.SetTaskDone(task.Id);
|
||||
|
||||
var reloaded = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Equal(TaskStatus.Done, reloaded!.Status);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SetTaskDone_FromRunning_Throws_AndLeavesStatusUnchanged()
|
||||
{
|
||||
var listId = await SeedListAsync();
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Running);
|
||||
|
||||
var hub = CreateHub();
|
||||
await Assert.ThrowsAsync<HubException>(() => hub.SetTaskDone(task.Id));
|
||||
|
||||
var reloaded = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Equal(TaskStatus.Running, reloaded!.Status);
|
||||
}
|
||||
|
||||
// ── UnsetTaskDone ──
|
||||
|
||||
[Fact]
|
||||
public async Task UnsetTaskDone_FromDone_TransitionsToIdle()
|
||||
{
|
||||
var listId = await SeedListAsync();
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Done);
|
||||
|
||||
var hub = CreateHub();
|
||||
await hub.UnsetTaskDone(task.Id);
|
||||
|
||||
var reloaded = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Equal(TaskStatus.Idle, reloaded!.Status);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task UnsetTaskDone_FromRunning_Throws_AndLeavesStatusUnchanged()
|
||||
{
|
||||
var listId = await SeedListAsync();
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Running);
|
||||
|
||||
var hub = CreateHub();
|
||||
await Assert.ThrowsAsync<HubException>(() => hub.UnsetTaskDone(task.Id));
|
||||
|
||||
var reloaded = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Equal(TaskStatus.Running, reloaded!.Status);
|
||||
}
|
||||
|
||||
// ── DequeueTask ──
|
||||
|
||||
[Fact]
|
||||
public async Task DequeueTask_FromQueued_TransitionsToIdle_AndClearsBlockedByTaskId()
|
||||
{
|
||||
var listId = await SeedListAsync();
|
||||
var pred = await SeedTaskAsync(listId, TaskStatus.Queued);
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Queued, blockedByTaskId: pred.Id);
|
||||
|
||||
var hub = CreateHub();
|
||||
await hub.DequeueTask(task.Id);
|
||||
|
||||
var reloaded = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Equal(TaskStatus.Idle, reloaded!.Status);
|
||||
Assert.Null(reloaded.BlockedByTaskId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task DequeueTask_FromRunning_Throws_AndLeavesStatusUnchanged()
|
||||
{
|
||||
// Simulates the picker having already claimed the task between the UI reading its
|
||||
// row and the dequeue call landing -- the DB status must not be clobbered.
|
||||
var listId = await SeedListAsync();
|
||||
var task = await SeedTaskAsync(listId, TaskStatus.Running);
|
||||
|
||||
var hub = CreateHub();
|
||||
await Assert.ThrowsAsync<HubException>(() => hub.DequeueTask(task.Id));
|
||||
|
||||
var reloaded = await _tasks.GetByIdAsync(task.Id);
|
||||
Assert.Equal(TaskStatus.Running, reloaded!.Status);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user