fix(daemon): wait on tracked workers in TestPoolDrainKillsStraggler (#919)

[cairn-intern]

Summary

Fixes #919.

TestPoolDrainKillsStraggler was racing on loaded CI because it waited on pool.QueueDepth() == 1 (slot occupancy) before calling Drain(). QueueDepth() becomes 1 as soon as Run leases a slot; the worker is only added to p.active later, after Launcher returns (runOnce → track). Drain() reads len(p.active) and takes the n == 0 early return if it runs in that window, so straggler.killed stays 0.

This is a test synchronization defect, not a production Drain bug.

Change

In internal/daemon/pool_test.go, wait on the state Drain actually reads:

waitFor(t, func() bool { return len(pool.WorkerStats()) == 1 })

WorkerStats() is built from p.active.

I grepped neighbouring pool tests for the same QueueDepth() wait-before-Drain pattern. The only other QueueDepth() wait is in TestPoolQueuesWhenFull, which is asserting slot occupancy / queueing, not drain tracking — left unchanged.

Test-only; no production code change.

Notes

Opened by cairn-intern. Parent issue has issue-approved + bug.

I did not run go test for this change (no local checkout). Worth go test ./internal/daemon/ -run TestPoolDrainKillsStraggler on CI.

There is an overlapping in-flight PR (#927) that also cites #919 but takes a larger production Drain/launch-race approach. This PR is the small test-only sync the issue describes.

Summary by CodeRabbit

• Tests
  • Improved test synchronization for worker shutdown scenarios.
  • Added clarification to ensure straggler workers are properly detected and terminated during drain operations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1c4a94d5-fd98-4724-8ffa-568b40114820

📥 Commits

Reviewing files that changed from the base of the PR and between 27b319c and 6975197.

📒 Files selected for processing (1)

• internal/daemon/pool_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

Walkthrough

The pool drain test now waits for the worker to appear in WorkerStats() before testing straggler termination. Comments document why QueueDepth() does not provide sufficient synchronization.

Changes

Pool drain test synchronization

Layer / File(s)	Summary
Wait for tracked worker readiness internal/daemon/pool_test.go	TestPoolDrainKillsStraggler now waits for one tracked worker through WorkerStats() instead of using QueueDepth(). Comments explain the tracking race.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 69751

This localized test-only change waits on the worker state used by Drain, with no actionable merge-blocking risk remaining after normal checks and review.

Suggested reviewers: gnanam1990

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the daemon test fix and states that the test now waits on tracked workers. It matches the primary change.
Linked Issues check	✅ Passed	The change satisfies issue #919 by replacing the QueueDepth-based wait with len(pool.WorkerStats()) == 1. This synchronizes the test with the worker-tracking state that Drain() reads and addresses the…
Out of Scope Changes check	✅ Passed	The only change is the targeted synchronization update and explanatory comments in internal/daemon/pool_test.go. No unrelated or production-code changes are present.

Full details: Linked Issues check

Explanation

The change satisfies issue #919 by replacing the QueueDepth-based wait with len(pool.WorkerStats()) == 1. This synchronizes the test with the worker-tracking state that Drain() reads and addresses the reported race.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[cairn-intern]

Closing as duplicate of #927, which already includes this test-sync (WorkerStats wait) plus the production Drain/launch-race fix. Sorry for the noise.