test(distributed): correct the claims the e2e comments make

Review of the whole branch found five comments that would send a reader to
the wrong place, plus three smaller inaccuracies. Nothing here changes
behaviour.

The KNOWN RACE note on both backend-log WebSocket handlers said the fix
needs an atomic snapshot-plus-subscribe "under the store lock". It does
not: BackendLogStore.mu guards only the buffers map, and AppendLine
enqueues and fans out under the per-buffer buf.mu. Whoever took the store
lock would ship and the race would survive, so both notes now name buf.mu
and say what s.mu does and does not exclude.

Two comments in the cluster harness quoted Eventually(c.FrontendAlive)
.Should(BeFalse()). FrontendAlive takes an index, so Gomega rejects that
with "requested 1 arguments but received 0". Both now quote the closure
form the specs actually use, and say why the closure is needed.

proveHealthCheckingIsAlive claimed to prove the health monitor ran for the
whole preceding window. It proves the monitor was alive at the end of it,
and inferring backwards needs any wedge to be sticky. In the
peer-replica-death spec that inverts: health checks are single-flighted by
a session-scoped pg_try_advisory_lock, the spec SIGKILLs the replica that
may hold it, and until Postgres reaps the session the survivor acquires
nothing and checks nothing silently. Consistently(healthy) can then pass
because nothing was checking, with the positive control still succeeding
once the lock frees. The doc now states what is proven, names that gap,
and says the assertion is a floor rather than a proof.

The Makefile still called DISTRIBUTED_TEST_FLAKES a retry count, which is
what seeded that error into the two docs just corrected against it, and
the workflow called the 15s window a reconcile tick when the mechanism is
HealthCheckInterval in the node health monitor.

Also: the cluster suite measured 509.1s / 509.8s / 512.3s, so about
8m30s and not the 8m39s/8m40s three files claimed; the dead-worker spec
title implied two independent detectors when both probes read one
advisory-lock-serialised verdict out of the same row; and the
sanitizeDBName length assertion used <= 50, which an empty string also
satisfies, where the invariant for an over-long input is exactly 50.

Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>

.agents/building-and-testing.md

@@ -51,7 +51,7 @@ Rules (both gates):
 Two suites cover distributed mode (frontend replicas, worker nodes, PostgreSQL, NATS), split by a Ginkgo label:
 
 - `make test-e2e-distributed` runs `Distributed && !VLLMMultinode && !Cluster` over `./tests/e2e/distributed` recursively. Services are wired directly into the test binary. ~240 specs, ~75s.
-- `make test-e2e-cluster` runs `Cluster` and spawns real `local-ai` child processes through the `tests/e2e/distributed/cluster` helper package. 6 specs, 8m39s measured over three consecutive runs (509.1s / 509.8s / 512.3s).
+- `make test-e2e-cluster` runs `Cluster` and spawns real `local-ai` child processes through the `tests/e2e/distributed/cluster` helper package. 6 specs, about 8m30s measured over three consecutive runs (509.1s / 509.8s / 512.3s, so 8m29s to 8m32s).
 
 Both jobs live in `.github/workflows/tests-e2e-distributed.yml`, with `timeout-minutes: 45` each. They trigger on pull requests *and* on every push to `master`; the `paths-ignore` filter (see [.agents/ci-caching.md](ci-caching.md)) sits on the pull-request trigger only, so a master push always runs both. They are advisory only because `master` carries no branch protection, which is a repository setting and not a YAML key: `continue-on-error: true` would flip the run's *conclusion* to success and hide the failure, so it is not used.
 

.github/workflows/tests-e2e-distributed.yml

@@ -108,10 +108,11 @@ jobs:
     # which spec hung, to the runner, which prints nothing: a red job with no
     # evidence, which is how a suite gets disabled rather than fixed.
     #
-    # The suite itself is 8m39s over three consecutive runs (509.1s / 509.8s /
-    # 512.3s) on a developer box, and will be slower here. Three specs sit at
-    # ~167s each because they wait out a 60s staleness threshold plus a 15s
-    # reconcile tick. Do not shorten those windows to make this job faster: the
+    # The suite itself is about 8m30s over three consecutive runs (509.1s /
+    # 509.8s / 512.3s, so 8m29s to 8m32s) on a developer box, and will be slower
+    # here. Three specs sit at ~167s each because they wait out a 60s staleness
+    # threshold plus a 15s health-check tick (HealthCheckInterval, in
+    # core/services/nodes/health.go, not one of the reconcilers). Do not shorten those windows to make this job faster: the
     # wait is what stops the assertions from passing before the system could
     # have reacted, which was a real false green earlier on.
     timeout-minutes: 45

CONTRIBUTING.md

@@ -276,7 +276,7 @@ make test-e2e-cluster       # process-level: real local-ai child processes
 
 `make test-e2e-distributed` is the fast one (around 240 specs in roughly 75 seconds). It starts one PostgreSQL and one NATS for the whole run and gives each spec its own database. It runs each spec exactly once, with no retry: `DISTRIBUTED_TEST_FLAKES` defaults to 1 and feeds ginkgo's `--flake-attempts`, which counts *total attempts*, not retries. That is deliberately below the repo-wide `TEST_FLAKES=5`, because this suite exists to catch nondeterministic cluster behaviour and a retry hides exactly the failure it is meant to catch. Raise it locally when bisecting something unrelated.
 
-`make test-e2e-cluster` runs `local-ai` as real child processes, one per frontend replica and one per worker, so a spec can kill a replica and assert what the survivors do. Budget about 8m40s: three of its six specs wait out real staleness and health-check windows. It needs a built binary and the mock backend:
+`make test-e2e-cluster` runs `local-ai` as real child processes, one per frontend replica and one per worker, so a spec can kill a replica and assert what the survivors do. Budget about 8m30s (measured 509.1s / 509.8s / 512.3s over three consecutive runs): three of its six specs wait out real staleness and health-check windows. It needs a built binary and the mock backend:
 
 ```bash
 make build build-mock-backend

Makefile

@@ -340,10 +340,11 @@ run-e2e-aio: protogen-go
 	@echo 'Running e2e AIO tests'
 	$(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --flake-attempts $(TEST_FLAKES) -v -r ./tests/e2e-aio
 
-# Flake retries for the distributed suite. Defaults to 1, unlike TEST_FLAKES:
-# this suite exists to catch nondeterministic cluster behaviour, and retrying
-# hides exactly the failures it is meant to surface. Raise it locally if you are
-# bisecting something unrelated.
+# Total ginkgo attempts per spec for the distributed suite: --flake-attempts counts
+# attempts, not retries. Defaults to 1, so each spec runs once and is never retried,
+# unlike TEST_FLAKES=5. This suite exists to catch nondeterministic cluster behaviour,
+# and a retry hides exactly the failures it is meant to surface. Raise it locally if
+# you are bisecting something unrelated.
 DISTRIBUTED_TEST_FLAKES?=1
 
 # Distributed architecture e2e (PostgreSQL + NATS via testcontainers).

core/http/endpoints/localai/backend_logs.go

@@ -125,8 +125,12 @@ func BackendLogsWebSocketEndpoint(ml *model.ModelLoader) echo.HandlerFunc {
 		// a line appended in that window is never streamed. A viewer attaching while
 		// a model loads (when a backend is at its noisiest) can silently miss lines;
 		// they stay in the buffer, so a reload shows them. Fixing it needs an atomic
-		// snapshot-plus-subscribe under the store lock, not a reorder of these two
-		// calls, which would duplicate instead of drop.
+		// snapshot-plus-subscribe held under the buffer's own lock (buf.mu in
+		// pkg/model/backend_log_store.go), because that is the lock AppendLine takes
+		// while it enqueues and fans out to subscribers. The store-level s.mu guards
+		// only the buffers map and excludes nothing an appender does, so taking it
+		// leaves this race exactly where it is. Reordering these two calls is not a
+		// fix either: it would duplicate instead of drop.
 
 		// Send existing lines as initial batch
 		existingLines := ml.BackendLogs().GetLines(modelID)

core/services/nodes/file_transfer_server.go

@@ -843,8 +843,12 @@ func handleBackendLogsWS(w http.ResponseWriter, r *http.Request, logStore *model
 	// a line appended in that window is never streamed. A viewer attaching while
 	// a model loads (when a backend is at its noisiest) can silently miss lines;
 	// they stay in the buffer, so a reload shows them. Fixing it needs an atomic
-	// snapshot-plus-subscribe under the store lock, not a reorder of these two
-	// calls, which would duplicate instead of drop.
+	// snapshot-plus-subscribe held under the buffer's own lock (buf.mu in
+	// pkg/model/backend_log_store.go), because that is the lock AppendLine takes
+	// while it enqueues and fans out to subscribers. The store-level s.mu guards
+	// only the buffers map and excludes nothing an appender does, so taking it
+	// leaves this race exactly where it is. Reordering these two calls is not a
+	// fix either: it would duplicate instead of drop.
 
 	// Send existing lines as initial batch
 	existingLines := logStore.GetLines(modelID)

tests/e2e/distributed/cluster/failure.go

@@ -71,8 +71,15 @@ func (c *Cluster) KillWorker(i int) error {
 // dressed up as a failover one. No spec does that today; this note is here so
 // the first one that tries does not spend a day on it.
 //
-// After StopFrontendGracefully, wait for the process to actually go
-// (Eventually(c.FrontendAlive).Should(BeFalse())) before restarting. Restart
+// After StopFrontendGracefully, wait for the process to actually go before
+// restarting:
+//
+//	Eventually(func() bool { return c.FrontendAlive(i) }, "20s", "500ms").
+//		Should(BeFalse())
+//
+// FrontendAlive takes an index, so it has to be wrapped in a closure; handing
+// Gomega the method value directly fails with "requested 1 arguments but
+// received 0". Restart
 // terminates whatever is still running with SIGKILL, so restarting straight
 // after a SIGTERM cuts the drain short and quietly turns the rolling-update
 // case into the crash case, which is the opposite of what pairing those two
@@ -126,8 +133,11 @@ func (c *Cluster) FrontendAlive(i int) bool {
 // than the one it precedes. The window that stays open is the other one,
 // between the child exiting and waitid collecting it: there the child is a
 // zombie, signal 0 to a zombie succeeds, and alive reports true for a process
-// that is already dead. There is no local fix; the caller's is to poll,
-// Eventually(c.FrontendAlive).Should(BeFalse()), rather than assert once.
+// that is already dead. There is no local fix; the caller's is to poll rather
+// than assert once, wrapping the index-taking FrontendAlive in a closure:
+//
+//	Eventually(func() bool { return c.FrontendAlive(i) }, "20s", "500ms").
+//		Should(BeFalse())
 func (p *Process) alive() bool {
 	if p == nil || p.Cmd == nil || p.Cmd.Process == nil {
 		return false

tests/e2e/distributed/cluster_failover_test.go

@@ -134,11 +134,36 @@ func (p *rosterProbe) explainStuckOffline(worker, format string, args ...any) fu
 // It is the terminating positive control for the two specs that assert a
 // healthy worker STAYS healthy. On their own those are pure negative
 // assertions: a cluster whose health checking had wedged entirely, say by
-// leaking the Postgres advisory lock the monitor takes (health.go:110), would
-// freeze the roster and satisfy them while observing a corpse. Killing a worker
-// afterwards and requiring the roster to react proves the monitor was running
-// for the whole window, which is the only thing that makes the preceding
-// Consistently a statement about behaviour rather than about a stopped clock.
+// leaking the Postgres advisory lock the monitor takes
+// (core/services/nodes/health.go:112), would freeze the roster and satisfy them
+// while observing a corpse.
+//
+// WHAT IT ACTUALLY PROVES, which is less than it looks like. Killing a worker
+// afterwards and requiring the roster to react proves the monitor was alive at
+// the END of the preceding window. It does not observe the window itself. The
+// inference back across it holds only if a wedge would have been sticky, i.e.
+// still present when this helper ran.
+//
+// THE RESIDUAL GAP, and it is not hypothetical in the peer-replica-death spec.
+// Health checks are single-flighted across replicas by a session-scoped
+// pg_try_advisory_lock (advisorylock.TryWithLockCtx, non-blocking: a replica
+// that does not get the lock returns immediately and checks nothing, silently,
+// because checkAll discards the acquired flag). That spec SIGKILLs frontend 1,
+// which may have been holding the lock at the moment it died. Postgres releases
+// a session-level advisory lock only when it reaps the dead backend, so until
+// then frontend 0's ticks acquire nothing and no check runs. The roster freezes,
+// Consistently(healthy) passes BECAUSE NOTHING WAS CHECKING, and this helper
+// still succeeds afterwards once the session is reaped and the lock comes free.
+// That wedge is transient rather than permanent, which is exactly the shape the
+// backwards inference cannot see. Low probability, real, and unbounded only by
+// how fast Postgres notices a dead connection.
+//
+// So treat this as a floor and not a proof: it rules out a health monitor that
+// is permanently dead, which is the failure that would otherwise make the
+// preceding Consistently a statement about a stopped clock, and it does not rule
+// out a monitor that was idle for part of the window. Closing the gap needs a
+// positive observation from inside the window (a log or metric assertion that a
+// check ran), not a stronger assertion here.
 //
 // It costs a full detection cycle, which is why it is a shared helper: the
 // wall-clock price should be paid once per spec and explained once.
@@ -258,7 +283,7 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun
 		proveHealthCheckingIsAlive(c, restarted, 0)
 	})
 
-	It("settles a dead worker to offline on every replica", func() {
+	It("settles a dead worker to offline and both replicas report it offline", func() {
 		c := startCluster(2, 1)
 		worker := c.WorkerName(0)
 
@@ -297,6 +322,15 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun
 		// prevents it, so explainStuckOffline says so in the failure message
 		// when it sees a node stuck at unhealthy. Fixing it is LocalAI work,
 		// not test work.
+		// Reading the same verdict at both replicas proves shared-verdict
+		// propagation, NOT two independent detectors. Health checks are
+		// single-flighted by the advisory lock (see proveHealthCheckingIsAlive),
+		// so exactly one replica ran the check that wrote the offline status, and
+		// both probes then read that one Postgres row back. What this rules out
+		// is a replica that keeps a private roster, or one that reads the shared
+		// row and reports something else. A spec claiming both replicas can
+		// detect death on their own would have to isolate them from each other,
+		// which the shared database makes impossible by design.
 		for _, probe := range []*rosterProbe{at0, at1} {
 			Eventually(probe.statusOf, workerDeathTimeout, rosterPollInterval).
 				WithArguments(worker).

tests/e2e/distributed/dbname_test.go

@@ -16,7 +16,8 @@ var _ = Describe("Test database naming", Label("Distributed"), func() {
 			for i := 0; i < 100; i++ {
 				long += "a"
 			}
-			Expect(len(sanitizeDBName(long))).To(BeNumerically("<=", 50))
+			Expect(len(sanitizeDBName(long))).To(Equal(50),
+				"an over-long name must be truncated to exactly the 50-byte budget; <= 50 would also accept an empty name")
 		})
 
 		It("never produces an empty name", func() {