Skip to content

fix: deliver stranded follow-ups and surface message-queue metadata - #197

Open
OchnikBartek wants to merge 2 commits into
mainfrom
fix/181
Open

fix: deliver stranded follow-ups and surface message-queue metadata#197
OchnikBartek wants to merge 2 commits into
mainfrom
fix/181

Conversation

@OchnikBartek

Copy link
Copy Markdown
Member

Summary

Three fixes to the message queue that an event-driven producer hits and a human
typist rarely does: a follow-up queued as a run ends is no longer stranded, the
source in a message's metadata now reaches the model, the logs and the span,
and the queue is bounded so a refused submission fails loudly instead of piling
up. The public injection surface asked for in the issue is deliberately not part
of this change.

Related Issue

Refs #181.

Added

  • MessageQueue.discard_follow_up(keep=...) — removes pending follow-ups,
    sparing the ones a predicate keeps, so a caller can prune what a cancelled run
    made stale without dropping the rest.
  • queued_source() — returns a message's sanitized metadata["source"] label,
    reduced to \w, ., :, - and truncated to 32 characters because it is
    interpolated into the prompt label the model reads.
  • QueueFullError and MessageQueue(max_pending=...), defaulting to
    DEFAULT_MAX_PENDING (100 per priority); None removes the cap.
  • Docs: source labels, backpressure and cancellation pruning in
    docs/advanced/message-queue.md; queued_source and QueueFullError in
    docs/api/message-queue.md.

Changed

  • _agent_stream_worker in apps/cli/screens/chat.py now drains follow-ups in
    its finally block. A message enqueued after the post-run drain still saw the
    run as active, so it landed as a follow-up that nothing else would deliver —
    invisible until some later run happened to end.
  • On a cancelled run the worker discards only locally typed follow-ups; ones
    carrying a source survive and start a fresh turn. Previously every pending
    follow-up was discarded.
  • format_steering / format_follow_up now render the source:
    [steering via slack] …, [follow-up via jira] …, and - [via monitor] … per
    line in a batch. A message with no source is formatted exactly as before, so
    a locally typed follow-up still reaches the transcript verbatim.
  • MessageQueueCapability.before_model_request and run_with_queue log each
    delivered batch and set pydantic_deep.message_queue.{steering,follow_up}.{count,sources}
    on the enclosing span.
  • steer() / follow_up() route through a shared _put() that enforces the cap
    and logs the enqueue; both can now raise QueueFullError.
  • The TUI's >>/follow-up submit path catches QueueFullError and reports it as
    a notification instead of failing the message handler.
  • The monitor react sink in pydantic_deep/features/monitoring/toolset.py catches
    QueueFullError and logs it — MonitorManager._emit swallows sink exceptions,
    so the batch would otherwise vanish silently.

Testing

  • Added tests/test_tui.py::TestQueueDrainWhenIdle — three tests covering the
    stranded follow-up, the cancelled-run split between external and typed
    follow-ups, and the full-queue notification. Verified they fail against the
    unmodified chat.py: all three fail, all three pass with the change.
  • Added ~25 tests to tests/test_message_queue.py for the capacity bound,
    discard_follow_up, queued_source sanitizing/truncation, the source labels,
    and end-to-end label delivery through the capability and run_with_queue.
  • Added tests/test_monitoring.py::TestMonitorToolset::test_full_queue_drops_the_batch_with_a_warning.
  • make test — 2828 passed, coverage 100.00%.
  • make typecheck — 0 errors; make typecheck-mypy — clean across 210 files;
    ruff check / ruff format --check clean; make docs builds with no warnings.

Notes for Reviewers

  • The open question from the issue thread — should externally injected follow-ups
    survive a cancellation — is answered here as yes, with the discriminator
    being the presence of metadata["source"]: no source means typed in the TUI.
    It is a two-line change in the finally block if you want it the other way.
  • The default cap of 100 per priority is a behaviour change for any caller that
    enqueues more than that without the agent consuming; max_pending=None restores
    the old unbounded behaviour.
  • test_follow_up_arriving_after_the_post_run_drain_still_runs lands its message
    by patching _notify_degraded_mcp, the last call before the idle drain. That is
    the only deterministic seam inside the window; if the order in finally is
    changed the test silently stops covering the race. It is called out in a comment
    in the test.
  • Nothing here commits to an injection API. discard_follow_up and
    queued_source are library-level and reusable whichever surface is chosen,
    which is why the cancellation logic lives in pydantic_deep/ rather than in
    apps/cli.

@OchnikBartek
OchnikBartek requested a review from DEENUU1 August 3, 2026 13:53
@OchnikBartek OchnikBartek self-assigned this Aug 3, 2026
@github-project-automation github-project-automation Bot moved this to Triage in Vstorm OSS Aug 3, 2026
@coveralls

coveralls commented Aug 3, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 30820356581

Coverage remained the same at 100.0%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 61 of 61 lines across 2 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 6971
Covered Lines: 6971
Line Coverage: 100.0%
Coverage Strength: 1.0 hits per line

💛 - Coveralls

@DEENUU1 DEENUU1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work — a careful, well-scoped take on the two fixes Douwe flagged plus the queue bound, and it deliberately stays out of the still-open injection-API decision. The discard_follow_up(keep=...) split for external-vs-typed on cancel is exactly what the reporter asked for, the sanitizer is a sensible defensive choice, and the tests are the right level of rigour — verifying they fail against the unmodified chat.py and calling out the fragile _notify_degraded_mcp seam in a comment.

One thing I'd like your read on before this goes in: the new idle drain in the finally and the goal-loop scheduling at chat.py:1268 don't coordinate, so a goal turn can schedule two runs. Details inline. Everything else is fine.

Smaller, non-blocking: external steering still gets dropped on cancel with only a local notify (chat.py:1318), while external follow-ups now survive. Defensible — steering means "before the next LLM call" and there isn't one after a cancel — but it's the same silent-to-the-sender drop you're fixing for follow-ups, on the monitor/urgent path. Worth a sentence in the docs so a bridge author knows steering-on-cancel is best-effort.

Comment thread apps/cli/screens/chat.py

# A message queued after the post-run drain still saw the run as
# active, so it landed as a follow-up nothing else will deliver.
if not _follow_up_scheduled:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here's the case I'm unsure about: a run that finishes normally with a goal active. The goal continuation is scheduled back at chat.py:1268 — but only because _follow_up_scheduled was still False there. Then a message strands in the window this block covers, so this drain fires too, sets _follow_up_scheduled = True locally (too late — the goal already read the old value) and schedules _run_agent.

Now both _continue_goal and _run_agent(stranded_text) are on call_later. _continue_goal runs first, awaits the evaluator, and during that await _run_agent(stranded) starts a turn; when the evaluator returns unmet, _continue_goal calls _run_agent again and clobbers app.agent_task. Two concurrent turns streaming into the same widgets.

It's narrow — needs an active goal plus a message landing in that sub-second window — but a bridge is exactly what lands messages there. Simplest guard I see: skip this drain when a goal continuation is already pending, or move the goal decision into the finally next to this drain so one place owns "schedule the next turn." Did you already rule this out?

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

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

3 participants