Prevent mutual-ask deadlock; make ask timeout configurable#42
Open
iRonin wants to merge 1 commit into
Open
Conversation
Two sessions that issue a reply-waiting `ask` to each other at the same time deadlock: each blocks its turn waiting for a reply the other cannot send while it is itself blocked. Both only unblock at the 10-minute reply timeout. The broker now tracks each session's single outstanding ask edge (asker -> recipient). When an `ask` (expectsReply) arrives whose recipient is already awaiting a reply from the sender, the broker refuses it immediately with a "Mutual ask refused" delivery_failed instead of delivering, so the caller fails fast and can fall back to send/reply. Whichever ask the broker dequeues first wins (FIFO); a plain send is never affected. Edges clear on reply, on disconnect, and via a TTL equal to the reply timeout (so a timed-out ask leaves no phantom edge). The reply timeout (previously a hardcoded 10 minutes in both waitForReply and ReplyTracker) is now configurable via PI_INTERCOM_ASK_TIMEOUT_MS, read by both the extension and the broker so their values stay aligned. Tests: mutual-ask refusal round-trip (reverse ask fails fast; plain send and reply still work; edge clears so a later ask succeeds) and resolveAskTimeoutMs env parsing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #41. Relates to #14 (configurable timeout) and #29 (the single-session concurrent-ask crash, a different failure mode).
Problem
Two sessions that
askeach other at the same time deadlock: eachaskblocks its caller's turn, so neither is idle to answer the other, and both only unblock at the 10-minute reply timeout. See #41 for the full reproduction.Fix
Broker-side cycle guard (the broker is the only component with a global view of all sessions):
asker -> recipient).ask(expectsReply) arrives whose recipient is already awaiting a reply from the sender, refuse it immediately with aMutual ask refuseddelivery_failed. The caller's existing!deliveredpath turns this into an immediate tool error, so it fails fast and can fall back tosend/reply.send(no reply expected) is untouched.replyTomatch), on disconnect, and via a TTL.Configurable reply timeout (relates to #14): the previously hardcoded 10-minute timeout (in both
waitForReplyandReplyTracker) is nowPI_INTERCOM_ASK_TIMEOUT_MS, read by both the extension and the broker so the edge TTL stays aligned with the caller-side timeout (a timed-out ask leaves no phantom edge). Values below 1s are ignored.Tests
mutual ask is refused immediately so two sessions cannot deadlock: A asks B; the reverse B→A ask fails fast withMutual ask refused; a plainsendand areplystill work; after the reply clears the edge, B can ask A. (Asserts the reverse ask fails — the test fails if the guard is removed or is over-broad.)resolveAskTimeoutMs honours PI_INTERCOM_ASK_TIMEOUT_MS and ignores junk.npm test→ 38/38 passing.Notes
No protocol/message-type changes — reuses the existing
delivery_failedpath and theexpectsReply/replyTofields the broker already receives. No new dependencies.