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.
This commit is contained in:
@@ -1,8 +1,8 @@
|
|||||||
# External MCP tool surface
|
# External MCP tool surface
|
||||||
|
|
||||||
> **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
|
> **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
|
||||||
> Last verified against commit `bdee731` (2026-08-05).
|
> Last verified against commit `7cfe280` (2026-08-06).
|
||||||
> Drift check: `git log --oneline bdee731..HEAD -- src/ClaudeDo.Worker/External`
|
> Drift check: `git log --oneline 7cfe280..HEAD -- src/ClaudeDo.Worker/External`
|
||||||
> Stable structure only (no line numbers). See docs/explore-notes/README.md.
|
> 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
|
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.
|
**`GetTaskLog`** — latest run's log, tail-capped at 256 KB.
|
||||||
|
|
||||||
**`WaitForTaskChange(taskIds, timeoutSeconds = 60)`** — blocks until any given task leaves
|
**`WaitForTaskChange(taskIds, timeoutSeconds = 60, treatWaitingForChildrenAsBusy = false)`** —
|
||||||
`Queued`/`Running`, or times out. Returns immediately for a task already outside those two
|
blocks until any given task leaves `Queued`/`Running`, or times out. Returns immediately for a
|
||||||
(unknown ids reported as status `"NotFound"`, also immediate).
|
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
|
- 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
|
held connection, no busy loop) rather than hooking `HubBroadcaster` — deliberately isolated
|
||||||
so it can't regress the existing broadcast callers.
|
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
|
`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
|
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.
|
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.
|
- 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
|
**`GetQueueState()`** — read-only snapshot so a caller doesn't have to infer queue state from
|
||||||
|
|||||||
+16
-7
@@ -32,11 +32,17 @@ public sealed class TaskWaitMcpTools
|
|||||||
"Blocks until at least one of the given tasks leaves Queued/Running, or until timeoutSeconds elapses " +
|
"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 " +
|
"(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 " +
|
"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 " +
|
"of polling get_task in a loop. Pitfall: a planning parent with children goes Running -> " +
|
||||||
"MCP_TOOL_TIMEOUT >= 930000 (ms) for a long wait to actually be held open -- ClaudeDo's own " +
|
"WaitingForChildren while its children are still working, and by default that counts as \"changed\" -- " +
|
||||||
"launchers already set this. Result: { changed: [{ taskId, status }], timedOut }.")]
|
"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<WaitForTaskChangeResult> WaitForTaskChange(
|
public async Task<WaitForTaskChangeResult> WaitForTaskChange(
|
||||||
string[] taskIds, int timeoutSeconds = 60, CancellationToken cancellationToken = default)
|
string[] taskIds, int timeoutSeconds = 60, bool treatWaitingForChildrenAsBusy = false,
|
||||||
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
if (taskIds.Length == 0)
|
if (taskIds.Length == 0)
|
||||||
throw new ArgumentException("taskIds must not be empty.", nameof(taskIds));
|
throw new ArgumentException("taskIds must not be empty.", nameof(taskIds));
|
||||||
@@ -49,7 +55,7 @@ public sealed class TaskWaitMcpTools
|
|||||||
{
|
{
|
||||||
while (true)
|
while (true)
|
||||||
{
|
{
|
||||||
var changed = await CheckOnceAsync(taskIds, linked.Token);
|
var changed = await CheckOnceAsync(taskIds, treatWaitingForChildrenAsBusy, linked.Token);
|
||||||
if (changed.Count > 0)
|
if (changed.Count > 0)
|
||||||
return new WaitForTaskChangeResult(changed, TimedOut: false);
|
return new WaitForTaskChangeResult(changed, TimedOut: false);
|
||||||
|
|
||||||
@@ -62,7 +68,8 @@ public sealed class TaskWaitMcpTools
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private async Task<IReadOnlyList<TaskStatusChangeDto>> CheckOnceAsync(string[] taskIds, CancellationToken ct)
|
private async Task<IReadOnlyList<TaskStatusChangeDto>> CheckOnceAsync(
|
||||||
|
string[] taskIds, bool treatWaitingForChildrenAsBusy, CancellationToken ct)
|
||||||
{
|
{
|
||||||
await using var ctx = await _dbFactory.CreateDbContextAsync(ct);
|
await using var ctx = await _dbFactory.CreateDbContextAsync(ct);
|
||||||
var rows = await ctx.Tasks
|
var rows = await ctx.Tasks
|
||||||
@@ -80,7 +87,9 @@ public sealed class TaskWaitMcpTools
|
|||||||
result.Add(new TaskStatusChangeDto(id, "NotFound"));
|
result.Add(new TaskStatusChangeDto(id, "NotFound"));
|
||||||
continue;
|
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()));
|
result.Add(new TaskStatusChangeDto(id, status.ToString()));
|
||||||
}
|
}
|
||||||
return result;
|
return result;
|
||||||
|
|||||||
@@ -46,7 +46,7 @@ public sealed class TaskWaitMcpToolsTests : IDisposable
|
|||||||
var sut = BuildSut();
|
var sut = BuildSut();
|
||||||
var sw = Stopwatch.StartNew();
|
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();
|
sw.Stop();
|
||||||
Assert.False(result.TimedOut);
|
Assert.False(result.TimedOut);
|
||||||
@@ -61,7 +61,7 @@ public sealed class TaskWaitMcpToolsTests : IDisposable
|
|||||||
var sut = BuildSut();
|
var sut = BuildSut();
|
||||||
var sw = Stopwatch.StartNew();
|
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();
|
sw.Stop();
|
||||||
Assert.False(result.TimedOut);
|
Assert.False(result.TimedOut);
|
||||||
@@ -76,7 +76,7 @@ public sealed class TaskWaitMcpToolsTests : IDisposable
|
|||||||
var sut = BuildSut();
|
var sut = BuildSut();
|
||||||
var sw = Stopwatch.StartNew();
|
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);
|
await Task.Delay(150);
|
||||||
// Simulate the status change a broadcast would announce, via a separate context
|
// 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 sut = BuildSut();
|
||||||
var sw = Stopwatch.StartNew();
|
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();
|
sw.Stop();
|
||||||
Assert.True(result.TimedOut);
|
Assert.True(result.TimedOut);
|
||||||
@@ -117,7 +117,83 @@ public sealed class TaskWaitMcpToolsTests : IDisposable
|
|||||||
{
|
{
|
||||||
var sut = BuildSut();
|
var sut = BuildSut();
|
||||||
await Assert.ThrowsAsync<ArgumentException>(() =>
|
await Assert.ThrowsAsync<ArgumentException>(() =>
|
||||||
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]
|
[Fact]
|
||||||
|
|||||||
Reference in New Issue
Block a user