The closure note in cluster/failure.go quoted a Gomega error that Gomega
does not emit. Describe the argument-count failure and the
Eventually().WithArguments() hint instead, so nobody greps for a string
that never appears.
The advisory-lock note in cluster_failover_test.go called the wedge
window unbounded. A SIGKILLed local child closes its socket at once, the
Postgres backend reads EOF and is reaped in milliseconds, so the
mechanism bounds the window tightly. Say bounded, and keep the low
probability but real framing, which was right.
The workflow comment attributed HealthCheckInterval to
core/services/nodes/health.go. It is declared in
core/config/distributed_config.go:64; health.go only carries the ticker
on the unexported checkInterval. Point a debugger at the right file.
Comments only, no behaviour change.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
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>
The two specs that assert a healthy worker stays healthy were pure negatives:
they say nothing happened. A cluster whose health checking had wedged, by
leaking the advisory lock the monitor takes at health.go:110, would freeze the
roster and satisfy both while observing a corpse. Kill the worker once the
window closes and require the roster to settle it to offline, so the preceding
Consistently is a statement about behaviour rather than about a stopped clock.
Applied to the cold-restart spec as well as the peer-death one: a restart is
exactly the event that could leave a replacement unable to check anything.
Document the hazard that can make an offline assertion hang. The staleness
branch skips a node already marked unhealthy (health.go:153-155), a skip meant
for nodes an operator took down, which also swallows the flap: an unhealthy mark
landing after the heartbeat goes stale means MarkOffline is never called and the
node stays unhealthy forever. Name the file and line at the assertion, and have
the failure message say so when the roster shows a node stuck there, so a
timeout sends the reader to LocalAI rather than to the harness.
Stop calling the two-replica registration spec a race. Start spawns workers
sequentially and the registrations land about a second apart; it is a
shared-roster identity test, and saying otherwise invites someone to trust it
for something it does not check.
WorkerRegistrar now bound-checks its index like every other index-taking method
here. It answered 0 for an out-of-range worker, and 0 is a real frontend index,
so the failure mode was a spec killing the wrong replica.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Four scenarios with no prior equivalent: killing a replica must not disturb a
worker that never depended on it, a cold-restarted replica must rehydrate the
roster from shared state and keep accepting the worker's heartbeats, a dead
worker must settle to offline on every replica, and two replicas registering a
worker each must converge on one roster.
The timings are measured, not assumed. Node liveness is heartbeat freshness, so
the only eviction path is StaleNodeThreshold (60s) plus one HealthCheckInterval
tick (15s), and neither is reachable from the CLI. A worker whose registrar was
killed was observed going offline at 74.2s. Every window here is sized to
outlast that, because an assertion that expires before the system could have
reacted proves nothing.
Two assertions are deliberately unlike the obvious form. Statuses are compared
for equality against a probe that returns a sentinel on error, rather than
asserting a name is absent from the healthy list: the list probe returns nil on
any error, and "does not contain" is satisfied by nil, so a 401 at the second
replica would have passed while observing nothing. And a killed worker is
required to settle to exactly offline, because it first flaps to unhealthy at
~8s and back to healthy at ~14s, which any not-healthy matcher would accept.
SpreadWorkerRegistrations is new, off by default, and exists so the racing
spec is a race: the harness otherwise points every worker at frontend 0, which
would have left that scenario asserting on two sequential writes through one
process. The default is unchanged because the baseline specs depend on it.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Ettore Di Giacinto