fix(worker): survive a worker restart when cancelling a mid-merge unit-merge parent
CancelAsync only checked PlanningMergeOrchestrator's in-memory HasActiveMerge, which is empty after a restart. IActiveMergeState gains an async fallback that checks on-disk (GitService.IsMidMergeAsync) for a WaitingForReview parent with children, so the guard still blocks cancel until the merge is continued or aborted.
This commit is contained in:
@@ -7,4 +7,9 @@ namespace ClaudeDo.Worker.Planning;
|
|||||||
public interface IActiveMergeState
|
public interface IActiveMergeState
|
||||||
{
|
{
|
||||||
bool HasActiveMerge(string taskId);
|
bool HasActiveMerge(string taskId);
|
||||||
|
|
||||||
|
// In-memory state above is empty after a worker restart. This falls back to an on-disk
|
||||||
|
// check (repo mid-merge) for the one case that matters: a unit-merge parent paused on a
|
||||||
|
// conflict when the process died mid-drain.
|
||||||
|
Task<bool> HasActiveMergeAsync(string taskId, CancellationToken ct);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -164,6 +164,31 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState
|
|||||||
public bool HasActiveMerge(string parentTaskId) =>
|
public bool HasActiveMerge(string parentTaskId) =>
|
||||||
_states.TryGetValue(parentTaskId, out var s) && (s.CurrentSubtaskId is not null || s.IsFinalizing);
|
_states.TryGetValue(parentTaskId, out var s) && (s.CurrentSubtaskId is not null || s.IsFinalizing);
|
||||||
|
|
||||||
|
/// <summary>Stateless fallback for <see cref="HasActiveMerge"/>: after a worker restart
|
||||||
|
/// <see cref="_states"/> is empty, so a unit-merge parent paused on a conflict would look
|
||||||
|
/// cancellable when its repo is still mid-merge. Only a WaitingForReview task with children
|
||||||
|
/// (i.e. a would-be merge parent) triggers the on-disk check — an ordinary task in the same
|
||||||
|
/// list must stay cancellable even while the list's repo is mid-merge.</summary>
|
||||||
|
public async Task<bool> HasActiveMergeAsync(string taskId, CancellationToken ct)
|
||||||
|
{
|
||||||
|
if (HasActiveMerge(taskId)) return true;
|
||||||
|
|
||||||
|
string? workingDir;
|
||||||
|
bool hasChildren;
|
||||||
|
await using (var ctx = _dbFactory.CreateDbContext())
|
||||||
|
{
|
||||||
|
var task = await ctx.Tasks
|
||||||
|
.Include(t => t.List)
|
||||||
|
.SingleOrDefaultAsync(t => t.Id == taskId, ct);
|
||||||
|
if (task is null || task.Status != TaskStatus.WaitingForReview) return false;
|
||||||
|
workingDir = task.List.WorkingDir;
|
||||||
|
hasChildren = await ctx.Tasks.AnyAsync(c => c.ParentTaskId == taskId, ct);
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!hasChildren || string.IsNullOrWhiteSpace(workingDir)) return false;
|
||||||
|
return await _git.IsMidMergeAsync(workingDir, ct);
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>Externally-driven unit merges currently paused on a conflict, so the UI can
|
/// <summary>Externally-driven unit merges currently paused on a conflict, so the UI can
|
||||||
/// recover this on reconnect instead of relying solely on the one-shot broadcast. Checked
|
/// recover this on reconnect instead of relying solely on the one-shot broadcast. Checked
|
||||||
/// against <see cref="GitService.IsMidMergeAsync"/> rather than trusting the in-memory flag
|
/// against <see cref="GitService.IsMidMergeAsync"/> rather than trusting the in-memory flag
|
||||||
|
|||||||
@@ -286,7 +286,7 @@ public sealed class TaskStateService : ITaskStateService
|
|||||||
// from under it would flip the parent to Cancelled while the orchestrator keeps merging —
|
// 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,
|
// 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.
|
// leaving the merged children's diffs stranded with no way to roll them back.
|
||||||
if (_mergeState().HasActiveMerge(taskId))
|
if (await _mergeState().HasActiveMergeAsync(taskId, ct))
|
||||||
return new TransitionResult(false, "A merge is in progress for this task; wait for it to finish before cancelling.");
|
return new TransitionResult(false, "A merge is in progress for this task; wait for it to finish before cancelling.");
|
||||||
|
|
||||||
List<string> cancelledChildIds;
|
List<string> cancelledChildIds;
|
||||||
|
|||||||
@@ -66,6 +66,7 @@ file sealed class NoActiveMergeState : IActiveMergeState
|
|||||||
{
|
{
|
||||||
public static readonly NoActiveMergeState Instance = new();
|
public static readonly NoActiveMergeState Instance = new();
|
||||||
public bool HasActiveMerge(string taskId) => false;
|
public bool HasActiveMerge(string taskId) => false;
|
||||||
|
public Task<bool> HasActiveMergeAsync(string taskId, CancellationToken ct) => Task.FromResult(false);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Never actually invoked: TicketSystemConfig.IsConfigured is false in every TaskStateServiceBuilder
|
// Never actually invoked: TicketSystemConfig.IsConfigured is false in every TaskStateServiceBuilder
|
||||||
|
|||||||
@@ -162,6 +162,36 @@ public sealed class PlanningMergeOrchestratorTests : IDisposable
|
|||||||
Assert.Contains(spy, c => c.Method == "PlanningCompleted");
|
Assert.Contains(spy, c => c.Method == "PlanningCompleted");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task CancelAsync_UnitMergeParentMidMergeAfterSimulatedRestart_BlocksUntilAborted()
|
||||||
|
{
|
||||||
|
var db = NewDb();
|
||||||
|
var repo = NewRepo();
|
||||||
|
GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main");
|
||||||
|
|
||||||
|
var (parentId, _, _, _) = await SeedPlanningThreeChildrenMiddleConflictsAsync(db, repo);
|
||||||
|
|
||||||
|
var (orch, _) = BuildOrchestrator(db);
|
||||||
|
var startResult = await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||||
|
Assert.Equal(TaskMergeService.StatusConflict, startResult.Status);
|
||||||
|
|
||||||
|
// Simulate a worker restart: a brand-new orchestrator instance has an empty in-memory
|
||||||
|
// _states dictionary, so the guard must fall back to the on-disk mid-merge check.
|
||||||
|
var (freshOrch, _) = BuildOrchestrator(db);
|
||||||
|
var cancelBuilt = TaskStateServiceBuilder.Build(db.CreateFactory(), () => freshOrch);
|
||||||
|
|
||||||
|
var blocked = await cancelBuilt.State.CancelAsync(parentId, DateTime.UtcNow, CancellationToken.None);
|
||||||
|
Assert.False(blocked.Ok);
|
||||||
|
|
||||||
|
using (var ctx = db.CreateContext())
|
||||||
|
Assert.Equal(TaskStatus.WaitingForReview, ctx.Tasks.Single(t => t.Id == parentId).Status);
|
||||||
|
|
||||||
|
GitRepoFixture.RunGit(repo.RepoDir, "merge", "--abort");
|
||||||
|
|
||||||
|
var ok = await cancelBuilt.State.CancelAsync(parentId, DateTime.UtcNow, CancellationToken.None);
|
||||||
|
Assert.True(ok.Ok);
|
||||||
|
}
|
||||||
|
|
||||||
private async Task<(string parentId, string subA, string subB, string subC)> SeedPlanningThreeChildrenMiddleConflictsAsync(
|
private async Task<(string parentId, string subA, string subB, string subC)> SeedPlanningThreeChildrenMiddleConflictsAsync(
|
||||||
DbFixture db, GitRepoFixture repo)
|
DbFixture db, GitRepoFixture repo)
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -15,6 +15,7 @@ file sealed class FakeActiveMergeState : IActiveMergeState
|
|||||||
{
|
{
|
||||||
public HashSet<string> ActiveTaskIds { get; } = new();
|
public HashSet<string> ActiveTaskIds { get; } = new();
|
||||||
public bool HasActiveMerge(string taskId) => ActiveTaskIds.Contains(taskId);
|
public bool HasActiveMerge(string taskId) => ActiveTaskIds.Contains(taskId);
|
||||||
|
public Task<bool> HasActiveMergeAsync(string taskId, CancellationToken ct) => Task.FromResult(HasActiveMerge(taskId));
|
||||||
}
|
}
|
||||||
|
|
||||||
public sealed class TaskStateServiceTests : IDisposable
|
public sealed class TaskStateServiceTests : IDisposable
|
||||||
|
|||||||
Reference in New Issue
Block a user