[FIXED] Deliver service imported messages to stream imports of other accounts - #8361
[FIXED] Deliver service imported messages to stream imports of other accounts#8361Adel-Ayoub wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d736335c0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // If this message was a service import, deliver only to stream imports | ||
| // of other accounts. Delivering back to the account it came from could | ||
| // loop the message or siphon off queue deliveries. | ||
| if flags&pmrMsgImportedFromService != 0 && sub.client.acc == c.acc { |
There was a problem hiding this comment.
When service imports are chained, c.acc is still the original publisher's account, not the account whose service import is currently being processed. For example, if A imports a service from B, B imports that service from C, and B also stream-imports the same subject from C, this check compares B's shadow subscription against A and lets the request flow back into B through the stream import, defeating the loop/queue-siphon guard for intermediate importers; the duplicated queue-path predicate below has the same issue. The guard needs the current importing account from processServiceImport, rather than the producer client's account.
Useful? React with 👍 / 👎.
…accounts Signed-off-by: Adel-Ayoub <adelayoub.maaziz@gmail.com>
d736335 to
69ae010
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ae010204
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // If this message was a service import, deliver only to stream imports | ||
| // of accounts the message has not passed through yet. Delivering back | ||
| // to one of those could loop the message or siphon off queue deliveries. | ||
| if flags&pmrMsgImportedFromService != 0 && c.accOnServiceImportPath(sub.client.acc) { |
There was a problem hiding this comment.
When A request/reply service-imports svc from B and an unrelated C stream-imports svc from B, this relaxed check now delivers the service request to C's shadow subscription and deliverMsg marks didDeliver. In that configuration C only has the stream import, while the generated _R_... reply subject is a response import in B's account, so C cannot satisfy the reply; with no B responder, or when the queue copy below randomly picks C's shadow queue sub over a real B responder, the caller loses the no-responders signal and waits for a timeout. Please keep these shadow deliveries from satisfying service-import requests with replies.
Useful? React with 👍 / 👎.
1 participant
Problem
With three accounts, where A service-imports from B and C stream-imports from B, a message published from A reaches subscribers in B but never the subscriber in C when both clients are connected to the same server. The same config works across a cluster, since the message is forwarded over the route before the check and the service import flag is not carried on the wire. The receiving server then treats it as a normal routed message and delivers it to the stream import.
The drop comes from 705f8b6 ("Do not forward service import messages to a stream export"), which skips every stream import shadow subscription while processing a service-imported message. That guard protects an account that imports both a stream and a service from the same exporter, where the message could loop back into the importer or its shadow queue subs could grab deliveries meant for the exporter (both covered by TestServiceAndStreamStackOverflow). Keying the skip on the flag alone also suppresses shadow subscriptions of unrelated accounts that can't form such a cycle.
Fix
Narrow the guard at both spots in processMsgResults so it only skips shadow subscriptions of accounts the message has already passed through, that is the publisher's account plus every hop on the service import chain (c.pa.psi). Same-account behavior is unchanged and TestServiceAndStreamStackOverflow still passes. Stream imports of unrelated accounts now receive the message on a single server the same way they already do across a route.
Test
Added TestAccountServiceImportDeliveredToStreamImportOfOtherAccount with the three account setup from the issue. It fails on main with the subscriber in C timing out, and passes with the fix. Also added TestAccountChainedServiceImportDeliveredOnceToStreamImports for chained imports (A imports svc from B, B implements it by importing from C and also stream-imports from C), where an intermediate account must not receive a duplicate through its own stream import.
Ran locally:
Resolves #8338