Drain mixed-wait queued message groups - #2867
Conversation
|
🚨 SLOP COP 🚨 · I am reviewing this pull request now. I will check security, code quality, performance, architecture, and the changed behavior. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| eligibility === "plugin" | ||
| ? member.waitHolder !== null | ||
| : member.sendAt !== null && member.sendAt <= eligibility, | ||
| member.waitHolder !== null || |
There was a problem hiding this comment.
🚨 slopcop/review — [P1] This predicate still strands groups that mix a plugin wait with a core wait.
The group API permits mixed wait types. A plugin-held row can share a group with a thread-busy row.
The requested drain rejects the core-wait row here. The idle drain rejects the plugin-wait row.
I added a temporary regression test for this case. It failed because the drain made zero hook attempts.
Please use one shared eligibility model for plugin, timer, and core drain signals. Add tests for both signal orders.
There was a problem hiding this comment.
Valid finding—fixed in 9a44024. Automatic grouped drains now share one per-member eligibility model across plugin, timer, thread-busy, and turn-starting waits while validation and the whole-group claim remain inside the immediate transaction. I added production-path coverage for plugin + thread-busy in both signal orders, verified that a still-blocked plugin group does not churn through the idle drain, covered the turn-starting sibling path, and reran the plugin + timer and Send now atomicity regressions.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
This change lets a grouped prompt leave the queue after its separate plugin and schedule waits clear. The database still claims the complete group together.
I found one high-impact issue.
- The new predicate supports plugin waits and due timers only. A group with plugin and core waits can remain queued forever.
- A temporary regression test used a plugin-held row and a satisfied
thread-busyrow. The drain made zero hook attempts. - The group API permits this combination. The idle drain also rejects the plugin-held row.
I found no security issue. The claim stays inside one immediate database transaction and preserves thread ownership checks.
I found no performance issue. The predicate keeps the existing linear group scan and adds only a clock read.
I found no direct duplicate implementation. However, separate drain-specific eligibility rules caused this gap. One shared per-member eligibility model would reduce this risk.
Verification:
- The focused server file passed all nine existing tests.
- The temporary regression failed with zero hook attempts instead of one. I removed the test after the check.
- The local application loaded through doobie without console errors. The queued-message route returned HTTP 200.
- All current GitHub CI checks pass.
I posted the specific defect inline. I used a comment review only.
Automatic grouped claims required every member to match the same sweep trigger, so groups mixing plugin and timer waits could never become eligible. Compose per-member plugin or due-time eligibility while retaining the immediate whole-group claim.
18cba15 to
9a44024
Compare
## Human comments ## What was wrong Queued-message readiness was decided independently by startup events, idle sweeps, plugin/timer drains, failed-row handling, and manual Stop handling. Each fix could pass alone while their predicates disagreed when composed: a settled `turn-starting` row could be discovered but rejected, a failed grouped sibling could ride with a healthy row, and a stale runtime event or in-flight hook could cross a manual Stop. Startup admission also trusted a stale active thread after it changed to error, which could strand a row or change `steer` semantics. ## What changed - Made startup admission atomic: it either queues against the still-active startup or retries once from the current thread state. - Centralized automatic whole-group readiness across plugin, time, `thread-busy`, and `turn-starting` waits. - Replaced the positional `sendNow`/optional-callback claim API with an explicit `automatic` versus `explicit-send` policy. - Enforced failed-row exclusion and manual-Stop pause inside the group claim transaction. Explicit Send now remains the override; independent scheduled, plugin-held, and system-notice rows remain drainable. - Required a post-Stop client request before a later root turn can clear the pause, and rechecked the pause at final dispatch consumption. - Used one SQL `IN` predicate for idle-drainable wait kinds. This supersedes #2866 and #2867 and includes the failed-group and Stop-boundary follow-ups. There is no host-daemon wire, schema, CLI, SDK, migration, or documentation contract change. ## How you verified - Added deterministic regressions for the settled-startup lost wake, mixed-wait groups, failed grouped siblings, stale post-Stop starts, hook-held Stop races, and active-to-error startup admission for both `steer` modes. - Focused Turbo server matrix: 70/70 passed. - Focused Turbo DB/query-plan matrix: 68/68 passed. - Standalone production-path Stop race harness: both races passed. - Full DB suite: 444/444 passed. - Full server suite: 2,131 passed; 10 unrelated `install-machine-script` tests failed because their fake host daemon did not connect. - `pnpm exec turbo run typecheck --filter=@bb/server --filter=@bb/db` passed. - `pnpm exec turbo run build --filter=@bb/server` passed. - `git diff --check` passed. > AGENT GENERATED
1 participant
Human comments
What was wrong
The atomic grouped-queue landing correctly stopped automatic drains from splitting a grouped prompt. The follow-up eligibility predicates were still sweep-specific, though: every member had to look plugin-held or scheduled. That stranded plugin + timer groups, and the same split model stranded plugin + core groups such as plugin +
thread-busy, because neither the plugin sweep nor the idle drain could claim the complete group.What changed
Automatic drains now use one shared per-member eligibility model for plugin, timer, and core waits. A timer must be due,
thread-busymust observe an idle/pending thread, andturn-startingmust observe an active turn; waits released by provisioning, host reconnect, and interaction settlement retain their existing clear-and-drain paths. The triggering sweep still supplies the candidate, then the complete group is validated and claimed in one immediate transaction. The idle path additionally requires an idle-drainable member, so a standalone plugin wait is not repeatedly retried just because its thread is idle.The result preserves atomic dispatch while allowing different members' release signals to compose in either order. Explicit Send now, retries, pacing, queue ordering, requeueing, and concurrency behavior remain intact.
There are no wire, schema, CLI, SDK, guide, or documentation contract changes, so no host-daemon protocol bump is needed.
How you verified
Before the complete fix, the real server/DB plugin + core reproduction made zero hook attempts and left both grouped rows queued. The focused regressions also failed for both core/plugin signal orders and the grouped
turn-startingfollow-up. After the fix, both real-path reproductions empty the queue and create exactly one new turn; the focused coverage also proves a still-blocked plugin group does not churn in the idle drain.pnpm exec turbo run test --filter=@bb/server -- --run test/threads/requested-queue-drain.test.ts test/threads/thread-send-dispatch.test.ts test/threads/turn-failed-retry.test.ts test/threads/queue-drain-failure.test.ts test/public/public-thread-stop-runtime.test.ts— 49 passedpnpm exec turbo run test --filter=@bb/server -- --run test/public/public-thread-data.test.ts -t "sends the contiguous lead queued-message group as one turn request"— 1 selected test passedpnpm exec turbo run test --filter=@bb/db -- --run test/data/queued-thread-messages.test.ts test/data/queued-message-waits.test.ts— 44 passedpnpm exec turbo run typecheck --filter=@bb/server --filter=@bb/dbpnpm exec turbo run build --filter=@bb/servergit diff --check@slopcop