From 79f90a9a8e37ea5f02bd614ef1f09da275759185 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 13:38:02 +0200 Subject: [PATCH] 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 instead, mirroring the existing Func 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. --- docs/explore-notes/review-merge.md | 27 +++++++- .../Islands/DetailsIslandViewModel.cs | 45 ++++++++++++- .../Planning/Interfaces/IActiveMergeState.cs | 10 +++ .../Planning/PlanningMergeOrchestrator.cs | 2 +- src/ClaudeDo.Worker/Program.cs | 4 ++ src/ClaudeDo.Worker/State/TaskStateService.cs | 11 ++++ tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs | 1 + .../DetailsIslandReviewActionsTests.cs | 65 +++++++++++++++++++ .../Infrastructure/TaskStateServiceBuilder.cs | 10 ++- .../State/TaskStateServiceTests.cs | 35 ++++++++++ 10 files changed, 205 insertions(+), 5 deletions(-) create mode 100644 src/ClaudeDo.Worker/Planning/Interfaces/IActiveMergeState.cs diff --git a/docs/explore-notes/review-merge.md b/docs/explore-notes/review-merge.md index cc4e288a..b01365f9 100644 --- a/docs/explore-notes/review-merge.md +++ b/docs/explore-notes/review-merge.md @@ -1,7 +1,7 @@ # Review, merge & conflict resolution > **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative. -> Last verified against commit `20bce9b` (2026-08-06). +> Last verified against commit `0d1e3b9` (2026-08-06). > Drift check: `git log --oneline 20bce9b..HEAD -- src/ClaudeDo.Worker/Lifecycle src/ClaudeDo.Worker/State src/ClaudeDo.Worker/Planning src/ClaudeDo.Ui/ViewModels/Conflicts src/ClaudeDo.Worker/External` > Stable structure only (no line numbers). See docs/explore-notes/README.md. @@ -56,6 +56,31 @@ like any other `Done` child once the parent is approved. The MCP surface has no An empty branch is still mergeable by design (some tasks — e.g. an audit — legitimately produce no diff); this is a visibility fix, not a merge gate. +## Cancel is blocked while a unit merge is draining + +`ApproveReview` on a task with children awaits `PlanningMergeOrchestrator.StartAsync` / +`DrainAsync` synchronously — the parent sits in `WaitingForReview` for the whole (potentially +minutes-long, one-child-at-a-time) drain. Without a guard, a concurrent `CancelReview` (UI or +`update_task_status`/`cancel_task` via MCP) could flip the parent to `Cancelled` mid-drain while +the orchestrator kept merging children's worktrees onto the target branch; `FinalizeParentDoneAsync` +then finds the parent no longer `WaitingForReview` and gives up, leaving the merged children's +diffs stranded with no rollback. + +`TaskStateService.CancelAsync` now rejects with a `TransitionResult` reason whenever +`PlanningMergeOrchestrator.HasActiveMerge(taskId)` is true for the task being cancelled, before any +DB write. Cycle note: `TaskStateService` can't take a direct constructor dependency on +`PlanningMergeOrchestrator` (which itself depends on `ITaskStateService`), so it takes a lazily-resolved +`Func` instead — same cycle-breaking shape as the existing `Func` +handed to `PlanningChainCoordinator`. `IActiveMergeState` (`Planning/Interfaces/`) is `PlanningMergeOrchestrator`'s +only public surface `TaskStateService` needs. + +The UI mirrors this as polish: `DetailsIslandViewModel.IsMergeDraining` (set on +`PlanningMergeStartedEvent`, cleared on `PlanningMergeAborted`/`PlanningCompleted` for the bound +task) gates `CancelReviewCommand`'s `CanExecute` — same shape as `WorktreesOverviewModalViewModel.IsMerging` +gating `CanMergeAll`. The worker-side guard remains the actual correctness fix; the button gate +just avoids inviting a click the worker would reject. `CancelReviewAsync`'s catch now raises +`ErrorReported` (→ shell `FlashFooterError`) instead of swallowing the rejection silently. + ## Post-merge verify gate A list can set `ListConfigEntity.VerifyCommand` (List Settings modal → Verification). diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 0a1ca8a2..907e61db 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -35,6 +35,9 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable private readonly Action _workerTaskFinishedHandler; private readonly Action _workerWorktreeUpdatedHandler; private readonly Action _workerTaskUpdatedHandler; + private readonly Action _workerPlanningMergeStartedHandler; + private readonly Action _workerPlanningMergeAbortedHandler; + private readonly Action _workerPlanningCompletedHandler; [ObservableProperty] private bool _isNotesMode; [ObservableProperty] private bool _isPrepMode; @@ -351,6 +354,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable ResetAndRetryCommand.NotifyCanExecuteChanged(); ContinueCommand.NotifyCanExecuteChanged(); SendRoadblockReplyCommand.NotifyCanExecuteChanged(); + CancelReviewCommand.NotifyCanExecuteChanged(); } }; _worker.PropertyChanged += _workerPropertyChangedHandler; @@ -392,6 +396,24 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable }; _worker.TaskUpdatedEvent += _workerTaskUpdatedHandler; + _workerPlanningMergeStartedHandler = (planningTaskId, _) => + { + if (Task?.Id == planningTaskId) IsMergeDraining = true; + }; + _worker.PlanningMergeStartedEvent += _workerPlanningMergeStartedHandler; + + _workerPlanningMergeAbortedHandler = planningTaskId => + { + if (Task?.Id == planningTaskId) IsMergeDraining = false; + }; + _worker.PlanningMergeAbortedEvent += _workerPlanningMergeAbortedHandler; + + _workerPlanningCompletedHandler = planningTaskId => + { + if (Task?.Id == planningTaskId) IsMergeDraining = false; + }; + _worker.PlanningCompletedEvent += _workerPlanningCompletedHandler; + ChildOutcomes.CollectionChanged += (_, _) => { Merge.SyncChildOutcomes(HasChildOutcomes, Subtasks.Count); @@ -409,6 +431,9 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable _worker.TaskFinishedEvent -= _workerTaskFinishedHandler; _worker.WorktreeUpdatedEvent -= _workerWorktreeUpdatedHandler; _worker.TaskUpdatedEvent -= _workerTaskUpdatedHandler; + _worker.PlanningMergeStartedEvent -= _workerPlanningMergeStartedHandler; + _worker.PlanningMergeAbortedEvent -= _workerPlanningMergeAbortedHandler; + _worker.PlanningCompletedEvent -= _workerPlanningCompletedHandler; AgentSettings.Dispose(); Prep.Dispose(); } @@ -438,6 +463,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable DequeueCommand.NotifyCanExecuteChanged(); ResetAndRetryCommand.NotifyCanExecuteChanged(); ContinueCommand.NotifyCanExecuteChanged(); + CancelReviewCommand.NotifyCanExecuteChanged(); // A state change means a new run/review cycle: the diff must be // re-inspected before merge can be approved again. ReviewDiffViewed = false; @@ -547,6 +573,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable OnPropertyChanged(nameof(TaskIdBadge)); Monitor.Reset(); RoadblockReplyDraft = string.Empty; + IsMergeDraining = false; Subtasks.Clear(); ChildOutcomes.Clear(); Attachments.Clear(); @@ -1204,14 +1231,28 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable catch { /* stale review action; broadcast reconciles */ } } - [RelayCommand] + // True while a unit merge is actively draining this task's subtasks onto the target + // branch — set from PlanningMergeStarted, cleared on PlanningMergeAborted/Completed. + // Mirrors the worker-side guard in TaskStateService.CancelAsync (the real correctness + // fix); this just keeps the button from inviting a click the worker will reject anyway. + [ObservableProperty] + [NotifyCanExecuteChangedFor(nameof(CancelReviewCommand))] + private bool _isMergeDraining; + + [RelayCommand(CanExecute = nameof(CanCancelReview))] private async System.Threading.Tasks.Task CancelReviewAsync() { if (Task is null || !_worker.IsConnected) return; try { await _worker.CancelReviewAsync(Task.Id); } - catch { /* stale review action; broadcast reconciles */ } + catch (Exception ex) + { + ErrorReported?.Invoke(ex.Message); + } } + private bool CanCancelReview() => + Task != null && _worker.IsConnected && !IsMergeDraining; + private async System.Threading.Tasks.Task ReloadAttachmentsAsync() { if (Task is null) return; diff --git a/src/ClaudeDo.Worker/Planning/Interfaces/IActiveMergeState.cs b/src/ClaudeDo.Worker/Planning/Interfaces/IActiveMergeState.cs new file mode 100644 index 00000000..c156fe40 --- /dev/null +++ b/src/ClaudeDo.Worker/Planning/Interfaces/IActiveMergeState.cs @@ -0,0 +1,10 @@ +namespace ClaudeDo.Worker.Planning; + +// Lets TaskStateService guard CancelAsync against an in-progress unit merge without a +// hard constructor cycle back to PlanningMergeOrchestrator (which itself depends on +// ITaskStateService) — TaskStateService takes a Func instead, resolved +// lazily so both singletons can finish constructing before either calls into the other. +public interface IActiveMergeState +{ + bool HasActiveMerge(string taskId); +} diff --git a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs index 0e49aeb8..dbafc844 100644 --- a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs +++ b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs @@ -13,7 +13,7 @@ namespace ClaudeDo.Worker.Planning; /// A unit-merge conflict currently paused, driven by an MCP session rather than the UI. public sealed record ExternalPlanningMergeConflict(string PlanningTaskId, string SubtaskId); -public sealed class PlanningMergeOrchestrator +public sealed class PlanningMergeOrchestrator : IActiveMergeState { private readonly IDbContextFactory _dbFactory; private readonly TaskMergeService _merge; diff --git a/src/ClaudeDo.Worker/Program.cs b/src/ClaudeDo.Worker/Program.cs index ae85c54d..c4fd4ed1 100644 --- a/src/ClaudeDo.Worker/Program.cs +++ b/src/ClaudeDo.Worker/Program.cs @@ -101,12 +101,16 @@ builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton>(sp => () => sp.GetRequiredService()); +// PlanningMergeOrchestrator itself depends on ITaskStateService, so TaskStateService can only +// reach it lazily (Func) — same cycle-breaking shape as the Func above. +builder.Services.AddSingleton>(sp => () => sp.GetRequiredService()); builder.Services.AddSingleton(sp => new TaskStateService( sp.GetRequiredService>(), sp.GetRequiredService(), sp.GetRequiredService(), sp.GetRequiredService(), sp.GetRequiredService(), + sp.GetRequiredService>(), sp.GetRequiredService>())); // Agent file management. diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index c04fe777..cf1b86c2 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/src/ClaudeDo.Worker/State/TaskStateService.cs @@ -16,6 +16,7 @@ public sealed class TaskStateService : ITaskStateService private readonly IQueueWaker _waker; private readonly PlanningChainCoordinator _chain; private readonly RunCancellationRegistry _runCancels; + private readonly Func _mergeState; private readonly ILogger _logger; public TaskStateService( @@ -24,6 +25,7 @@ public sealed class TaskStateService : ITaskStateService IQueueWaker waker, PlanningChainCoordinator chain, RunCancellationRegistry runCancels, + Func mergeState, ILogger logger) { _dbFactory = dbFactory; @@ -31,6 +33,7 @@ public sealed class TaskStateService : ITaskStateService _waker = waker; _chain = chain; _runCancels = runCancels; + _mergeState = mergeState; _logger = logger; } @@ -255,6 +258,14 @@ public sealed class TaskStateService : ITaskStateService public async Task CancelAsync( string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false) { + // A unit merge drains its children's worktrees onto the target branch while the parent + // sits in WaitingForReview for the whole (potentially minutes-long) drain. Cancelling out + // from under it would flip the parent to Cancelled while the orchestrator keeps merging — + // FinalizeParentDoneAsync then finds the parent no longer WaitingForReview and gives up, + // leaving the merged children's diffs stranded with no way to roll them back. + if (_mergeState().HasActiveMerge(taskId)) + return new TransitionResult(false, "A merge is in progress for this task; wait for it to finish before cancelling."); + List cancelledChildIds; await using (var ctx = await _dbFactory.CreateDbContextAsync(ct)) { diff --git a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs index 75a79abd..00060353 100644 --- a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs +++ b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs @@ -55,6 +55,7 @@ public abstract class StubWorkerClient : IWorkerClient public void RaiseHandoffRequested(string taskId, IReadOnlyList survivingTaskIds) => HandoffRequestedEvent?.Invoke(taskId, survivingTaskIds); public void RaisePlanningMergeConflict(string planningTaskId, string subtaskId, IReadOnlyList 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); diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs index aba181ae..4eec5c2c 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs @@ -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); + } } diff --git a/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs b/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs index 283a193e..0948959d 100644 --- a/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs +++ b/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs @@ -20,7 +20,8 @@ public static class TaskStateServiceBuilder CountingQueueWaker Waker, RunCancellationRegistry RunCancels); - public static Built Build(IDbContextFactory dbFactory) + public static Built Build( + IDbContextFactory dbFactory, Func? 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.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; diff --git a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs index a2ce8a7b..70c08627 100644 --- a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs @@ -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 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]