node: make the backend probe timeout configurable - #377
Open
fer-marino wants to merge 3 commits into
Open
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable backend probe timeout (--backend-probe-timeout) to allow command-spawned service backends with slow cold-start times to be successfully advertised. Feedback on the changes suggests replacing time.After with time.NewTimer in the test helper to avoid short-term memory leaks, and updating probeTimeout() to fall back to the default timeout if backendProbeTimeout is zero or negative to handle zero-initialized registries robustly.
aojea
reviewed
Sep 9, 2026
Collaborator
|
sing the CLA and address the review commentds and we are good to go, thanks |
sam-node.yaml's command-spawned MCP services are given a hard-coded 2s
(dhtProbeTimeout) to answer an MCP initialize before the service is
registered but withheld from advertisement ("backend did not answer:
context deadline exceeded"), with no retry observed afterwards.
2s is tighter than the cold-start cost of realistic backends. Measured
directly: a bare `import fastmcp` (Python) takes ~2.7s, and even SAM's
own reference example, `npx -y @modelcontextprotocol/server-everything`,
takes ~4.0s to answer initialize on Windows even warm/cached - SAM's own
documented reference server cannot reliably meet its own default.
Adds a new ServiceRegistry.SetBackendProbeTimeout, wired from a new
--backend-probe-timeout flag (0 keeps the existing 2s default, matching
the convention already used by --dht-max-record-age and similar flags).
Backward compatible: NewServiceRegistry's default is unchanged, and every
existing call site that doesn't pass the new flag behaves exactly as
before (verified: all pre-existing internal/node tests pass unmodified,
including 6 that fail identically on unmodified main - confirmed via git
stash - so unrelated to this change).
Also fixes three of the four node.Options{} construction sites in
cmd/sam-node/main.go that already wire NewServiceRegistry-adjacent flags
(DHTMaxRecordAge et al.) but were missing this one; the fourth (join-only
enrollment path) doesn't call RegisterStaticServices and is out of scope.
Adds TestServiceRegistry_BackendProbeTimeoutIsConfigurable, using a new
slowProbingService test fake that (unlike the existing probingService)
actually respects context deadlines, so it can demonstrate: the same slow
backend fails to advertise under the default timeout and succeeds once
given more time.
…ix timer leak in test - aojea: renamed dhtProbeTimeout to defaultDHTProbeTimeout for clarity now that there's also a configurable per-registry value. - gemini-code-assist: probeTimeout() now falls back to defaultDHTProbeTimeout when backendProbeTimeout is <= 0, so a zero-initialized ServiceRegistry (e.g. newServiceRegistryForTest's struct literal, which bypasses NewServiceRegistry) behaves the same as a properly constructed one instead of timing out every probe immediately. - gemini-code-assist: slowProbingService.Probe now uses time.NewTimer with a deferred Stop() instead of time.After, avoiding the short-term timer leak when the context is cancelled before the delay elapses. All internal/node tests pass, including the new TestServiceRegistry_BackendProbeTimeoutIsConfigurable.
fer-marino
force-pushed
the
fix/configurable-backend-probe-timeout
branch
from
September 9, 2026 09:38
8384722 to
81db9e9
Compare
aojea
reviewed
Sep 9, 2026
…etter pair Config is populated once, at construction, everywhere else in this package (see internal/node/options.go) - this had grown a setter and a getter instead. NewServiceRegistry now takes backendProbeTimeout directly and defaults it in the constructor if <= 0; the timeout is never mutated afterwards, so reading the plain field needs no lock.
Collaborator
|
I prefer to squash the commits, having the intermediate steps merged are not useful because they are discarded in commits later |
Contributor
Author
|
Sure. That is usually my preference too |
aojea
reviewed
Sep 9, 2026
| } | ||
|
|
||
| // Default applies default values to Options if they are not specified. | ||
| func (o *Options) Default() { |
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
2 participants
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 #376.
sam-node.yaml's command-spawned MCP services are given a hard-coded 2s (dhtProbeTimeout) to answer an MCP initialize before the service is registered but withheld from advertisement ("backend did not answer: context deadline exceeded"), with no retry observed afterwards.
2s is tighter than the cold-start cost of realistic backends. Measured directly: a bare
import fastmcp(Python) takes ~2.7s, and even SAM's own reference example,npx -y @modelcontextprotocol/server-everything, takes ~4.0s to answer initialize on Windows even warm/cached - SAM's own documented reference server cannot reliably meet its own default.Adds a new ServiceRegistry.SetBackendProbeTimeout, wired from a new --backend-probe-timeout flag (0 keeps the existing 2s default, matching the convention already used by --dht-max-record-age and similar flags). Backward compatible: NewServiceRegistry's default is unchanged, and every existing call site that doesn't pass the new flag behaves exactly as before (verified: all pre-existing internal/node tests pass unmodified, including 6 that fail identically on unmodified main - confirmed via git stash - so unrelated to this change).
Also fixes three of the four node.Options{} construction sites in cmd/sam-node/main.go that already wire NewServiceRegistry-adjacent flags (DHTMaxRecordAge et al.) but were missing this one; the fourth (join-only enrollment path) doesn't call RegisterStaticServices and is out of scope.
Adds TestServiceRegistry_BackendProbeTimeoutIsConfigurable, using a new slowProbingService test fake that (unlike the existing probingService) actually respects context deadlines, so it can demonstrate: the same slow backend fails to advertise under the default timeout and succeeds once given more time.