Skip to content

fix(queue): harden busy-queue ordering and identity - #225

Open
Marenz wants to merge 5 commits into
grinev:mainfrom
Marenz:feat/queue-identity-safety
Open

fix(queue): harden busy-queue ordering and identity#225
Marenz wants to merge 5 commits into
grinev:mainfrom
Marenz:feat/queue-identity-safety

Conversation

@Marenz

@Marenz Marenz commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #219 that hardens the busy-queue against Telegram ordering and OpenCode delivery-identity races.

  • Keeps delayed albums ahead of later commands, callbacks, and media while preserving their captured session and project.
  • Scopes response modes and self-input suppression to parent message IDs so failures and identical external text cannot affect another response.
  • Uses bounded same-UUID admission retries, treats 409 as already admitted, rejects mismatched acknowledgements, and retains state after ambiguous transport failures.
  • Prevents unrelated interactions from being displaced by /settings while a task is busy.
  • Preserves the 20 MiB raw-media cap, including rich photos and text files.

@grinev

grinev commented Sep 7, 2026

Copy link
Copy Markdown
Owner

@Marenz thanks for the follow-up work on #219 — the per-message identity idea is the right direction. Unfortunately the branch cannot be merged yet: it does not survive the first message in a real chat, and there are a few other issues.

Blockers

1. messageID is a plain UUID, but OpenCode expects a msg-prefixed id.
On a fresh session, every prompt fails immediately with Failed to send request to OpenCode.:

"name": "BadRequest",
"message": "Expected a string starting with \"msg\", got \"e9474d0e-d477-46bf-8533-df4421dfccbd\" at [\"messageID\"]"

Verified manually against OpenCode Server 1.18.29. Three places generate the id with randomUUID():

  • src/bot/handlers/prompt.ts:517promptAsync({ messageID }) — every prompt
  • src/bot/callbacks/command-catalog-callback-handler.ts:288session.command({ messageID })
  • src/app/managers/prompt-queue-manager.ts:59 → queue item id → session.prompt({ id, delivery: "queue" })

So text, voice, albums and the whole queue path are broken. The test suite does not catch this because the SDK is mocked everywhere and nothing asserts the id format — please add a test that checks the id we actually send.

2. An album is silently dropped when there is no active session.
captureTarget() (media-group-handler.ts:290) stores sessionId: getCurrentSession()?.id ?? null, so null when no session exists. The check in prompt.ts:238-248 runs before the session is created and compares strictly:

currentSession?.id !== options.target.sessionId  // undefined !== null → true

Steps: /new, then send an album. The bot replies "downloading files" and then stays silent forever. Suggested fix: (currentSession?.id ?? null) !== options.target.sessionId, plus a test with sessionId: null. The existing test only covers a non-null session id.

3. A legitimate context-change refusal is invisible to the user.
Same block (prompt.ts:244) only writes a logger.warn, and flushBatch ignores the return value of processPrompt. When the user switches project while the album is downloading, nothing is sent to the chat. Please reply with an explicit message.

4. The branch conflicts with main (src/bot/handlers/prompt.ts, tests/bot/handlers/prompt.test.ts, tests/bot/commands/commands.test.ts). A rebase is needed.

Major

5. The user is no longer told when a prompt fails to reach OpenCode.
prompt.ts:553-559 replaced markIdle() + clearRun() + bot.prompt_send_error with two logger.warn calls. Since markBusy() is already set at line 509, a transport failure leaves the session marked busy with no message at all; the user only recovers through reconcileBusyStateNow on the next update (and only if the server answers again), otherwise /abort is the only way out. The queued path has bounded retries, the foreground path has nothing. The test that guarded this behaviour (still notifies the user when promptAsync rejects before the run starts) was deleted. Please add a retry/timeout for the foreground path or restore the notification.

6. "Remove from queue" is now a race.
Admission starts right after enqueueing (tryEnqueuePromptIfBusyvoid dispatchNextQueuedPrompt()). If the user presses the remove button while admission is in flight, finishAdmission returns false and the bot answers "Message removed from the queue", but OpenCode has already accepted it and will run it (prompt-queue-dispatch.ts:216-224). In practice the item also disappears from the keyboard almost instantly, so the queue UI (buttons, the limit of 5, queue.full) is close to decorative now.

7. The ordering middleware blocks every callback in the chat while an album is downloading.
telegramInputOrderMiddleware (telegram-input-order-manager.ts:80-108) calls waitForPending(chatId) for any update without ctx.message — that is, for all callback queries — and waits for every deferred album message, with no timeout. While a 10-file album is being downloaded and encoded, button presses hang, including answers to OpenCode questions and permission requests. Telegram expires a callback query after about a minute, so answerCallbackQuery will then fail. There is also no emergency release: the only release() is in the finally of flushBatch, and the early return at media-group-handler.ts:208 sits before the try. Please add a timeout and consider not blocking callbacks at all.

8. /settings during a busy session works only once.
The guard now allows /settings when the current state is an open settings menu (interaction-guard-decision.ts:41-45), but replyWithInlineMenu starts with interactionManager.tryStart(), which returns null when a state already exists, so the user gets interaction.blocked.finish_current instead of the menu (inline-menu.ts:107-118). The new allowance cancels itself on the second /settings.

9. No minimum OpenCode Server version is documented or checked. delivery: "queue" and a client-supplied message id both depend on server support. Worth stating the minimum version in README/PRODUCT.md.

Minor

10. This is a product change, not only hardening. The bot stops owning the queue and pushes prompts to OpenCode immediately. That makes MAX_QUEUED_PROMPTS, the remove buttons and the {count}/{max} counter in queue.added (dropped in all 10 languages) mostly meaningless. This needs a decision from the maintainer, not just a code review.

11. Changes outside the stated scope. progress.compact.finished_header was rewritten in all 10 locales ("Finished Work" → "Response complete"). The reasoning is understandable, but it is not mentioned in the PR description.

12. Dead code. After the switch to registerMessage/consumeMessage, the old register(), consume(), prune(), SUPPRESSION_TTL_MS and entriesBySession are unused in src/ and only referenced by tests (external-input-suppression-manager.ts:1-52).

13. Unused parameter. consumeMessage(sessionId, messageId, _fallbackText) never uses the third argument; if a text fallback is not planned, drop it from ConsumeSuppressedInput too.

14. Session-wide cleanup undoes the per-message scoping. On session.idle, event-subscription-service.ts:1178-1180 calls clearPromptResponseMode(sessionId) without a message id and clearSession(sessionId). If OpenCode reports idle between items of its own queue, the TTS mode and echo suppression of prompts that are still pending are lost — exactly what this PR tries to separate. Clearing by parentMessageId would be safer.

Marenz and others added 5 commits September 7, 2026 14:42
Bind Telegram-origin suppression to message IDs instead of text so identical external input is never hidden. Expire missed-event entries on wall clock time and clear them across session and runtime teardown.
Track text and TTS delivery by session and parent message identity. A failed response can now retire only its own mode without stripping TTS from another queued response.
Defer later commands, callbacks, voice, audio, photos, documents, and albums behind earlier album batches. Capture the original project and session so a delayed album cannot land in a newly selected context.
Allow /settings only when no unrelated interaction owns input, while preserving the active settings flow. Update queue wording and documentation for durable OpenCode admission semantics.
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
@Marenz
Marenz force-pushed the feat/queue-identity-safety branch from 4f57e49 to 7b2bad2 Compare September 7, 2026 12:52
@Marenz

Marenz commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Restored the bot-owned queue and fixed the applicable review points. Full checks pass. Tested against real OpenCode 1.18.29: plain UUIDs reproduce the 400; msg_ prompt IDs are accepted. Command completion and end-to-end Telegram behavior remain untested live.

@grinev

grinev commented Sep 10, 2026

Copy link
Copy Markdown
Owner

@Marenz thanks for the follow-up — this is a clear improvement over the previous revision. The msg_-prefixed id, the explicit context-change reply, unblocking callbacks, re-opening /settings, and the removal of the dead code all look good. typecheck, lint and the full test suite (1761 tests) pass on my side.

There are still a few things I would fix before merging.

Major

  1. The input-order wait has no timeout. telegramInputOrderManager.waitForEarlier returns a promise that is only settled by release(), which happens in the finally of flushBatch after files are downloaded and processUserPrompt runs. If a download hangs, every later text/voice/photo/document message in that chat is stuck forever. Callbacks were unblocked, but messages still have no deadline — could you add a timeout (and release on it)?

  2. A failed foreground prompt leaves the session busy. In onError the markIdle() / clearRun() calls were removed, so after a transport failure the session stays busy until the next update happens to reconcile it, otherwise the user needs /abort. Note that safeBackgroundTask also calls onError for synchronous throws that happen before the request is sent, when the prompt was definitely not accepted. The new test locks in this behaviour, but I think we should confirm it is what we want.

  3. The captured session/project does not survive the queue. media-group-handler.ts passes sessionId/directory to tryEnqueuePromptIfBusy, but PromptQueueManager.add drops them and dispatchNextQueuedPrompt never passes a target. So the context check only guards the delayed-but-not-queued album path, not the queued one — which is not what the PR description promises. Either wire target through the dispatch or remove the unused parameters.

Minor

  • Docs contradict the code. PRODUCT.md:56 still says prompts are admitted to "OpenCode's ordered session queue", but the final commit restored the bot-owned queue. Please update the wording.
  • Missing tests for the new paths. There is no coverage for the target refusal branch (bot.prompt_send_error) or for an album captured with sessionId: null, which was one of the items asked for earlier.
  • Response modes can leak. They are no longer cleared on session_idle/session_error, and unlike suppression entries they have no TTL. If a completion arrives without parentID, the entry lives until the session is switched.
  • The command path is still unverified. Suppression is keyed by the messageID sent to session.command. If that endpoint ignores the client-supplied id, the bot's own command text will come back as "external user input". Worth a live check before merge.

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