Review found the recurring class: assertions that a wrong implementation
also satisfies.
The "refuse promptly, never park the peer" guarantee was stated in three
places and tested in none. Removing the Close from the no-relay branch left
the whole cluster suite green, because the specs asserted only that some
error arrived and yamux reports a read deadline as ErrTimeout: a parked
stream satisfied that as well as a refused one. Both specs now require an
ENDING, EOF or a reset, inside a deadline short enough that parking is
unmistakable, and both go red when the Close is removed.
Deregistration existed only in a comment. Membership.Stop ended the loop and
left the row behind, so every clean rolling restart had peers dialling a
corpse for the full liveness window; the shutdown comment described the
opposite. Registry.Deregister deletes the row and the connections that
replica owned, in one transaction, for the reason the sweeper does both, and
an e2e spec pins departure inside a budget shorter than the liveness window
so it cannot pass on the sweeper doing the work. Before: the spec times out
with both replicas still live. After: 3.6s.
The configured advertised address bypassed every check discovery makes, so
the one value most likely to be copied between hosts, 127.0.0.1, was taken
verbatim and would make every peer dial itself. Both paths now share one
rejection rule: unparseable is refused, "this host" is warned about once and
honoured, because a single-host deployment uses it correctly.
Two comments claimed more than the code does. The sweeper said a stalled
replica recovers via re-register; only its instance row does, while the
connections another replica reaped stay gone and the sockets stay held here
- phase 2 must re-claim, on re-register, every connection a replica still
holds locally. And Owner became OwnerRow, documenting that the owner it
names may be dead for up to InstanceLiveness plus a heartbeat and that any
caller acting on it must join instances itself, so the deferred constraint
lives at the call site rather than in a report; the plain name is left free
for the joining version.
Minors: warn once when the peer link mounts with no registration token, so
an operator sees the cause rather than 401s; Stop no longer blocks forever
when Start was never called; corrected the NewRegistry migration doc and an
e2e comment that described a 6s window as "throughout".
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Tasks 1 to 5 built an instances table, a splice, both halves of a peer link
and an epoch fence, and nothing in the tree called any of it: no replica
registered, no route was mounted, no sweeper ran. Proving phase 1 end to
end therefore had to start by wiring it.
A frontend in distributed mode now publishes the address its peers dial,
heartbeats it, and sweeps replicas that stopped answering along with the
connection rows they owned, in one pass so the two can never disagree about
who is alive. It serves the peer link and owns the sessions peers dial in,
refusing streams on them until phase 2 installs a relay: a session nobody
accepts on does not fail a peer's Open, it hangs it.
The address is the one peers use, not the one the process binds, and it is
derived from the route to PostgreSQL. That derivation only holds while the
database is remote, so LOCALAI_DISTRIBUTED_ADVERTISE_ADDR sets it
explicitly and a replica that can determine neither warns and keeps
serving rather than failing to start.
Three e2e scenarios run against real local-ai processes, real PostgreSQL
and real dials: replicas publish addresses that can actually be connected
to; a sibling opens a stream over the peer link and is refused without the
cluster token; and a killed replica is reported unreachable, never absent,
loses the claim it held, and takes no worker with it. Each was verified by
mutation: eight injected defects, each failing the scenario that claims to
catch it.
Also moves RegisterClusterRoutes to core/http/routes beside every other
registrar, folds AutoMigrate and the epoch sequence into one
cluster.Migrate, and turns the peer route's auth-coverage spec into a real
assertion: it drives the request through the actual auth middleware
instead of comparing two string constants, which the old spec would have
passed even with the exemption deleted.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
The NodeConnection model carried `default:now()`, which is PostgreSQL syntax
reaching the DDL, so AutoMigrate failed on the single-binary SQLite path and
took every SQLite caller of nodes.NewNodeRegistry down with it. Stamp the
database clock as an expression inside Claim instead, the way Register
already does, and leave the column plain.
CREATE SEQUENCE is Postgres-only for the same reason, so it is skipped on
another dialect, and Claim refuses that dialect outright: a fence that cannot
draw a token must say so rather than fail later as a missing function.
Also correct a claim the previous commit made in both the doc comment and its
message. An epoch is unique and never reissued, but it is not ordered: the
insert path draws its sequence value before taking the row lock, so a claim
that inserts after a release can be handed a lower number than one already
issued. Uniqueness is what Release needs, since it matches by equality;
callers must never compare epochs for order.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Release deletes the row, so a per-row `epoch + 1` restarted the numbering at 1
for the next claim. A replica could then be handed an epoch it already held:
claim w1 at epoch 1, lose the link silently, watch another replica claim and
release, reclaim and be handed 1 again, and its delayed cleanup for the first
dead link would match the live claim and delete it. The fence has to be
unique per node over time, not per row lifetime.
Every claim now draws nextval from a dedicated sequence on both the insert and
the conflict paths, so an epoch is never issued twice. The draw still happens
after the row lock on the conflict path, so the winning claim still holds the
highest epoch handed out.
Also drop last_seen. Nothing maintained it and it was always equal to
connected_at, but an indexed column named that way invites a second liveness
clock; whether the owner is alive is Instance.LastSeen, and whether a claim is
current is the epoch.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
A worker whose link is silently broken reconnects to another replica while
the old owner's socket has not yet noticed. Without a fence both believe
they own it. Claim is a single atomic upsert returning the new epoch, and a
release must match both owner and epoch so a stale owner cannot delete a
live claim.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
The peer link's WebSocket adapter and route constant lived in
core/http/endpoints/cluster, so the dialler in core/services/cluster had to
import an HTTP endpoints package to reach them. That pulled echo, core/http/auth
and core/config into a package whose doc says it is deliberately free of such
dependencies, and it made core/services/nodes reach an endpoints package
transitively. It also has no way forward: the worker-connect handler needs the
tunnel registry and the node token store, both of which are cycles from there.
Move WebsocketConn and PeerPath into core/services/cluster and let the endpoints
package import it, which is the direction the rest of core/http flows. The route
and the auth exemption still cannot drift apart, now asserted where both are
visible rather than by a const reference across the boundary, and the assertion
is stronger than the one it replaces: it pins the route under the prefix instead
of pinning the prefix's spelling.
Also guard the fresh-dial path with ctx.Err(), mirroring the cached path. A
caller with a 300ms deadline dialling a live, listening peer was told the peer
was unreachable, which would be enough for one impatient client to get a healthy
replica routed around once the relay consults these errors.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Distinguishes a peer missing from the registry from a peer that will not
answer: the second must never be readable as node absence, or a network
hiccup between replicas evicts healthy workers.
The distinction is a property of the error type rather than of the call
sites. The unreachable error formats its cause into its message and keeps
it out of its unwrap chain, so an ErrInstanceNotFound picked up on the
dial path cannot reach a caller's absence check.
One yamux session is cached per peer and re-dialled when OpenStream on it
fails, which is how both a dead transport and a graceful remote go-away
arrive. A reset of one stream never reaches the pool, so an abandoned
request cannot cost every other worker its link.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
A dead yamux session does not always arrive wrapped. Session.close publishes
shutdownErr and closes shutdownCh before it force-closes the streams, so a
Write or Close landing in that window gets the raw cause back instead
(session.go:507-510, 528-533), and for a peer that vanished the raw cause is a
bare io.EOF. The generic io.EOF clause then reported the dead session as a
clean completion.
Remove the clause. A clean read-side EOF never reached it anyway: io.Copy
consumes that and reports nil, and neither *yamux.Stream nor *net.TCPConn
takes a WriteTo/ReadFrom path that would hand one back. Every existing spec
still passes, the io.EOF entry in the normal-termination table included, which
is what showed the branch was dead for legitimate endings and live only for
the bug.
Add a spec driving a real yamux session end to end. Every mux shape until now
was a synthesized error, which is exactly why a race inside the real library
stayed invisible.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Matching yamux errors with errors.Is was too broad. Session.close hands every
live stream ErrStreamReset wrapped around whatever killed the connection, so a
keepalive timeout, a broken TCP connection or a peer that simply vanished all
matched, and a relayed request that died reported a clean ending. Nothing
upstream would have retried or logged it.
Match the plain sentinels by identity, since only identity separates a stream
that was reset from the wrapped form that means the session died. Treat a
StreamError as a per-stream reset, and a GoAwayError as normal only when it
carries the no-error code, read off ErrRemoteGoAway because the constant is
unexported. ErrSessionShutdown needs no entry of its own; it is a GoAwayError
with that code.
Order matters as much as the matching: session death wraps its cause, which is
routinely io.EOF or a closed socket, so the mux checks run before the generic
endings. Reversing them alone puts a vanished peer back to nil.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
go-yamux/v5 matches none of its errors against net.ErrClosed, so the
classifier reported an ordinary teardown as a failure: when the session has
gone away, the FIN that Splice's own Close writes returns ErrSessionShutdown,
and a stream torn down under a live copy surfaces as ErrStreamClosed or a
reset. Splice owns that Close, so it owns the errors it produces; the
sentinels are named here rather than injected by the caller, which would make
a forgotten classifier reintroduce the same bug silently.
Cover the error half of the contract, which no in-memory pipe could reach: a
scripted stream now feeds Splice a genuine transport failure and each
closed-stream ending in turn. Replacing the tail of Splice with "return nil"
passed every previous spec.
Also assert that Splice does not return until the second direction has
finished, rename a spec that promised a leak check it never made, and correct
two comments that claimed more than the code did.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Returns on the first direction to finish and closes both sides so the other
unblocks; a sequential copy deadlocks on any protocol where the far side
speaks first. EOF and use-of-closed are normal termination, not errors.
The fourth spec covers a peer that stops reading mid-body, the case where a
copy is parked in Write rather than in Read. The other three tear down an
idle splice and pass even against a Splice that closes only one side.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
DiscoverAdvertisedAddr promised to return an error rather than a fallback
no peer can dial, but only rejected an unspecified address. With PostgreSQL
on the same host or pod as a replica, which is compose, single-node and any
sidecar layout, the route to it is loopback, so every replica advertised
127.0.0.1 and a peer dialling that reached itself. Loopback, link-local and
zoned source addresses are now rejected with an error naming the remedy, and
a port outside 1-65535 is rejected before it becomes an undialable address.
Liveness was also measured on each replica's own clock: Register and
Heartbeat stamped last_seen from the Go process, and Live compared those
rows against the reading replica's time.Now(). Skew therefore shrank or
stretched the window by writerBehind+readerAhead, evicting healthy peers or
keeping dead ones. Both sides now use the database clock, which is the one
clock every replica demonstrably shares.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Replicas need to find each other to relay worker traffic, and nothing in
the tree recorded a replica's address. The advertised address is discovered
by opening a UDP socket toward PostgreSQL and reading back the local
address, which yields the interface every replica demonstrably shares
without asking an operator to configure one.
Assisted-by: Claude Opus 5 [claude-code]
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Ettore Di Giacinto