Skip to content

emrg: queue messages sent while tool loop busy — inject at round boundary (P1, rant 2026-08-10T21:55:37) - #655

Merged
argszero merged 1 commit into
masterfrom
feature/session-queue-injection
Aug 10, 2026
Merged

emrg: queue messages sent while tool loop busy — inject at round boundary (P1, rant 2026-08-10T21:55:37)#655
argszero merged 1 commit into
masterfrom
feature/session-queue-injection

Conversation

@argszero

Copy link
Copy Markdown
Owner

Summary

P1 of the 'tool loop 进行中发新消息 — daemon 排队注入' rant (2026-08-10T21:55:37, design doc mid-turn-input-queue, v0.2.11, 9 review rounds). Messages sent while a session's tool loop is busy are now queued per session and injected at the next round boundary — aligned with OpenAI codex steer_input semantics: after the current round (LLM request + ALL tool executions) ends, before the next LLM request. Tools are never interrupted, messages are never lost.

P2 (GUI) / P3 (TUI) client sides are separate future rants — this PR is daemon core only (protocol frames are already broadcast, clients just render them).

Changes (emrg/server/daemon.py)

  • New state: _session_pending: dict[str, list[tuple[TaskRequest, bool]]] — per-session FIFO of (req, allow_tools).
  • A — busy branch: construct req + allow_tools first, then on busy append to the queue + broadcast task_queued (with position) instead of returning the old "session busy" error.
  • B — round-boundary injection: new _inject_pending_messages() helper drains the queue with an atomic pop() (messages appended during injection land in a fresh list and are injected next round — nothing dropped), persists each injected message via append_message (auto-compact safe), broadcasts steer_committed.
  • C — round budget: for rangewhile True with a budget check; stop / Case 3 / loop-exhaust all re-check the queue and inject + continue (injection rounds do NOT consume the round budget; round_num += 1 only on tool rounds). Assistant replies are appended to the local message list before injection so the LLM context stays coherent.
  • D — wrapper finally: _run_tool_loop_locked pops the queue on exit — clean end → queued_requeue (with request_ids, clients auto re-send); cancel / exception / disconnect → queued_cancelled. A caught cancel (which returns normally from the loop) is detected via cancel_event.is_set() so it never auto-requeues.
  • E — Ask mode: allow_tools rides in the queue tuple; an injected Ask message forces an empty tool set for its round (force_ask latch).
  • F — clear/delete: clear_session / delete_session drop the session's pending queue + broadcast queued_cancelled.

Tests (tests/test_ws_e2e.py)

  • test_task_queued_instead_of_busy_error — busy task → task_queued (position), not session busy; message injected into the turn (steer_committed), nothing lost.
  • test_pending_injected_at_round_boundary_after_tools — B's message queued during A's tool execution is injected after the tool completes; round 2's LLM request contains it; tool_end precedes injection.
  • test_pending_ask_injects_empty_tools — queued Ask message → round uses tools=[].
  • test_queued_requeue_on_normal_end — turn ends with a message still queued (race window) → queued_requeue with request_ids → client re-send works.
  • test_queued_cancelled_on_cancel — cancel drops the queue → queued_cancelled.
  • test_clear_session_drops_pending — clear_session pops the queue + broadcasts queued_cancelled.
  • test_per_session_isolation — a task on a different session runs immediately; the busy session queues.

pytest: 681 → 687 (README.md / README.cn.md / Agent.md counts synced). GUI tests unchanged: 179 (P1 is daemon-only).

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle

Verified locally before push: pytest 687/687 green (incl. 7 new queue-injection e2e tests), import check + emrg --help OK, GUI 179/179 unchanged. Implementation covers all design-doc change points A–F (mid-turn-input-queue v0.2.11): busy→queue, round-boundary injection, budget-safe continuation (stop/Case 3/exhaust re-drain), cancel-aware wrapper finally (queued_requeue vs queued_cancelled), Ask empty-toolset, clear/delete queue drop.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle

Reviewed the full diff (579+/35-, 5 files) after CI SUCCESS (run 31396944557, 1m24s). Design is sound and matches codex steer_input semantics:

  • A: busy branch constructs req+allow_tools first, then queues with task_queued (position) — the old session busy error path is fully replaced.
  • B: _inject_pending_messages uses atomic pop() so messages arriving during injection land in a fresh list (never dropped); persists via append_message (auto-compact safe) + steer_committed broadcast.
  • C: round budget correctly treats injection rounds as free (round_num only increments on tool rounds); budget-exhausted branch re-drains the queue before falling back to the max-rounds error.
  • D: wrapper finally distinguishes normal end (queued_requeue + request_ids) from cancel/error/disconnect (queued_cancelled); cancel_event.is_set() check prevents auto-requeue of a user-cancelled turn.
  • E/F: force_ask empty-toolset latch and clear/delete queue drop both covered.

Tests: 7 new e2e tests (681→687) exercise both positive (queued→injected→steer_committed, tool_end precedes injection) and negative states (cancel→queued_cancelled, normal-end race→queued_requeue, per-session isolation). Doc counts synced. GUI 179 unchanged (P1 daemon-only). No issues found.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle 20260810-221539 (3rd consecutive ✅, no ❌ interleaved)

Re-verified: head 8b6b908 unchanged; diff 5 files +579/−35 (daemon.py queue-injection A–F + 6 e2e tests + doc counts 681→687); old 'session busy' error path fully removed (only comment/test-doc references remain); CI test SUCCESS (run 31396944557). Merge condition satisfied.

@argszero
argszero merged commit 535efd2 into master Aug 10, 2026
1 check passed
@pm25coder

Copy link
Copy Markdown
Contributor

I tested PR #655 end-to-end on Windows (fresh checkout of feature/session-queue-injection @ 8b6b908) — all green:

  • pytest: 687 collected (632 passed + 55 pre-existing skips) — matches the claimed 681→687 bump
  • GUI: 179 tests (178 pass, 0 fail, 1 cancelled — the cancelled one is the pre-existing integration flake, same as on master)
  • doc guard: 3/3 PASS (tests/test_doc_counts.py)
  • import + CLI: from emrg.client.app import run_client OK, python -m emrg --help OK
  • CI: test run 31396944557 PASS on the branch

Code review notes (all non-blocking):

  1. Design is clean. The atomic pop() drain in _inject_pending_messages with fresh-list landing is a good answer to the append-during-inject race; _run_tool_loop_locked's finally covering requeue-vs-cancelled via cancel_event.is_set() is the right discriminator; the force_ask latch correctly propagates Ask mode into an injected round. The tests are genuinely discriminating (the tool_end-before-injection timing assertion in test_pending_injected_at_round_boundary_after_tools would fail if tools were interrupted).

  2. Backward-compat / silent-loss window — the one thing worth a decision: before P1, a busy session returned "session busy" (client showed feedback). After P1, an old client (v0.2.22-era GUI/TUI, which doesn't know task_queued/queued_requeue/queued_cancelled) will: queue silently → ignore the unknown frames → at turn end get queued_requeue (ignored) → the message is dropped with zero user-visible feedback. If the release carrying P1 ships before P2/P3 land, busy-session messages vanish silently for existing installs. Options: land P2/P3 in the same release, or keep a "busy" error fallback for connections that haven't acked the queue protocol.

  3. Session lock is now unbounded (behavior change worth noting): with a non-empty queue at budget-exhaust, round_num resets to 1, so a client that keeps queueing can hold the session busy lock indefinitely — the old code was bounded by max_tool_rounds + busy-error. A total-round or queue-cap guard in a later slice (aligning with emrg: GUI multi-session P6 finalize — cancel-before-close + cap-20 prompt + projectPath validation #643's cap-20 pattern) would bound worst-case lock time.

  4. Queue growth: _session_pending[sid] is unbounded while a turn is stuck (e.g. LLM hang). A per-session queue cap (drop-oldest + notify, or error) would be a cheap hardening.

  5. Protocol frames undocumented: task_queued / steer_committed / queued_requeue / queued_cancelled are only in code; the docs just bump counts. Since P2/P3 clients must implement these, a short frame-contract note in Agent.md would help both the client slices and future contributors.

Thanks for the careful round-boundary semantics — this is a solid P1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants