fix(worker): route FinalizeParentDoneAsync through TaskStateService
Replaces the direct EF Status write in PlanningMergeOrchestrator with _state.ApproveReviewAsync, enforcing the TaskStateService invariant as sole owner of Status writes. Handles the improvement-parent path where TaskMergeService already approved the parent's own worktree during the drain (status == Done on entry → still success). If the parent was concurrently cancelled, the transition guard rejects the approve, PlanningCompleted is not broadcast, and the cancelled status is preserved. ApproveReviewAsync now also sets FinishedAt. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -4,6 +4,7 @@ using ClaudeDo.Data.Git;
|
||||
using ClaudeDo.Data.Models;
|
||||
using ClaudeDo.Worker.Hub;
|
||||
using ClaudeDo.Worker.Lifecycle;
|
||||
using ClaudeDo.Worker.State;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using TaskStatus = ClaudeDo.Data.Models.TaskStatus;
|
||||
|
||||
@@ -16,6 +17,7 @@ public sealed class PlanningMergeOrchestrator
|
||||
private readonly PlanningAggregator _aggregator;
|
||||
private readonly HubBroadcaster _broadcaster;
|
||||
private readonly GitService _git;
|
||||
private readonly ITaskStateService _state;
|
||||
private readonly ILogger<PlanningMergeOrchestrator> _logger;
|
||||
|
||||
private sealed class State
|
||||
@@ -34,6 +36,7 @@ public sealed class PlanningMergeOrchestrator
|
||||
PlanningAggregator aggregator,
|
||||
HubBroadcaster broadcaster,
|
||||
GitService git,
|
||||
ITaskStateService state,
|
||||
ILogger<PlanningMergeOrchestrator> logger)
|
||||
{
|
||||
_dbFactory = dbFactory;
|
||||
@@ -41,6 +44,7 @@ public sealed class PlanningMergeOrchestrator
|
||||
_aggregator = aggregator;
|
||||
_broadcaster = broadcaster;
|
||||
_git = git;
|
||||
_state = state;
|
||||
_logger = logger;
|
||||
}
|
||||
|
||||
@@ -181,8 +185,9 @@ public sealed class PlanningMergeOrchestrator
|
||||
}
|
||||
|
||||
state.CurrentSubtaskId = null;
|
||||
await FinalizeParentDoneAsync(planningTaskId, state.IsPlanning, ct);
|
||||
await _broadcaster.PlanningCompleted(planningTaskId);
|
||||
var finalized = await FinalizeParentDoneAsync(planningTaskId, state.IsPlanning, ct);
|
||||
if (finalized)
|
||||
await _broadcaster.PlanningCompleted(planningTaskId);
|
||||
}
|
||||
finally
|
||||
{
|
||||
@@ -190,18 +195,30 @@ public sealed class PlanningMergeOrchestrator
|
||||
}
|
||||
}
|
||||
|
||||
private async Task FinalizeParentDoneAsync(string parentTaskId, bool isPlanning, CancellationToken ct)
|
||||
private async Task<bool> FinalizeParentDoneAsync(string parentTaskId, bool isPlanning, CancellationToken ct)
|
||||
{
|
||||
using var ctx = _dbFactory.CreateDbContext();
|
||||
var parent = await ctx.Tasks.SingleOrDefaultAsync(t => t.Id == parentTaskId, ct);
|
||||
if (parent is null) return;
|
||||
parent.Status = TaskStatus.Done;
|
||||
parent.FinishedAt = DateTime.UtcNow;
|
||||
await ctx.SaveChangesAsync(ct);
|
||||
var result = await _state.ApproveReviewAsync(parentTaskId, ct);
|
||||
if (!result.Ok)
|
||||
{
|
||||
// ApproveReviewAsync requires WaitingForReview. For improvement parents whose own
|
||||
// worktree is in the merge queue, TaskMergeService.ApproveIfWaitingForReviewAsync
|
||||
// already approved the parent during the drain — check for that expected path.
|
||||
await using var ctx = _dbFactory.CreateDbContext();
|
||||
var current = await ctx.Tasks
|
||||
.Where(t => t.Id == parentTaskId)
|
||||
.Select(t => (TaskStatus?)t.Status)
|
||||
.FirstOrDefaultAsync(ct);
|
||||
|
||||
// Surface the Done transition to the UI. Without this the parent row stays
|
||||
// visibly stuck in WaitingForReview even though the unit merge completed.
|
||||
await _broadcaster.TaskUpdated(parentTaskId);
|
||||
if (current != TaskStatus.Done)
|
||||
{
|
||||
// Parent was cancelled or moved to an unexpected state during the merge drain.
|
||||
// Do not overwrite — the external transition takes precedence.
|
||||
_logger.LogWarning(
|
||||
"Unit-merge drain completed but parent {ParentTaskId} could not be finalized (status: {Status}): {Reason}",
|
||||
parentTaskId, current, result.Reason);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
// Only planning builds an integration branch via the aggregator; skip cleanup otherwise.
|
||||
if (isPlanning)
|
||||
@@ -209,5 +226,7 @@ public sealed class PlanningMergeOrchestrator
|
||||
try { await _aggregator.CleanupIntegrationBranchAsync(parentTaskId, ct); }
|
||||
catch (Exception ex) { _logger.LogWarning(ex, "integration branch cleanup failed"); }
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -126,11 +126,14 @@ public sealed class TaskStateService : ITaskStateService
|
||||
|
||||
public async Task<TransitionResult> ApproveReviewAsync(string taskId, CancellationToken ct)
|
||||
{
|
||||
var now = DateTime.UtcNow;
|
||||
await using (var ctx = await _dbFactory.CreateDbContextAsync(ct))
|
||||
{
|
||||
var affected = await ctx.Tasks
|
||||
.Where(t => t.Id == taskId && t.Status == TaskStatus.WaitingForReview)
|
||||
.ExecuteUpdateAsync(s => s.SetProperty(t => t.Status, TaskStatus.Done), ct);
|
||||
.ExecuteUpdateAsync(s => s
|
||||
.SetProperty(t => t.Status, TaskStatus.Done)
|
||||
.SetProperty(t => t.FinishedAt, now), ct);
|
||||
|
||||
if (affected == 0)
|
||||
return new TransitionResult(false, "Task is not waiting for review; cannot approve.");
|
||||
|
||||
Reference in New Issue
Block a user