node: make the backend probe timeout configurable

[fer-marino]

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.

[google-cla]

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]

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]

sing the CLA and address the review commentds and we are good to go, thanks

[aojea]

I prefer to squash the commits, having the intermediate steps merged are not useful because they are discarded in commits later

[fer-marino]

Sure. That is usually my preference too

[aojea]

/gemini review

one last pass just to be sure, but this LGTM

[gemini-code-assist]

Code Review

This pull request introduces a configurable backend probe timeout (--backend-probe-timeout) to prevent command-spawned service backends with slow cold-start times from failing registration and advertisement. It updates the service registry to accept this timeout, falling back to a default of 2 seconds if not specified. Feedback on the changes suggests refactoring the newly added unit tests to avoid real-time delays (time.NewTimer), which can introduce flakiness in CI environments, in favor of a deterministic mock that inspects the context deadline.

[gemini-code-assist]

Using real-time delays (time.NewTimer with 60 * time.Millisecond) in unit tests can introduce flakiness, especially in resource-constrained or virtualized CI environments (like GitHub Actions) where CPU throttling can cause unexpected scheduling delays. Additionally, real-time sleeps slow down the test suite execution.

We can make this test 100% deterministic, faster, and free of real-time dependencies by using a deadline-inspecting and context-controlled fake service instead of a timer. For the timeout case, we can let Probe block on ctx.Done(). For the success case, we can inspect the deadline of the context passed to Probe to verify that the configured timeout is correctly applied.

type slowProbingService struct {
	*fakeService
	shouldTimeout bool
	lastDeadline  time.Time
	hasDeadline   bool
}

func newSlowProbingSvc(name string) *slowProbingService {
	return &slowProbingService{
		fakeService: newFakeSvc(name, api.ServiceType_SERVICE_TYPE_MCP),
	}
}

func (p *slowProbingService) Probe(ctx context.Context) error {
	p.lastDeadline, p.hasDeadline = ctx.Deadline()
	if p.shouldTimeout {
		<-ctx.Done()
		return ctx.Err()
	}
	return nil
}

// The bug behind #376: defaultDHTProbeTimeout was a hard-coded 2s with no way to
// raise it, so a backend whose own cold-start cost alone exceeds that -
// measured in practice for moderately-featured MCP server stacks - could
// never be advertised on its first registration. NewServiceRegistry's
// backendProbeTimeout parameter (wired from --backend-probe-timeout) is the
// fix: the same slow backend must fail to advertise under the default and
// succeed once constructed with more time.
func TestServiceRegistry_BackendProbeTimeoutIsConfigurable(t *testing.T) {
	t.Run("default timeout is too short for a slow backend", func(t *testing.T) {
		dht := &fakeDHT{}
		r := NewServiceRegistry(dht, 10*time.Millisecond)

		svc := newSlowProbingSvc("slow")
		svc.shouldTimeout = true
		if err := r.Register(context.Background(), svc); err != nil {
			t.Fatalf("Register: %v", err)
		}
		if len(dht.calls) != 0 {
			t.Errorf("Provide called %d times for a backend slower than the probe timeout, want 0", len(dht.calls))
		}
	})

	t.Run("raising the timeout applies the configured duration to the probe context", func(t *testing.T) {
		dht := &fakeDHT{}
		timeout := 500 * time.Millisecond
		r := NewServiceRegistry(dht, timeout)

		svc := newSlowProbingSvc("slow")
		if err := r.Register(context.Background(), svc); err != nil {
			t.Fatalf("Register: %v", err)
		}
		if len(dht.calls) != 2 {
			t.Errorf("Provide called %d times once given enough time to probe, want 2 (name + type CID)", len(dht.calls))
		}
		if !svc.hasDeadline {
			t.Fatal("expected probe context to have a deadline")
		}
		remaining := time.Until(svc.lastDeadline)
		if remaining > timeout || remaining < timeout-100*time.Millisecond {
			t.Errorf("expected probe deadline to be close to %v, got remaining %v", timeout, remaining)
		}
	})

	t.Run("zero or negative backendProbeTimeout falls back to defaultDHTProbeTimeout", func(t *testing.T) {
		for _, d := range []time.Duration{0, -1 * time.Second} {
			r := NewServiceRegistry(&fakeDHT{}, d)
			if got := r.backendProbeTimeout; got != defaultDHTProbeTimeout {
				t.Errorf("NewServiceRegistry(dht, %v).backendProbeTimeout = %v, want %v", d, got, defaultDHTProbeTimeout)
			}
		}
	})
}

[aojea]

@fer-marino since you have to rebase, what about considering the suggestion in #377 (comment) to avoid flakes? I think is reasonable

[fer-marino]

Both done, pushed as 1d047d1:

• Rebased onto main (had a couple of real conflicts against feat(node): declare labels in the node config file instead of --labels #382's labels-config move, resolved in the two "fix rebase conflict"/follow-up commits).
• Applied your suggested deterministic rewrite of the probe-timeout test verbatim - slowProbingService is now deadline-inspecting/context-controlled instead of a real timer, no more real-time sleeps.

[aojea]

Thanks