Skip to content

fix(approvals): save and wait on a request before the chat shows it - #186

Merged
httpsphl merged 1 commit into
mainfrom
fix/approval-order
Oct 5, 2026
Merged

httpsphl merged 1 commit into
mainfrom
fix/approval-order

Conversation

@httpsphl

@httpsphl httpsphl commented Oct 5, 2026

Copy link
Copy Markdown
Owner

Found while looking at a red CI on httpsphl/botloft#184. words_made_of_one_element_per_letter_keep_their_spaces failed on Ubuntu with approvals.answer → -32002 approval … not found. The rerun passed.

Cause

approvals::open called items::add, which saves the chat item and emits chat.item, before insert_approval. The answer's oneshot was also registered in daemon.approvals only after open returned. That leaves two windows:

  • The app answers before the approval row exists, and gets not found.
  • The app answers after the row exists but before the waiter is registered. The store is settled, but nobody tells the waiting tool, so the bot hears "expired" only at the timeout, even though the owner allowed it.

Fix

open now does these steps in this order:

  1. builds the chat item;
  2. saves it and the approval;
  3. registers the waiter;
  4. then items::announce.

Spec 10.1 records the order.

Test

tests/approval_order.rs answers 20 suggestions synchronously the moment their chat.item event arrives. Each answer must find the request, and the bot must hear it within 5 s. Locally, this test does not reproduce the race on the old code, because the emit and the insert run back to back on one thread. It guards the order, as CI's Linux runner showed it can break.

approvals::open announced the approval's chat item before saving the
approval, and the answer's channel was registered after that. A fast app
could answer a request that was not in the store yet (not found, seen on
CI's Linux runner in a browser test), or answer before the daemon waited
on it, so the bot would only hear back when the request expired. Now the
chat item and the approval are saved, the answer is waited on, and only
then does the app hear of it.
@httpsphl
httpsphl enabled auto-merge October 5, 2026 00:56
@httpsphl
httpsphl merged commit 7b9c801 into main Oct 5, 2026
4 checks passed
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.

1 participant