fix(worker,ui): block cancelling a task while its unit merge is draining
TaskStateService.CancelAsync allowed cancelling a WaitingForReview task even while PlanningMergeOrchestrator was mid-drain on it: ApproveReview awaits the whole multi-subtask merge synchronously, so a concurrent CancelReview (UI or MCP) could flip the parent to Cancelled while the orchestrator kept merging children onto the target branch, then FinalizeParentDoneAsync would find the parent no longer WaitingForReview and give up - leaving the merged diffs stranded with no rollback. CancelAsync now rejects with a clear reason when HasActiveMerge(taskId) is true. TaskStateService can't take PlanningMergeOrchestrator as a direct constructor dependency (circular back to ITaskStateService), so it takes a lazily-resolved Func<IActiveMergeState> instead, mirroring the existing Func<ITaskStateService> cycle-break already used for PlanningChainCoordinator. UI polish: DetailsIslandViewModel.IsMergeDraining gates CancelReviewCommand's CanExecute (same shape as WorktreesOverviewModalViewModel.IsMerging), and the command's catch now raises ErrorReported instead of swallowing the rejection silently.
This commit is contained in:
@@ -55,6 +55,7 @@ public abstract class StubWorkerClient : IWorkerClient
|
||||
public void RaiseHandoffRequested(string taskId, IReadOnlyList<string> survivingTaskIds) => HandoffRequestedEvent?.Invoke(taskId, survivingTaskIds);
|
||||
public void RaisePlanningMergeConflict(string planningTaskId, string subtaskId, IReadOnlyList<string> files, bool externallyDriven)
|
||||
=> PlanningMergeConflictEvent?.Invoke(planningTaskId, subtaskId, files, externallyDriven);
|
||||
public void RaisePlanningMergeStarted(string planningTaskId, string targetBranch) => PlanningMergeStartedEvent?.Invoke(planningTaskId, targetBranch);
|
||||
public void RaisePlanningMergeAborted(string planningTaskId) => PlanningMergeAbortedEvent?.Invoke(planningTaskId);
|
||||
public void RaisePlanningCompleted(string planningTaskId) => PlanningCompletedEvent?.Invoke(planningTaskId);
|
||||
|
||||
|
||||
@@ -161,4 +161,69 @@ public class DetailsIslandReviewActionsTests : IDisposable
|
||||
|
||||
Assert.Equal(worker.ExceptionMessage, reportedError);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void CancelReview_IsDisabled_WhileUnitMergeIsDraining()
|
||||
{
|
||||
var worker = new RecordingWorkerClient();
|
||||
var vm = BuildVm(worker);
|
||||
vm.Bind(new TaskRowViewModel { Id = "task-drain-1", Status = TaskStatus.WaitingForReview });
|
||||
|
||||
Assert.True(vm.CancelReviewCommand.CanExecute(null));
|
||||
|
||||
worker.RaisePlanningMergeStarted("task-drain-1", "main");
|
||||
Assert.False(vm.CancelReviewCommand.CanExecute(null));
|
||||
|
||||
worker.RaisePlanningCompleted("task-drain-1");
|
||||
Assert.True(vm.CancelReviewCommand.CanExecute(null));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void CancelReview_ReDisables_UntilMergeAborted()
|
||||
{
|
||||
var worker = new RecordingWorkerClient();
|
||||
var vm = BuildVm(worker);
|
||||
vm.Bind(new TaskRowViewModel { Id = "task-drain-2", Status = TaskStatus.WaitingForReview });
|
||||
|
||||
worker.RaisePlanningMergeStarted("task-drain-2", "main");
|
||||
Assert.False(vm.CancelReviewCommand.CanExecute(null));
|
||||
|
||||
worker.RaisePlanningMergeAborted("task-drain-2");
|
||||
Assert.True(vm.CancelReviewCommand.CanExecute(null));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void CancelReview_UnaffectedByMergeEvents_ForADifferentTask()
|
||||
{
|
||||
var worker = new RecordingWorkerClient();
|
||||
var vm = BuildVm(worker);
|
||||
vm.Bind(new TaskRowViewModel { Id = "task-drain-3", Status = TaskStatus.WaitingForReview });
|
||||
|
||||
worker.RaisePlanningMergeStarted("some-other-parent", "main");
|
||||
|
||||
Assert.True(vm.CancelReviewCommand.CanExecute(null));
|
||||
}
|
||||
|
||||
private sealed class ThrowingCancelWorkerClient : StubWorkerClient
|
||||
{
|
||||
public override bool IsConnected => true;
|
||||
public string ExceptionMessage { get; init; } =
|
||||
"A merge is in progress for this task; wait for it to finish before cancelling.";
|
||||
public override Task CancelReviewAsync(string taskId) => throw new Exception(ExceptionMessage);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task CancelReview_WhenWorkerThrows_RaisesErrorReported()
|
||||
{
|
||||
var worker = new ThrowingCancelWorkerClient();
|
||||
var vm = BuildVm(worker);
|
||||
vm.Bind(new TaskRowViewModel { Id = "task-cancel-err-1", Status = TaskStatus.WaitingForReview });
|
||||
|
||||
string? reportedError = null;
|
||||
vm.ErrorReported += msg => reportedError = msg;
|
||||
|
||||
await vm.CancelReviewCommand.ExecuteAsync(null);
|
||||
|
||||
Assert.Equal(worker.ExceptionMessage, reportedError);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -20,7 +20,8 @@ public static class TaskStateServiceBuilder
|
||||
CountingQueueWaker Waker,
|
||||
RunCancellationRegistry RunCancels);
|
||||
|
||||
public static Built Build(IDbContextFactory<ClaudeDoDbContext> dbFactory)
|
||||
public static Built Build(
|
||||
IDbContextFactory<ClaudeDoDbContext> dbFactory, Func<IActiveMergeState>? mergeState = null)
|
||||
{
|
||||
var hub = new CapturingHubContext();
|
||||
var broadcaster = new HubBroadcaster(hub);
|
||||
@@ -35,12 +36,19 @@ public static class TaskStateServiceBuilder
|
||||
waker,
|
||||
chain,
|
||||
runCancels,
|
||||
mergeState ?? (() => NoActiveMergeState.Instance),
|
||||
NullLogger<TaskStateService>.Instance);
|
||||
|
||||
return new Built(state, chain, hub, () => waker.Count, waker, runCancels);
|
||||
}
|
||||
}
|
||||
|
||||
file sealed class NoActiveMergeState : IActiveMergeState
|
||||
{
|
||||
public static readonly NoActiveMergeState Instance = new();
|
||||
public bool HasActiveMerge(string taskId) => false;
|
||||
}
|
||||
|
||||
public sealed class CountingQueueWaker : IQueueWaker
|
||||
{
|
||||
private int _count;
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
using ClaudeDo.Data;
|
||||
using ClaudeDo.Data.Models;
|
||||
using ClaudeDo.Data.Repositories;
|
||||
using ClaudeDo.Worker.Planning;
|
||||
using ClaudeDo.Worker.State;
|
||||
using ClaudeDo.Worker.Tests.Infrastructure;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
@@ -8,6 +9,12 @@ using TaskStatus = ClaudeDo.Data.Models.TaskStatus;
|
||||
|
||||
namespace ClaudeDo.Worker.Tests.State;
|
||||
|
||||
file sealed class FakeActiveMergeState : IActiveMergeState
|
||||
{
|
||||
public HashSet<string> ActiveTaskIds { get; } = new();
|
||||
public bool HasActiveMerge(string taskId) => ActiveTaskIds.Contains(taskId);
|
||||
}
|
||||
|
||||
public sealed class TaskStateServiceTests : IDisposable
|
||||
{
|
||||
private readonly DbFixture _db = new();
|
||||
@@ -319,6 +326,34 @@ public sealed class TaskStateServiceTests : IDisposable
|
||||
Assert.Equal(TaskStatus.Done, await GetStatusAsync(id));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task CancelAsync_WhileUnitMergeDraining_Rejects_AndDoesNotMutate()
|
||||
{
|
||||
var mergeState = new FakeActiveMergeState();
|
||||
var built = TaskStateServiceBuilder.Build(_factory, () => mergeState);
|
||||
var id = await SeedTaskAsync(TaskStatus.WaitingForReview);
|
||||
mergeState.ActiveTaskIds.Add(id);
|
||||
|
||||
var result = await built.State.CancelAsync(id, DateTime.UtcNow, default);
|
||||
|
||||
Assert.False(result.Ok);
|
||||
Assert.Equal(TaskStatus.WaitingForReview, await GetStatusAsync(id));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task CancelAsync_AfterUnitMergeDrainCompletes_Succeeds()
|
||||
{
|
||||
var mergeState = new FakeActiveMergeState();
|
||||
var built = TaskStateServiceBuilder.Build(_factory, () => mergeState);
|
||||
var id = await SeedTaskAsync(TaskStatus.WaitingForReview);
|
||||
// Merge drain finished (or never started) for this task — cancel behaves as before.
|
||||
|
||||
var result = await built.State.CancelAsync(id, DateTime.UtcNow, default);
|
||||
|
||||
Assert.True(result.Ok);
|
||||
Assert.Equal(TaskStatus.Cancelled, await GetStatusAsync(id));
|
||||
}
|
||||
|
||||
// ─── ResetToIdleAsync ─────────────────────────────────────────────────
|
||||
|
||||
[Fact]
|
||||
|
||||
Reference in New Issue
Block a user