Skip to content

fix(datalayer): sync cross-replica state from one goroutine, not one per endpoint - #8

Open
Lucas-Fernandes-Martins wants to merge 1 commit into
lfm/combined-fixesfrom
lfm/cross-replica-single-goroutine
Open

fix(datalayer): sync cross-replica state from one goroutine, not one per endpoint#8
Lucas-Fernandes-Martins wants to merge 1 commit into
lfm/combined-fixesfrom
lfm/cross-replica-single-goroutine

Conversation

@Lucas-Fernandes-Martins

@Lucas-Fernandes-Martins Lucas-Fernandes-Martins commented Aug 10, 2026

Copy link
Copy Markdown

Why

Review feedback on the upstream PR llm-d/llm-d-router#2310, which introduced the independent cross-replica sync ticker already on lfm/combined-fixes.

ahg-g asked why the sync runs a goroutine per endpoint:

Note that the polling dispatcher was designed to be per endpoint because it needed to establish a connection and poll information per endpoint. This doesn't seem to be the case for cross replica syncing, a single go routine iterating over the endpoints list and calling sync on each should be sufficient.

He's right, and the per-endpoint design was also leaking. The goroutine only returns on ctx.Done(), but NewEndpoint is called with the process-lifetime parentCtx (datastore.go:438), so ReleaseEndpoint stopped the collector while the sync ticker kept publishing state for an endpoint that no longer exists — a goroutine leak plus stale cross-replica writes. Copilot flagged the same thing independently.

A single loop over the live endpoints fixes both at once: no per-endpoint goroutine to leak, and a released endpoint simply stops being visited.

Summary

  • One runCrossReplicaSync(ctx) goroutine, started from the existing Runtime.Start(ctx, mgr), which already receives a context and runs immediately after Configure (where crossReplicaPub is built). Each tick it iterates the live endpoints and dispatches.
  • Collector keeps the endpoint it was started with. It already documents itself as running "data collection for a single endpoint" and Start is handed that endpoint, so this makes the existing registry enumerable via collectorManager.RangeEndpoints — no wrapper type, and no second registry that could drift out of step with the collectors.

No change to what is published or how often; only how many goroutines do it, and that removed endpoints stop.

Reviewer guide

File What to look at
pkg/epp/datalayer/runtime.go Real logic. The goroutine moves from NewEndpoint to Start, and runCrossReplicaSync drops its ep parameter in favour of RangeEndpoints.
pkg/epp/datalayer/collector.go Small addition. One ep field set by Start, plus an Endpoint() accessor under the existing mutex.
pkg/epp/datalayer/manager.go Purely additive. RangeEndpoints only; Register, Remove and StopAll are untouched.
pkg/epp/datalayer/cross_replica_publisher_test.go New tests only, appended at the end. Existing tests untouched.

Worth a second opinion on placing the launch in Start: it is the only Runtime method taking a lifetime context, and it runs after Configure, so crossReplicaPub is always set by then — but the ordering is load-bearing and the nil check is what keeps it safe if that ever changes.

Note that RangeEndpoints only sees collectors that have been started. A collector is registered a few lines before Start is called, so a brand-new endpoint can miss at most one tick — before which it has nothing worth publishing anyway.

Test plan

  • go build ./..., gofmt -l, and go vet ./pkg/epp/datalayer/... all clean.
  • go test -race ./pkg/epp/datalayer/... green, including the pre-existing cross-replica publisher and collector tests.
  • Two new tests:
    • TestRunCrossReplicaSync_PublishesAllEndpoints — one loop publishes every registered endpoint.
    • TestRunCrossReplicaSync_StopsAfterRelease — regression guard for the leak: publishing stops once the endpoint is removed from the registry.

Note on the third review comment

Copilot also flagged crossReplicaPublisherType as an unused const that would fail to compile. That is stale — the symbol no longer exists anywhere in the tree (grep finds no references) and the build is clean. No change needed.

…per endpoint

Addresses review feedback on llm-d#2310.

The PollingDispatcher is per endpoint because it establishes and polls a
connection per endpoint. Cross-replica syncing does neither: it reads
local state and hands it to the syncer, so one goroutine iterating the
endpoints is sufficient.

Doing it per endpoint was also leaking. The goroutine only returned on
ctx.Done(), and NewEndpoint receives the process-lifetime parentCtx, so
ReleaseEndpoint stopped the collector while the ticker kept publishing
state for an endpoint that no longer existed. Iterating the live set
each tick removes both the leak and the stale writes, with no teardown
to get wrong.

Collector already documents itself as running "data collection for a
single endpoint" and is handed that endpoint by Start, so it now keeps
it. That makes the existing collector registry enumerable without a
wrapper type or a second registry to keep in step.
@Lucas-Fernandes-Martins
Lucas-Fernandes-Martins force-pushed the lfm/cross-replica-single-goroutine branch from 48ab8aa to cc3bd7c Compare August 10, 2026 16:38
@Lucas-Fernandes-Martins
Lucas-Fernandes-Martins marked this pull request as ready for review August 10, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant