[FIXED] A request through a service import with no interest should return no reponders.

[derekcollison]

Since service imports are internal callbacks with no signaling for delivered status, when sending through a service import with no interest in the exporting account, the requestor would simply timeout.

Signed-off-by: Derek Collison derek@nats.io

[wallyqs]

LGTM

[kozlovic]

I would have to look at this a bit more for message tracing to be sure that we don't go negative, but I can't do this today. If you know it is ok, then disregard my comment.

[kozlovic]

Would have the changes been too big to make msgHandler (sub.icb) return a boolean? I am guessing yes, but just checking.

[derekcollison]

I just tried that and did not like the way the code was being changed, too much risk there IMO. So looking for alternative.

[kozlovic]

Hum... not sure if that will always works. For instance, we bumped the sub.nm only in the if !traceOnly { case in deliverMsg. Not sure if I can't not come up with a test case that would cause this to become negative...

[derekcollison]

• Thinking some more on this, under concurrent requests this could be racy and not report no responders. It won't go negative imo @kozlovic bit it could appear to be the same value or go up under concurrent load. Will take another look this morning.

[kozlovic]

It won't go negative imo

@derekcollison It will. Here is the changes I made to your PR, with printing the value of sub.nm before/after decrement, and replace the test's request with sending a request but as a trace-only message:

diff --git a/server/accounts.go b/server/accounts.go
index a0a3b08e..88a6ce59 100644
--- a/server/accounts.go
+++ b/server/accounts.go
@@ -2089,7 +2089,9 @@ func (a *Account) addServiceImportSub(si *serviceImport) error {
                if didDeliver := c.processServiceImport(si, acc, msg); !didDeliver {
                        // We did not deliver to anything, so we will decrement the subs num messages.
                        sub.client.mu.Lock()
+                       was := sub.nm
                        sub.nm--
+                       fmt.Printf("sub.nm was %v now %v\n", was, sub.nm)
                        sub.client.mu.Unlock()
                }
        }
diff --git a/server/accounts_test.go b/server/accounts_test.go
index 4a31fe8d..d6db9423 100644
--- a/server/accounts_test.go
+++ b/server/accounts_test.go
@@ -3713,6 +3713,18 @@ func TestAccountServiceImportNoResponders(t *testing.T) {
        nc := natsConnect(t, s.ClientURL(), nats.UserInfo("accImp", "accImp"))
        defer nc.Close()
 
-       _, err := nc.Request("foo", []byte("request"), 250*time.Millisecond)
-       require_Error(t, err, nats.ErrNoResponders)
+       traceSub := natsSubSync(t, nc, "trace.subj")
+
+       replySub := natsSubSync(t, nc, nc.NewInbox())
+
+       msg := nats.NewMsg("foo")
+       msg.Data = []byte("request")
+       msg.Header.Set(MsgTraceDest, traceSub.Subject)
+       msg.Header.Set(MsgTraceOnly, "true")
+       msg.Reply = replySub.Subject
+
+       nc.PublishMsg(msg)
+
+       traceMsg := natsNexMsg(t, traceSub, time.Second)
+       t.Logf("trace=%s", traceMsg.Data)
 }

and here is the output:

=== RUN   TestAccountServiceImportNoResponders
sub.nm was 0 now -1
    accounts_test.go:3729: trace={"server":{"name":"NDMTM4KGAXTTJCYSCX7SOL6AWUTD6S3ND4KMAR6E6WRZYJZP5HVCYQKU","host":"0.0.0.0","id":"NDMTM4KGAXTTJCYSCX7SOL6AWUTD6S3ND4KMAR6E6WRZYJZP5HVCYQKU","ver":"2.11.0-dev","jetstream":false,"flags":0,"seq":13,"time":"2025-02-20T14:04:37.965243Z"},"request":{"header":{"Nats-Trace-Dest":["trace.subj"],"Nats-Trace-Only":["true"]},"msgsize":71},"events":[{"type":"in","ts":"2025-02-20T07:04:37.964443-07:00","kind":0,"cid":7,"acc":"accImp","subj":"foo"},{"type":"si","ts":"2025-02-20T07:04:37.964578-07:00","acc":"accExp","from":"foo","to":"foo"}]}
--- PASS: TestAccountServiceImportNoResponders (0.01s)
PASS
ok  	github.com/nats-io/nats-server/v2/server	1.536s

[derekcollison]

I will switch msgHandler to return a bool, alot of changes to make but the correct approach versus my first attempt.

[kozlovic]

I will switch msgHandler to return a bool, alot of changes to make but the correct approach versus my first attempt.

Or, you could maybe add a new field in c.pa to track that, but keep in mind that service imports can be chained (I believe), so it would have to be some sort of counter instead of a boolean. Then in in deliverMsg(), where now you check for sub.nm, we could instead check if that new c.pa.dlvCounter (bad name, but you know what I mean) is positive or not.

[kozlovic]

.. but I guess it would be the same since you would still need to go through all handlers to actually update this counter, which then better to have simply msgHandler() return a boolean.

[derekcollison]

ok I tried a different approach based on @kozlovic suggestion which I think works well. Will see if all tests are green.

Could @neilalexander and @kozlovic take another look?

[neilalexander]

LGTM

[kozlovic]

LGTM