mirror of
https://github.com/mudler/LocalAI.git
synced 2026-10-01 18:44:35 -04:00
Review round 1 on the end-to-end proof. Zero blocking items, eleven non-blocking, and three of them turned out to be production defects rather than notes on the report. The one that matters is a misclassification the phase is built to prevent. A dial carries the caller's deadline down to the socket, so when the budget runs out the socket's timer fires and the error travels back up through the WebSocket handshake and the multiplexer. The context's cancellation is a separate timer whose func the scheduler has to run before ctx.Err() stops returning nil, and nothing orders the two. Under contention the socket's error is back in PeerPool.Open first, ctx.Err() reads nil, and a peer that is listening and healthy is reported as ErrPeerUnreachable to a caller that simply ran out of time. An unreachable peer is a fact a caller may act on and an expired deadline is not, and core/services/nodes routes around a replica it is told is unreachable. callerRanOut answers that question in one place: ctx.Err() when it is set, and otherwise the wall clock against the caller's own deadline. That is sound because it is the same instant the socket compared itself against, so if the socket's timer fired this comparison is past it too. The ambiguous instant resolves towards the caller, which is the direction that never blames a peer. The spec that caught it, peerlink_test.go's "blames the caller's deadline", was red in three of seven -race runs and had been since Task 5, which is often enough to read as noise and is why single-run verification never saw it. Rather than leave the proof to a coin flip, a second spec makes the window deterministic: Open is handed a context whose deadline has passed and whose cancellation has not been delivered, against an address nothing is listening on, so the dial fails for real. It reddens without the fix. The peer link's yamux windows were applied to one end only. A receive window is advertised by the side that RECEIVES, so configuring the dialler alone tunes exactly one direction, and the direction left on the 256 KiB default is the one that carries a relayed model artifact INTO the replica that owns the worker's tunnel. That is the largest thing the link ever moves and it is the direction the load measurement exercises: the review read it as flowing toward the dialler and it does not. PeerLinkConfig is now exported and used on both ends. Measured, same box, 128 MiB staged through the relay against the same transfer without one: the relayed path cost 1.6x to 2.0x the direct path's transfer window before, and 1.06x to 1.25x after. The SSRF reachability spec could be fooled into reporting an SSRF that did not happen. It bound the victim on 127.0.0.2 at an ephemeral port and required 127.0.0.1 at the same port to refuse, so any other spec in the run holding that number made the dial succeed; red one run in seven, green five of five in isolation. It now picks from below the kernel's ephemeral range, the same fix the harness got for the adjacent-port collision. The rest are the specs and the report saying what they mean. Scenario 1's advertisement assertion could not tell "the worker advertises nothing" from "the JSON key moved", which matters because removing the advertisement is the change it covers. It was green against a renamed key. The roster now keeps the raw key set beside the decoded fields and the spec requires both keys present before reading them as empty. Scenario 4's refusal-body check was a four-way disjunction admitting bare "tunnel", "not connected" and "unroutable". Those alternatives were inert and each would be satisfied by refusals that say nothing about routing, in the one assertion the whole negative control rests on. It is "no route" alone. The head-of-line gate bounded the worst probe by the whole transfer window, which admits about eightfold degradation and loosens as the box slows. It is now half the window, plus a scale-free ratio against the worst probe under the SAME cold load with nothing to transfer, which is the control that isolates the transfer from the load. Not tighter than that, and the reason is measured rather than cautious: under a concurrent -race suite the worst relayed probe reached a fifth of its window, so a quarter-window gate would have had 1.2x of margin, and a spec that fails one run in three is worse than no spec. The report entry printed p90 and p99 off samples of twenty, where both land on the same element and p99 often lands on the max, so one number appeared three times under three names. A quantile is now printed only when the sample can separate it. Two claims in the report were wrong and are withdrawn rather than softened. Scenario 2's race is closed by the trailing re-read of the owner, not by the pre-assertion the report credited: a move to the non-owner mid-request would serve directly and still return 200, and only the trailing read reddens on it. And "the median request is unchanged" holds on this box and not on the reviewer's, where the relayed median rises up to 82% and p99 up to 3.5x. What survives on both is structural: the worst probe is a small fraction of the window in which bytes are moving, so the session interleaves rather than serialising. Sharing a session with a bulk transfer costs latency; it does not cost service. The disk footprint note undercounted, and the reviewer lost a run to a full disk on this box, so it is worth having right: two bulk models seeded into two frontends and staged to the worker is about 768 MiB, not 512 MiB. Left alone deliberately: the worker's backend port allocator still hands out ports without checking they are free, and its default range still overlaps the kernel's ephemeral range. It is confirmed, it is out of scope here, and it is being tracked as a named follow-up rather than fixed under an e2e task. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
347 lines
12 KiB
Go
347 lines
12 KiB
Go
package cluster_test
|
|
|
|
import (
|
|
"context"
|
|
"io"
|
|
"net"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"sync"
|
|
"time"
|
|
|
|
clusterep "github.com/mudler/LocalAI/core/http/endpoints/cluster"
|
|
"github.com/mudler/LocalAI/core/services/cluster"
|
|
"github.com/mudler/LocalAI/core/services/testutil"
|
|
|
|
"github.com/labstack/echo/v4"
|
|
"github.com/libp2p/go-yamux/v5"
|
|
. "github.com/onsi/ginkgo/v2"
|
|
. "github.com/onsi/gomega"
|
|
"gorm.io/gorm"
|
|
)
|
|
|
|
// servePeerRoute mounts the peer handler on the route both sides agree on.
|
|
//
|
|
// It deliberately does not call routes.RegisterClusterRoutes: that registrar
|
|
// lives in core/http/routes, which imports half the server, and these specs are
|
|
// about the handler and the dialler rather than about the route table. The path
|
|
// comes from the same constant the registrar uses, so the two cannot drift.
|
|
func servePeerRoute(e *echo.Echo, token string, onPeer func(string, *yamux.Session)) {
|
|
e.GET(cluster.PeerPath, clusterep.PeerHandler(token, onPeer))
|
|
}
|
|
|
|
// deadlinePassed is a context whose deadline has elapsed and whose
|
|
// cancellation has not been delivered, which is the state a caller is in for
|
|
// the moment between the two timers that fire at its deadline. Only Deadline is
|
|
// overridden: the embedded context supplies a nil Done and a nil Err, which is
|
|
// what a context in that window reports.
|
|
type deadlinePassed struct{ context.Context }
|
|
|
|
func (deadlinePassed) Deadline() (time.Time, bool) {
|
|
return time.Now().Add(-time.Millisecond), true
|
|
}
|
|
|
|
var _ = Describe("Peer pool", func() {
|
|
var (
|
|
db *gorm.DB
|
|
reg *cluster.Registry
|
|
pool *cluster.PeerPool
|
|
srv *httptest.Server
|
|
accepted chan *yamux.Session
|
|
ctx context.Context
|
|
)
|
|
|
|
// startPeer stands up a real peer server and registers it under peerID.
|
|
startPeer := func(peerID string) *httptest.Server {
|
|
e := echo.New()
|
|
servePeerRoute(e, "peer-token", func(_ string, s *yamux.Session) {
|
|
accepted <- s
|
|
})
|
|
ts := httptest.NewServer(e)
|
|
addr := strings.TrimPrefix(ts.URL, "http://")
|
|
Expect(reg.Register(ctx, peerID, addr, "test")).To(Succeed())
|
|
return ts
|
|
}
|
|
|
|
BeforeEach(func() {
|
|
ctx = context.Background()
|
|
db = testutil.SetupTestDB()
|
|
Expect(cluster.Migrate(ctx, db)).To(Succeed())
|
|
reg = cluster.NewRegistry(db)
|
|
accepted = make(chan *yamux.Session, 4)
|
|
pool = cluster.NewPeerPool("self", "peer-token", reg)
|
|
DeferCleanup(pool.Close)
|
|
srv = startPeer("peer-1")
|
|
DeferCleanup(srv.Close)
|
|
})
|
|
|
|
It("opens a working stream to a live peer", func() {
|
|
st, err := pool.Open(ctx, "peer-1")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
DeferCleanup(func() { _ = st.Close() })
|
|
|
|
var serverSess *yamux.Session
|
|
Eventually(accepted, "10s").Should(Receive(&serverSess))
|
|
|
|
go func() {
|
|
defer GinkgoRecover()
|
|
_, _ = st.Write([]byte("ping"))
|
|
}()
|
|
|
|
got := make(chan []byte, 1)
|
|
go func() {
|
|
defer GinkgoRecover()
|
|
in, e := serverSess.AcceptStream()
|
|
if e != nil {
|
|
return
|
|
}
|
|
buf := make([]byte, 4)
|
|
if _, e := io.ReadFull(in, buf); e == nil {
|
|
got <- buf
|
|
}
|
|
}()
|
|
Eventually(got, "10s").Should(Receive(Equal([]byte("ping"))))
|
|
})
|
|
|
|
It("identifies itself to the peer by its own instance id", func() {
|
|
// The peer records which replica is on the far end of the link, so a
|
|
// pool that sent the peer's id (or nothing) would leave every inbound
|
|
// link anonymous and indistinguishable from every other.
|
|
ids := make(chan string, 1)
|
|
e := echo.New()
|
|
servePeerRoute(e, "peer-token", func(id string, _ *yamux.Session) { ids <- id })
|
|
ts := httptest.NewServer(e)
|
|
DeferCleanup(ts.Close)
|
|
Expect(reg.Register(ctx, "peer-named", strings.TrimPrefix(ts.URL, "http://"), "test")).To(Succeed())
|
|
|
|
st, err := pool.Open(ctx, "peer-named")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
DeferCleanup(func() { _ = st.Close() })
|
|
Eventually(ids, "10s").Should(Receive(Equal("self")))
|
|
})
|
|
|
|
It("reuses one session across opens rather than dialling per stream", func() {
|
|
a, err := pool.Open(ctx, "peer-1")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
DeferCleanup(func() { _ = a.Close() })
|
|
Eventually(accepted, "10s").Should(Receive())
|
|
|
|
b, err := pool.Open(ctx, "peer-1")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
DeferCleanup(func() { _ = b.Close() })
|
|
|
|
// A second dial would deliver a second server session. One session
|
|
// serving both streams is the property under test: peer links are
|
|
// pooled, not per-stream.
|
|
Consistently(accepted, "2s", "200ms").ShouldNot(Receive())
|
|
})
|
|
|
|
It("returns ErrPeerUnreachable when the peer is registered but not listening", func() {
|
|
dead := startPeer("peer-dead")
|
|
dead.Close()
|
|
|
|
_, err := pool.Open(ctx, "peer-dead")
|
|
Expect(err).To(MatchError(cluster.ErrPeerUnreachable),
|
|
"a peer that will not answer must be a transport error, never node absence")
|
|
Expect(err).ToNot(MatchError(cluster.ErrInstanceNotFound),
|
|
"an unreachable peer must never be readable as an absent node; a replica acting on absence evicts healthy workers")
|
|
})
|
|
|
|
It("returns ErrPeerUnreachable when the peer answers but rejects the credentials", func() {
|
|
// A token mismatch is a live peer refusing the link, not a missing
|
|
// row. Reporting absence here would evict every worker behind a peer
|
|
// that was merely rolled out with a stale secret.
|
|
e := echo.New()
|
|
servePeerRoute(e, "a-different-token", func(_ string, s *yamux.Session) { accepted <- s })
|
|
ts := httptest.NewServer(e)
|
|
DeferCleanup(ts.Close)
|
|
Expect(reg.Register(ctx, "peer-strict", strings.TrimPrefix(ts.URL, "http://"), "test")).To(Succeed())
|
|
|
|
_, err := pool.Open(ctx, "peer-strict")
|
|
Expect(err).To(MatchError(cluster.ErrPeerUnreachable))
|
|
Expect(err).ToNot(MatchError(cluster.ErrInstanceNotFound))
|
|
})
|
|
|
|
It("returns ErrInstanceNotFound when the peer is not in the registry", func() {
|
|
_, err := pool.Open(ctx, "never-registered")
|
|
Expect(err).To(MatchError(cluster.ErrInstanceNotFound))
|
|
Expect(err).ToNot(MatchError(cluster.ErrPeerUnreachable),
|
|
"a node that was never registered is absent, not merely unreachable")
|
|
})
|
|
|
|
It("re-dials after the cached session dies", func() {
|
|
first, err := pool.Open(ctx, "peer-1")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(first.Close()).To(Succeed())
|
|
|
|
var serverSess *yamux.Session
|
|
Eventually(accepted, "10s").Should(Receive(&serverSess))
|
|
Expect(serverSess.Close()).To(Succeed())
|
|
srv.Close()
|
|
|
|
// A replacement peer comes back on a new address under the same id,
|
|
// which is what a restarted replica looks like.
|
|
replacement := startPeer("peer-1")
|
|
DeferCleanup(replacement.Close)
|
|
|
|
Eventually(func() error {
|
|
st, e := pool.Open(ctx, "peer-1")
|
|
if e == nil {
|
|
_ = st.Close()
|
|
}
|
|
return e
|
|
}, "15s", "500ms").Should(Succeed())
|
|
|
|
// The replacement's own session proves the pool re-dialled the address
|
|
// it re-read from the registry rather than resurrecting the dead one.
|
|
Eventually(accepted, "10s").Should(Receive())
|
|
})
|
|
|
|
It("does not drop the pooled session when a single stream is reset by the peer", func() {
|
|
// A peer-initiated stream reset is scoped to one request. Dropping the
|
|
// session on it would tear down every other worker's traffic on the
|
|
// same link, so the pool must keep the session and hand out a fresh
|
|
// stream on it.
|
|
st, err := pool.Open(ctx, "peer-1")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
var serverSess *yamux.Session
|
|
Eventually(accepted, "10s").Should(Receive(&serverSess))
|
|
|
|
go func() {
|
|
defer GinkgoRecover()
|
|
_, _ = st.Write([]byte("x"))
|
|
}()
|
|
var inbound *yamux.Stream
|
|
Eventually(func() error {
|
|
s, e := serverSess.AcceptStream()
|
|
inbound = s
|
|
return e
|
|
}, "10s").Should(Succeed())
|
|
// Reset, not a graceful close: this is the *StreamError{Remote:true}
|
|
// the far end sends when it abandons a request.
|
|
Expect(inbound.Reset()).To(Succeed())
|
|
Eventually(func() error {
|
|
_, e := st.Write([]byte("y"))
|
|
return e
|
|
}, "10s", "100ms").Should(HaveOccurred())
|
|
|
|
next, err := pool.Open(ctx, "peer-1")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
DeferCleanup(func() { _ = next.Close() })
|
|
Consistently(accepted, "2s", "200ms").ShouldNot(Receive(),
|
|
"a reset stream must not cost the whole peer link")
|
|
})
|
|
|
|
It("blames the caller's deadline, not the peer, when a dial runs out of time", func() {
|
|
// A listener that completes the TCP connection and then says nothing,
|
|
// which is what a peer under load or behind a wedged proxy looks like.
|
|
// The peer is not unreachable; the caller is impatient. Reporting
|
|
// ErrPeerUnreachable here would make an impatient client enough to get
|
|
// a healthy replica routed around.
|
|
ln, err := net.Listen("tcp", "127.0.0.1:0")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
DeferCleanup(func() { _ = ln.Close() })
|
|
// The accept loop owns the connections it holds and closes them when
|
|
// the listener goes away, so nothing is shared with the spec goroutine.
|
|
go func() {
|
|
defer GinkgoRecover()
|
|
var held []net.Conn
|
|
defer func() {
|
|
for _, c := range held {
|
|
_ = c.Close()
|
|
}
|
|
}()
|
|
for {
|
|
c, e := ln.Accept()
|
|
if e != nil {
|
|
return
|
|
}
|
|
// Hold the connection open without ever answering the upgrade.
|
|
held = append(held, c)
|
|
}
|
|
}()
|
|
Expect(reg.Register(ctx, "peer-silent", ln.Addr().String(), "test")).To(Succeed())
|
|
|
|
deadlined, cancel := context.WithTimeout(ctx, 300*time.Millisecond)
|
|
DeferCleanup(cancel)
|
|
_, err = pool.Open(deadlined, "peer-silent")
|
|
Expect(err).To(MatchError(context.DeadlineExceeded))
|
|
Expect(err).ToNot(MatchError(cluster.ErrPeerUnreachable),
|
|
"the caller ran out of time; the peer never got a verdict")
|
|
Expect(err).ToNot(MatchError(cluster.ErrInstanceNotFound))
|
|
})
|
|
|
|
It("blames the caller's deadline even when its cancellation has not landed yet", func() {
|
|
// The same rule as the spec above, at the instant that makes it hard.
|
|
//
|
|
// A dial carries the caller's deadline down to the socket, so the
|
|
// socket's timer and the context's cancellation timer fire at the same
|
|
// moment and nothing orders them. The socket's error can be back in
|
|
// Open before the scheduler has run the context's cancel func, and in
|
|
// that window ctx.Err() is nil while the caller's budget is
|
|
// unambiguously spent. Reading only ctx.Err() there reports a peer that
|
|
// is listening and healthy as unreachable.
|
|
//
|
|
// That window is real: this spec's sibling above reproduces it under
|
|
// `-race` about three runs in seven, which is exactly often enough to
|
|
// be dismissed as noise. Here it is made deterministic instead, by
|
|
// handing Open a context in precisely that state: deadline passed,
|
|
// cancellation not delivered. Nothing is faked about the dial, which
|
|
// runs for real against an address nothing is listening on.
|
|
refused, err := net.Listen("tcp", "127.0.0.1:0")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
addr := refused.Addr().String()
|
|
Expect(refused.Close()).To(Succeed())
|
|
Expect(reg.Register(ctx, "peer-refusing", addr, "test")).To(Succeed())
|
|
|
|
_, err = pool.Open(deadlinePassed{ctx}, "peer-refusing")
|
|
Expect(err).To(MatchError(context.DeadlineExceeded))
|
|
Expect(err).ToNot(MatchError(cluster.ErrPeerUnreachable),
|
|
"the caller's budget was spent before the dial was made; blaming the peer for it is how an impatient client gets a healthy replica routed around")
|
|
Expect(err).ToNot(MatchError(cluster.ErrInstanceNotFound))
|
|
})
|
|
|
|
It("dials once when many callers open the same peer at the same time", func() {
|
|
// Without a per-peer lock held across the dial, every concurrent
|
|
// caller races to dial and all but one of the resulting sessions is
|
|
// dropped on the floor still holding a live WebSocket.
|
|
const callers = 16
|
|
streams := make(chan net.Conn, callers)
|
|
var wg sync.WaitGroup
|
|
for range callers {
|
|
wg.Add(1)
|
|
go func() {
|
|
defer wg.Done()
|
|
defer GinkgoRecover()
|
|
st, err := pool.Open(context.Background(), "peer-1")
|
|
Expect(err).ToNot(HaveOccurred())
|
|
streams <- st
|
|
}()
|
|
}
|
|
wg.Wait()
|
|
close(streams)
|
|
|
|
count := 0
|
|
for st := range streams {
|
|
count++
|
|
DeferCleanup(func(c net.Conn) { _ = c.Close() }, st)
|
|
}
|
|
Expect(count).To(Equal(callers))
|
|
|
|
Eventually(accepted, "10s").Should(Receive())
|
|
Consistently(accepted, "2s", "200ms").ShouldNot(Receive(),
|
|
"concurrent opens must share one dial, not race to dial per caller")
|
|
})
|
|
|
|
It("refuses to open after Close and is safe to close twice", func() {
|
|
pool.Close()
|
|
pool.Close()
|
|
|
|
_, err := pool.Open(ctx, "peer-1")
|
|
Expect(err).To(HaveOccurred())
|
|
Expect(err).ToNot(MatchError(cluster.ErrInstanceNotFound),
|
|
"a locally closed pool says nothing about whether the node exists")
|
|
})
|
|
})
|