From af168300605aea074f16b3b2aad47a0a4f77a8d6 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 11:32:00 +0200 Subject: [PATCH] feat(worker): add treatWaitingForChildrenAsBusy to wait_for_task_change WaitingForChildren already counted as "changed" since it's outside Queued/Running, so waiting on a planning parent returned immediately even though its children were still running. The new opt-in flag (default false, unchanged behavior) keeps polling through WaitingForChildren and only reports changed once the parent reaches WaitingForReview or a terminal status. --- docs/explore-notes/external-mcp.md | 17 ++-- .../External/TaskWaitMcpTools.cs | 23 +++-- .../External/TaskWaitMcpToolsTests.cs | 86 +++++++++++++++++-- 3 files changed, 109 insertions(+), 17 deletions(-) diff --git a/docs/explore-notes/external-mcp.md b/docs/explore-notes/external-mcp.md index bc28c2f6..599f0c30 100644 --- a/docs/explore-notes/external-mcp.md +++ b/docs/explore-notes/external-mcp.md @@ -1,8 +1,8 @@ # External MCP tool surface > **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative. -> Last verified against commit `bdee731` (2026-08-05). -> Drift check: `git log --oneline bdee731..HEAD -- src/ClaudeDo.Worker/External` +> Last verified against commit `7cfe280` (2026-08-06). +> Drift check: `git log --oneline 7cfe280..HEAD -- src/ClaudeDo.Worker/External` > Stable structure only (no line numbers). See docs/explore-notes/README.md. Covers `src/ClaudeDo.Worker/External/` — the always-on MCP tools ClaudeDo exposes to general @@ -105,9 +105,9 @@ failing item never aborts the rest — and rejects batches over **100 items**. **`GetTaskLog`** — latest run's log, tail-capped at 256 KB. -**`WaitForTaskChange(taskIds, timeoutSeconds = 60)`** — blocks until any given task leaves -`Queued`/`Running`, or times out. Returns immediately for a task already outside those two -(unknown ids reported as status `"NotFound"`, also immediate). +**`WaitForTaskChange(taskIds, timeoutSeconds = 60, treatWaitingForChildrenAsBusy = false)`** — +blocks until any given task leaves `Queued`/`Running`, or times out. Returns immediately for a +task already outside those two (unknown ids reported as status `"NotFound"`, also immediate). - Implemented as an **async DB poll** (short-lived `DbContext` per check, 500 ms delay, no held connection, no busy loop) rather than hooking `HubBroadcaster` — deliberately isolated so it can't regress the existing broadcast callers. @@ -118,6 +118,13 @@ failing item never aborts the rest — and rejects batches over **100 items**. `timedOut: true` instead of racing the client's own abort. A caller running claude with a different (or default: 60 s) `MCP_TOOL_TIMEOUT` will still see its own client-side timeout fire first; the server has no way to detect or compensate for that. +- **`treatWaitingForChildrenAsBusy` pitfall (default `false`, backward-compatible):** a planning + parent with children goes `Running` → `WaitingForChildren` while children are still working, + and by default that already counts as "changed" (it's outside `Queued`/`Running`) — so waiting + on a parent returns immediately even though the unit isn't done. Set the flag to keep polling + through `WaitingForChildren`; the call then only reports changed once the parent reaches + `WaitingForReview` or a terminal status. Does not list or watch the parent's children — + callers still need their own ids for that. - Replaced the list handler's old "sleep + poll `get_task` in a loop" Phase 3 instruction. **`GetQueueState()`** — read-only snapshot so a caller doesn't have to infer queue state from diff --git a/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs index 1dd4aef1..cede7a45 100644 --- a/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs +++ b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs @@ -32,11 +32,17 @@ public sealed class TaskWaitMcpTools "Blocks until at least one of the given tasks leaves Queued/Running, or until timeoutSeconds elapses " + "(clamped server-side to 900s). Returns immediately if any task is already outside Queued/Running " + "when called (an unknown id is reported as status \"NotFound\" and counts as changed). Use this instead " + - "of polling get_task in a loop. Requires the calling claude process to run with " + - "MCP_TOOL_TIMEOUT >= 930000 (ms) for a long wait to actually be held open -- ClaudeDo's own " + - "launchers already set this. Result: { changed: [{ taskId, status }], timedOut }.")] + "of polling get_task in a loop. Pitfall: a planning parent with children goes Running -> " + + "WaitingForChildren while its children are still working, and by default that counts as \"changed\" -- " + + "so waiting on a parent returns immediately even though the work isn't done. Set " + + "treatWaitingForChildrenAsBusy=true to keep waiting through WaitingForChildren; the call then only " + + "returns once the parent reaches WaitingForReview or a terminal status (default: false, unchanged " + + "legacy behavior). Requires the calling claude process to run with MCP_TOOL_TIMEOUT >= 930000 (ms) for " + + "a long wait to actually be held open -- ClaudeDo's own launchers already set this. " + + "Result: { changed: [{ taskId, status }], timedOut }.")] public async Task WaitForTaskChange( - string[] taskIds, int timeoutSeconds = 60, CancellationToken cancellationToken = default) + string[] taskIds, int timeoutSeconds = 60, bool treatWaitingForChildrenAsBusy = false, + CancellationToken cancellationToken = default) { if (taskIds.Length == 0) throw new ArgumentException("taskIds must not be empty.", nameof(taskIds)); @@ -49,7 +55,7 @@ public sealed class TaskWaitMcpTools { while (true) { - var changed = await CheckOnceAsync(taskIds, linked.Token); + var changed = await CheckOnceAsync(taskIds, treatWaitingForChildrenAsBusy, linked.Token); if (changed.Count > 0) return new WaitForTaskChangeResult(changed, TimedOut: false); @@ -62,7 +68,8 @@ public sealed class TaskWaitMcpTools } } - private async Task> CheckOnceAsync(string[] taskIds, CancellationToken ct) + private async Task> CheckOnceAsync( + string[] taskIds, bool treatWaitingForChildrenAsBusy, CancellationToken ct) { await using var ctx = await _dbFactory.CreateDbContextAsync(ct); var rows = await ctx.Tasks @@ -80,7 +87,9 @@ public sealed class TaskWaitMcpTools result.Add(new TaskStatusChangeDto(id, "NotFound")); continue; } - if (status != TaskStatus.Queued && status != TaskStatus.Running) + var busy = status == TaskStatus.Queued || status == TaskStatus.Running + || (treatWaitingForChildrenAsBusy && status == TaskStatus.WaitingForChildren); + if (!busy) result.Add(new TaskStatusChangeDto(id, status.ToString())); } return result; diff --git a/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs index 96e7ad9b..99bdb3d2 100644 --- a/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs @@ -46,7 +46,7 @@ public sealed class TaskWaitMcpToolsTests : IDisposable var sut = BuildSut(); var sw = Stopwatch.StartNew(); - var result = await sut.WaitForTaskChange([task.Id], timeoutSeconds: 30, CancellationToken.None); + var result = await sut.WaitForTaskChange([task.Id], timeoutSeconds: 30, cancellationToken: CancellationToken.None); sw.Stop(); Assert.False(result.TimedOut); @@ -61,7 +61,7 @@ public sealed class TaskWaitMcpToolsTests : IDisposable var sut = BuildSut(); var sw = Stopwatch.StartNew(); - var result = await sut.WaitForTaskChange(["missing-id"], timeoutSeconds: 30, CancellationToken.None); + var result = await sut.WaitForTaskChange(["missing-id"], timeoutSeconds: 30, cancellationToken: CancellationToken.None); sw.Stop(); Assert.False(result.TimedOut); @@ -76,7 +76,7 @@ public sealed class TaskWaitMcpToolsTests : IDisposable var sut = BuildSut(); var sw = Stopwatch.StartNew(); - var waitTask = sut.WaitForTaskChange([task.Id], timeoutSeconds: 10, CancellationToken.None); + var waitTask = sut.WaitForTaskChange([task.Id], timeoutSeconds: 10, cancellationToken: CancellationToken.None); await Task.Delay(150); // Simulate the status change a broadcast would announce, via a separate context @@ -104,7 +104,7 @@ public sealed class TaskWaitMcpToolsTests : IDisposable var sut = BuildSut(); var sw = Stopwatch.StartNew(); - var result = await sut.WaitForTaskChange([task.Id], timeoutSeconds: 1, CancellationToken.None); + var result = await sut.WaitForTaskChange([task.Id], timeoutSeconds: 1, cancellationToken: CancellationToken.None); sw.Stop(); Assert.True(result.TimedOut); @@ -117,7 +117,83 @@ public sealed class TaskWaitMcpToolsTests : IDisposable { var sut = BuildSut(); await Assert.ThrowsAsync(() => - sut.WaitForTaskChange([], timeoutSeconds: 5, CancellationToken.None)); + sut.WaitForTaskChange([], timeoutSeconds: 5, cancellationToken: CancellationToken.None)); + } + + [Fact] + public async Task WaitForTaskChange_WaitingForChildren_DefaultBehavior_ReturnsImmediately() + { + var task = await SeedTaskAsync(TaskStatus.WaitingForChildren); + var sut = BuildSut(); + var sw = Stopwatch.StartNew(); + + var result = await sut.WaitForTaskChange([task.Id], timeoutSeconds: 30, cancellationToken: CancellationToken.None); + + sw.Stop(); + Assert.False(result.TimedOut); + Assert.Equal("WaitingForChildren", Assert.Single(result.Changed).Status); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(2), $"took {sw.Elapsed}"); + } + + [Fact] + public async Task WaitForTaskChange_TreatWaitingForChildrenAsBusy_DoesNotReturnImmediately_TimesOut() + { + var task = await SeedTaskAsync(TaskStatus.WaitingForChildren); + var sut = BuildSut(); + var sw = Stopwatch.StartNew(); + + var result = await sut.WaitForTaskChange( + [task.Id], timeoutSeconds: 1, treatWaitingForChildrenAsBusy: true, cancellationToken: CancellationToken.None); + + sw.Stop(); + Assert.True(result.TimedOut); + Assert.Empty(result.Changed); + Assert.True(sw.Elapsed >= TimeSpan.FromMilliseconds(900), $"took {sw.Elapsed}"); + } + + [Fact] + public async Task WaitForTaskChange_TreatWaitingForChildrenAsBusy_ReturnsWhenParentReachesWaitingForReview() + { + var task = await SeedTaskAsync(TaskStatus.WaitingForChildren); + var sut = BuildSut(); + var sw = Stopwatch.StartNew(); + + var waitTask = sut.WaitForTaskChange( + [task.Id], timeoutSeconds: 30, treatWaitingForChildrenAsBusy: true, cancellationToken: CancellationToken.None); + + await Task.Delay(150); + await using (var writeCtx = _db.CreateContext()) + { + var writeRepo = new TaskRepository(writeCtx); + var loaded = await writeRepo.GetByIdAsync(task.Id); + loaded!.Status = TaskStatus.WaitingForReview; + await writeRepo.UpdateAsync(loaded); + } + + var result = await waitTask; + sw.Stop(); + + // Bound is generous (well under timeoutSeconds) purely to prove this didn't just + // coincidentally land on the timeout path -- correctness is already covered by + // Assert.False(result.TimedOut) above; this is not a performance assertion. + Assert.False(result.TimedOut); + Assert.Equal("WaitingForReview", Assert.Single(result.Changed).Status); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(25), $"took {sw.Elapsed}"); + } + + [Fact] + public async Task WaitForTaskChange_TreatWaitingForChildrenAsBusy_UnknownId_StillReturnsImmediatelyAsNotFound() + { + var sut = BuildSut(); + var sw = Stopwatch.StartNew(); + + var result = await sut.WaitForTaskChange( + ["missing-id"], timeoutSeconds: 30, treatWaitingForChildrenAsBusy: true, cancellationToken: CancellationToken.None); + + sw.Stop(); + Assert.False(result.TimedOut); + Assert.Equal("NotFound", Assert.Single(result.Changed).Status); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(2), $"took {sw.Elapsed}"); } [Fact]