diff --git a/core/application/distributed.go b/core/application/distributed.go index 512b81780..fbe46a981 100644 --- a/core/application/distributed.go +++ b/core/application/distributed.go @@ -375,10 +375,12 @@ func initDistributed(cfg *config.ApplicationConfig, authDB *gorm.DB, configLoade if err != nil { return "", err } - if node.HTTPAddress == "" { - return "", fmt.Errorf("node %s has no HTTP address for file transfer", nodeID) - } - return node.HTTPAddress, nil + // An empty HTTPAddress is no longer a refusal. A tunnel-only worker + // reports none and does not need one: the http stream tag ignores + // the target and the worker routes to its own server. The host is + // only ever the URL's host component here, and WorkerHTTPHost + // supplies one that resolves nowhere so it cannot become a dial. + return nodes.WorkerHTTPHost(nodeID, node.HTTPAddress), nil }, cfg.Distributed.RegistrationToken, workerHTTPDialer) xlog.Info("File stager initialized (HTTP direct transfer)") } diff --git a/core/http/endpoints/localai/nodes.go b/core/http/endpoints/localai/nodes.go index f10cc9c83..f404555ac 100644 --- a/core/http/endpoints/localai/nodes.go +++ b/core/http/endpoints/localai/nodes.go @@ -801,11 +801,11 @@ func NodeBackendLogsListEndpoint(registry *nodes.NodeRegistry, registrationToken return c.JSON(http.StatusNotFound, nodeError(http.StatusNotFound, "node not found")) } - if node.HTTPAddress == "" { - return c.JSON(http.StatusBadGateway, nodeError(http.StatusBadGateway, "node has no HTTP address")) - } - - resp, err := proxyHTTPToWorker(ctx, dialFor, nodeID, node.HTTPAddress, "/v1/backend-logs", registrationToken) + // No HTTPAddress guard: a tunnel-only worker reports none, and the + // http stream tag ignores the target anyway. WorkerHTTPHost fills the + // URL's host with something that identifies the node and resolves + // nowhere; the tunnel decides where the bytes go. + resp, err := proxyHTTPToWorker(ctx, dialFor, nodeID, nodes.WorkerHTTPHost(nodeID, node.HTTPAddress), "/v1/backend-logs", registrationToken) if err != nil { return c.JSON(http.StatusBadGateway, nodeError(http.StatusBadGateway, fmt.Sprintf("failed to reach worker: %v", err))) } @@ -831,12 +831,8 @@ func NodeBackendLogsLinesEndpoint(registry *nodes.NodeRegistry, registrationToke return c.JSON(http.StatusNotFound, nodeError(http.StatusNotFound, "node not found")) } - if node.HTTPAddress == "" { - return c.JSON(http.StatusBadGateway, nodeError(http.StatusBadGateway, "node has no HTTP address")) - } - path := "/v1/backend-logs/" + url.PathEscape(modelID) - resp, err := proxyHTTPToWorker(ctx, dialFor, nodeID, node.HTTPAddress, path, registrationToken) + resp, err := proxyHTTPToWorker(ctx, dialFor, nodeID, nodes.WorkerHTTPHost(nodeID, node.HTTPAddress), path, registrationToken) if err != nil { return c.JSON(http.StatusBadGateway, nodeError(http.StatusBadGateway, fmt.Sprintf("failed to reach worker: %v", err))) } @@ -888,7 +884,7 @@ func NodeBackendLogsWSEndpoint(registry *nodes.NodeRegistry, registrationToken s // NetDialContext below is what decides where the connection goes. A // missing dialer is a failure, not a direct dial: see // nodes.ErrNoWorkerDialer. - workerURL := fmt.Sprintf("ws://%s/v1/backend-logs/%s/ws", node.HTTPAddress, url.PathEscape(modelID)) + workerURL := fmt.Sprintf("ws://%s/v1/backend-logs/%s/ws", nodes.WorkerHTTPHost(nodeID, node.HTTPAddress), url.PathEscape(modelID)) workerHeaders := http.Header{} if registrationToken != "" { workerHeaders.Set("Authorization", "Bearer "+registrationToken) @@ -899,8 +895,21 @@ func NodeBackendLogsWSEndpoint(registry *nodes.NodeRegistry, registrationToken s workerDial = dialFor(nodeID) } if workerDial == nil { - return c.JSON(http.StatusBadGateway, nodeError(http.StatusBadGateway, - fmt.Sprintf("cannot reach node %s: %v", nodeID, nodes.ErrNoWorkerDialer))) + // A JSON body cannot be written here: the response writer was + // hijacked by the upgrade above, so the status line is long gone + // and the write lands nowhere. The browser has to be told the same + // way every other failure past the upgrade tells it, with a close + // frame, and the socket has to be closed or it leaks for the life + // of the process. + // Best-effort: the browser may already have gone, and there is + // nothing left to report the failure to either way. The CLOSE is + // what matters and it is unconditional. + _ = browserWS.WriteMessage(websocket.CloseMessage, + websocket.FormatCloseMessage(websocket.CloseInternalServerErr, "no route to worker")) + _ = browserWS.Close() + xlog.Error("Cannot stream backend logs: no way to reach the worker", + "node", nodeID, "error", nodes.ErrNoWorkerDialer) + return nil } workerDialer := websocket.Dialer{HandshakeTimeout: 10 * time.Second, NetDialContext: workerDial} diff --git a/core/services/cluster/dialer.go b/core/services/cluster/dialer.go index b4a04551a..fbb95d583 100644 --- a/core/services/cluster/dialer.go +++ b/core/services/cluster/dialer.go @@ -34,6 +34,102 @@ type PeerOpener interface { // and core/services/nodes reclaims the models of a worker it believes absent. var ErrNoRelayPath = errors.New("cluster: this replica cannot relay to the owner of that worker") +// ErrNoRoute reports that this replica could not get a request to a worker's +// backend, and is the FIFTH condition this phase keeps apart. +// +// It says nothing about whether the worker exists or is running. A worker's +// PRESENCE is its heartbeat, which lives in core/services/nodes and which this +// package cannot see; what this package can see is whether a route exists right +// now, and those are different questions with different answers. A worker that +// is registered, heartbeating and serving models can be unroutable from here +// for a whole list of ordinary reasons: it has not dialled its tunnel yet after +// a frontend-first upgrade, the replica holding its tunnel is restarting, the +// ownership row is a moment stale, this replica has no peer mesh. +// +// Every failure to RESOLVE OR OPEN a route carries it, so a consumer that must +// not act on absence has exactly one check to make. The specific condition +// stays in the unwrap chain underneath for anyone that can act on it, with one +// deliberate exception: see routeFailure. +// +// It is not carried by a REFUSAL from the worker itself. A worker that answers +// is present by demonstration, and folding its answer into "no route" would +// throw away the one thing on this path that is real evidence. +var ErrNoRoute = errors.New("cluster: no route from this replica to that worker") + +// noRouteError reports a worker this replica cannot route to, keeping the cause +// in its message and OUT of its unwrap chain. +// +// Withholding the cause is the entire point, and it is the same guarantee +// unreachableError makes for peers. The causes this is built over are absence +// claims: ErrNoConnection ("no live replica holds this worker's tunnel") and +// ErrInstanceNotFound ("no such frontend replica"). Both are true statements +// about the CLUSTER and neither is a statement about the worker, but a consumer +// matching on them would read them as one, and the consequence is the +// catastrophe this phase is built around: a scheduler concludes a worker that +// is heartbeating and serving has gone away, and reclaims its models. +// +// The guarantee therefore belongs to the type. There is no path by which an +// absence sentinel gets out, so no call site can leak one. +type noRouteError struct { + nodeID string + cause error +} + +func (e *noRouteError) Error() string { + return fmt.Sprintf("cluster: no route from this replica to node %q: %v", e.nodeID, e.cause) +} + +// Unwrap reports only ErrNoRoute. The cause reaches a human through Error() and +// reaches no error-matching caller at all. +func (e *noRouteError) Unwrap() error { return ErrNoRoute } + +// routeFailure is the ONE place a Dial failure is turned into an error, and the +// one place the absence rule is expressed. +// +// The rule: an absence claim never reaches a caller, and everything else stays +// matchable. It is a single predicate in a single function on purpose. An +// earlier shape in this phase encoded one policy in two predicates, and +// reverting either left the suite green because the error reached the same +// answer down the other path; a rule whose correctness argument IS its mutation +// evidence cannot afford to be un-mutatable in pieces. Falsifying either half +// of isAbsenceClaim now reddens a named spec. +func routeFailure(nodeID string, cause error) error { + if isAbsenceClaim(cause) { + return &noRouteError{nodeID: nodeID, cause: cause} + } + return fmt.Errorf("reaching node %q: %w: %w", nodeID, ErrNoRoute, cause) +} + +// isAbsenceClaim reports whether an error asserts that something does not +// exist. Those are the errors routeFailure keeps out of the chain. +// +// Both are about the CLUSTER rather than about the worker. ErrNoConnection says +// no live replica holds the worker's tunnel; ErrInstanceNotFound says a peer +// replica is not in the deployment. Neither can be answered by this package +// with "and therefore the worker is gone", because this package does not know +// what a worker is beyond an id in a connection row. +func isAbsenceClaim(err error) bool { + return errors.Is(err, ErrNoConnection) || errors.Is(err, ErrInstanceNotFound) +} + +// isWorkerAnswer reports whether an error is the WORKER's own refusal, read off +// the reply it sent. +// +// Those three sentinels are the only ones ReadStreamReply produces from a frame +// the worker actually wrote. Everything else it returns is a failure to read +// one, which is the tunnel breaking rather than the worker speaking. +// +// A reply carrying a code this frontend does not recognise is deliberately NOT +// counted here, even though a worker did send it. Classifying it as an answer +// would let a newer worker's vocabulary be read by an older frontend as +// evidence about a backend, and the consequence of guessing wrong in that +// direction is a reaped replica; guessing wrong the other way costs a retry. +func isWorkerAnswer(err error) bool { + return errors.Is(err, ErrStreamTagUnknown) || + errors.Is(err, ErrStreamTargetUnavailable) || + errors.Is(err, ErrStreamRequestInvalid) +} + // dialHandshakeTimeout bounds the request/reply exchange that opens every // stream, when the caller stated no deadline of its own. // @@ -79,13 +175,18 @@ func NewWorkerDialer(tunnels *TunnelRegistry, peers PeerOpener) *WorkerDialer { // inference that is quiet for minutes, and a deadline left over from the // handshake would abort it. // -// The errors are kept apart on purpose and a caller may act on them -// differently. ErrNoConnection means no live replica holds this worker's -// tunnel, which is the one answer that means the worker is absent. ErrNotOwner -// means the routing was stale and re-resolving may find it. ErrPeerUnreachable -// means a replica would not answer, ErrNoRelayPath that none could be dialled, -// and the tunnelproto sentinels that the worker itself refused. Nothing here -// ever converts one of the others into absence. +// EVERY failure to resolve or open a route carries ErrNoRoute, and NO failure +// carries an absence sentinel. That pair is the contract, and it is what makes +// this safe to consume from a package that reclaims a worker's models when it +// decides the worker has gone: there is one check to make, and there is nothing +// to mistake for absence even if the caller makes none. +// +// Underneath the umbrella the conditions stay apart and a caller may act on +// them differently. ErrNotOwner means the routing was stale and re-resolving +// may find it; ErrPeerUnreachable means a replica would not answer; +// ErrNoRelayPath means none could be dialled. A refusal from the WORKER carries +// its own tunnelproto sentinel and no umbrella at all, because a worker that +// answers has demonstrated it is there. func (d *WorkerDialer) Dial(ctx context.Context, nodeID, tag, target string) (net.Conn, error) { stream, err := d.tunnels.Open(ctx, nodeID) if err == nil { @@ -93,10 +194,9 @@ func (d *WorkerDialer) Dial(ctx context.Context, nodeID, tag, target string) (ne } if !errors.Is(err, ErrNotOwner) { // The tunnel is held HERE and its session would not carry a stream. - // Reported as itself: answering ErrNotOwner would send the caller to - // resolve an owner that is this same replica, and answering absence - // would tell a scheduler to reclaim a worker that is attached. - return nil, err + // ErrNotOwner stays out of it: that answer would send the caller to + // resolve an owner which is this same replica. + return nil, routeFailure(nodeID, err) } return d.relay(ctx, nodeID, tag, target) } @@ -137,10 +237,12 @@ func (d *WorkerDialer) relay(ctx context.Context, nodeID, tag, target string) (n // come back as ErrNoConnection here. owner, _, err := d.tunnels.reg.Owner(ctx, nodeID) if err != nil { - // ErrNoConnection and database failures both pass through as - // themselves. This is the ONLY path by which this function can produce - // an absence error, and it produces it only when Owner did. - return nil, err + // ErrNoConnection is the ordinary answer here, and it is precisely the + // one that must not get out: it means no live replica holds this + // worker's tunnel, which a worker that has not dialled in yet produces + // on every single request while it sits there heartbeating and serving. + // routeFailure keeps it in the message and out of the chain. + return nil, routeFailure(nodeID, err) } if owner == d.tunnels.selfID { // The table names this replica and the registry above said the tunnel @@ -149,19 +251,20 @@ func (d *WorkerDialer) relay(ctx context.Context, nodeID, tag, target string) (n // would resolve the same owner and relay again. Reported as the routing // fact so the caller re-resolves, which terminates: the row is either // re-claimed by whoever holds the worker now, or swept. - return nil, fmt.Errorf("opening a stream to node %q: the connection row names this replica, which no longer holds the tunnel: %w", nodeID, ErrNotOwner) + return nil, routeFailure(nodeID, fmt.Errorf("the connection row names this replica, which no longer holds the tunnel: %w", ErrNotOwner)) } if d.peers == nil { - return nil, fmt.Errorf("opening a stream to node %q held by replica %q: %w", nodeID, owner, ErrNoRelayPath) + return nil, routeFailure(nodeID, fmt.Errorf("the tunnel is held by replica %q: %w", owner, ErrNoRelayPath)) } stream, err := d.peers.Open(ctx, owner) if err != nil { - // Whatever the pool said, unchanged in its unwrap chain: - // ErrPeerUnreachable, ErrInstanceNotFound for an owner swept since the - // lookup above, or ErrPoolClosed while this process shuts down. None of - // them is a statement about the WORKER, and none is converted into one. - return nil, fmt.Errorf("opening a stream to node %q through replica %q: %w", nodeID, owner, err) + // ErrPeerUnreachable and ErrPoolClosed keep their identity; the one + // case the pool can also produce, ErrInstanceNotFound for an owner + // swept between the lookup above and this dial, is an absence claim + // about the REPLICA and routeFailure withholds it. Either way nothing + // here is a statement about the worker. + return nil, routeFailure(nodeID, fmt.Errorf("through replica %q: %w", owner, err)) } // The caller's remaining time, stated so the owning replica can bound its @@ -169,14 +272,15 @@ func (d *WorkerDialer) relay(ctx context.Context, nodeID, tag, target string) (n // the owner falls back to without it. if err := WriteRelayRequest(stream, nodeID, remainingBudget(ctx)); err != nil { _ = stream.Close() - return nil, fmt.Errorf("naming node %q on a stream to replica %q: %w", nodeID, owner, err) + return nil, routeFailure(nodeID, fmt.Errorf("naming the node on a stream to replica %q: %w", owner, err)) } if err := ReadRelayReply(stream); err != nil { - // ReadRelayReply already separates a refusal (ErrNotOwner, - // ErrRelayUnavailable, ErrRelayRequestInvalid) from a failure to read - // one, and neither kind is ever an absence error. + // A refusal from the OWNING REPLICA, not from the worker. It says the + // owner would not relay, which is a route that does not exist, so the + // umbrella is right for all of them; ErrNotOwner, ErrRelayUnavailable + // and ErrRelayRequestInvalid stay in the chain underneath. _ = stream.Close() - return nil, fmt.Errorf("relaying to node %q through replica %q: %w", nodeID, owner, err) + return nil, routeFailure(nodeID, fmt.Errorf("through replica %q: %w", owner, err)) } return d.handshake(ctx, stream, nodeID, tag, target) } @@ -189,20 +293,27 @@ func (d *WorkerDialer) relay(ctx context.Context, nodeID, tag, target string) (n // session, and a frontend that retries would exhaust the worker's stream // budget rather than the worker's patience. func (d *WorkerDialer) handshake(ctx context.Context, stream net.Conn, nodeID, tag, target string) (net.Conn, error) { - if deadline, ok := handshakeDeadline(ctx); ok { - if err := stream.SetDeadline(deadline); err != nil { - _ = stream.Close() - return nil, fmt.Errorf("arming the handshake deadline for node %q: %w", nodeID, err) - } + if err := stream.SetDeadline(handshakeDeadline(ctx)); err != nil { + _ = stream.Close() + return nil, routeFailure(nodeID, fmt.Errorf("arming the handshake deadline: %w", err)) } if err := WriteStreamRequest(stream, tag, target); err != nil { + // The stream would not carry the request, so the tunnel broke under it. + // Nothing was asked of the worker and nothing was learned about it. _ = stream.Close() - return nil, fmt.Errorf("asking node %q for %q on %q: %w", nodeID, tag, target, err) + return nil, routeFailure(nodeID, fmt.Errorf("asking for %q on %q: %w", tag, target, err)) } if err := ReadStreamReply(stream); err != nil { _ = stream.Close() - return nil, fmt.Errorf("opening %q on node %q: %w", tag, nodeID, err) + if isWorkerAnswer(err) { + // The worker wrote a refusal, so it is connected and answering. + // This is the ONE failure on the whole path that is real evidence + // about the worker, and putting the umbrella on it would throw that + // away. + return nil, fmt.Errorf("opening %q on node %q: %w", tag, nodeID, err) + } + return nil, routeFailure(nodeID, fmt.Errorf("opening %q: %w", tag, err)) } // Cleared unconditionally rather than only when one was armed, so that this @@ -213,7 +324,7 @@ func (d *WorkerDialer) handshake(ctx context.Context, stream net.Conn, nodeID, t // bounds a peer that has stopped answering. if err := stream.SetDeadline(time.Time{}); err != nil { _ = stream.Close() - return nil, fmt.Errorf("clearing the handshake deadline for node %q: %w", nodeID, err) + return nil, routeFailure(nodeID, fmt.Errorf("clearing the handshake deadline: %w", err)) } xlog.Debug("opened a tunnelled stream to a worker", "node", nodeID, "tag", tag, "target", target) return stream, nil @@ -221,13 +332,18 @@ func (d *WorkerDialer) handshake(ctx context.Context, stream net.Conn, nodeID, t // handshakeDeadline is when the handshake must be done by: the caller's own // deadline when it has one and it is the sooner, and the backstop otherwise. -func handshakeDeadline(ctx context.Context) (time.Time, bool) { +// +// There is always one, which is why this returns no "was there one" flag: a +// context with no deadline still gets the backstop, so the caller has nothing +// to branch on. It used to return a bool that was unconditionally true, and the +// branch behind it could not be taken. +func handshakeDeadline(ctx context.Context) time.Time { backstop := time.Now().Add(dialHandshakeTimeout) deadline, ok := ctx.Deadline() if !ok || deadline.After(backstop) { - return backstop, true + return backstop } - return deadline, true + return deadline } // remainingBudget is how long the caller is still willing to wait, or zero when diff --git a/core/services/cluster/dialer_test.go b/core/services/cluster/dialer_test.go index ffcfe3775..2e0b945d3 100644 --- a/core/services/cluster/dialer_test.go +++ b/core/services/cluster/dialer_test.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "net" + "sync" "time" "github.com/mudler/LocalAI/core/services/cluster" @@ -26,15 +27,31 @@ import ( type stubPeers struct { sess *yamux.Session err error + + // opened records the peers this pool was asked for, so a spec can assert + // that a replica was NOT dialled. That is the only way to tell a dialer + // that resolved a live owner from one that resolved a dead row and then + // found out the hard way. + mu sync.Mutex + opened []string } -func (s *stubPeers) Open(ctx context.Context, _ string) (net.Conn, error) { +func (s *stubPeers) Open(ctx context.Context, peerID string) (net.Conn, error) { + s.mu.Lock() + s.opened = append(s.opened, peerID) + s.mu.Unlock() if s.err != nil { return nil, s.err } return s.sess.OpenStream(ctx) } +func (s *stubPeers) peersDialled() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.opened...) +} + // dialResult carries what a Dial produced, so a spec can wait on a channel // rather than on a clock. type dialResult struct { @@ -124,7 +141,8 @@ func refuseOneStream(worker *yamux.Session, reason error) { // // It is the assertion this whole phase turns on. core/services/nodes reclaims a // worker's models when it concludes the worker is absent, so an unreachable -// peer or a refusing worker arriving as absence would evict healthy work. +// peer, a stale ownership row or a worker that has not dialled its tunnel yet +// arriving as absence would evict healthy work. func expectNotAbsence(err error) { GinkgoHelper() Expect(err).To(HaveOccurred()) @@ -132,6 +150,18 @@ func expectNotAbsence(err error) { Expect(err).ToNot(MatchError(cluster.ErrInstanceNotFound)) } +// expectNoRoute asserts the umbrella is present as well as absence being gone. +// +// The umbrella is what crosses the package boundary. A consumer that reclaims +// models has one check to make, and it can only make it if EVERY failure to +// resolve or open a route carries it; a single path that forgets is a path +// where a live worker gets reaped. +func expectNoRoute(err error) { + GinkgoHelper() + expectNotAbsence(err) + Expect(err).To(MatchError(cluster.ErrNoRoute)) +} + var _ = Describe("The worker dialer", func() { var ( db *gorm.DB @@ -200,16 +230,25 @@ var _ = Describe("The worker dialer", func() { Expect(string(echoed)).To(Equal("ping")) }) - It("leaves no read deadline armed on the stream it hands back", func() { + It("leaves no deadline armed on the stream it hands back", func() { // The handshake is bounded; the request that follows it is the // caller's business and may be a generation that is quiet for - // minutes. A deadline left armed here would abort it. + // minutes. A deadline left armed here would abort it, and in + // production the dial context is the model-load or request budget, + // so the stream would die tens of seconds in. + // + // The first version of this spec did not assert that. It set a + // 300ms context and then wrote immediately, so the armed deadline + // had not expired and deleting the clear left it green: it detected + // only a deadline set in the PAST. What makes it bite is waiting for + // the dial context to actually expire FIRST, on its own Done channel + // rather than a sleep, and only then using the stream. frontend, worker := workerTunnel() _, err := mine.Attach(ctx, "w1", frontend) Expect(err).ToNot(HaveOccurred()) seen := serveOneStream(worker) - deadlined, cancel := context.WithTimeout(ctx, 300*time.Millisecond) + deadlined, cancel := context.WithTimeout(ctx, 200*time.Millisecond) defer cancel() d := cluster.NewWorkerDialer(mine, nil) result := dialAsync(d, deadlined, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000") @@ -220,12 +259,16 @@ var _ = Describe("The worker dialer", func() { Expect(out.err).ToNot(HaveOccurred()) DeferCleanup(func() { _ = out.conn.Close() }) - // The dial context's deadline has now passed. A stream still - // carrying it would fail this write and this read. - Eventually(func() error { - _, err := out.conn.Write([]byte("ping")) - return err - }, "10s").Should(Succeed()) + // The one wait this spec cannot replace with an event of its own: + // there is nothing to observe until the dial's deadline is behind + // us, and the deadline is the thing under test. + <-deadlined.Done() + Expect(deadlined.Err()).To(HaveOccurred()) + + // Both directions, because SetDeadline arms read and write and a + // clear that only covered one would still kill a live request. + _, err = out.conn.Write([]byte("ping")) + Expect(err).ToNot(HaveOccurred()) echoed := make([]byte, 4) Eventually(readInto(out.conn, echoed), "10s").Should(Receive(BeNil())) Expect(string(echoed)).To(Equal("ping")) @@ -241,8 +284,12 @@ var _ = Describe("The worker dialer", func() { var out dialResult Eventually(dialAsync(d, ctx, "w1", "nonsense", ""), "10s").Should(Receive(&out)) Expect(out.err).To(MatchError(cluster.ErrStreamTagUnknown)) - // A refusal is PROOF the worker is connected and answered. + // A refusal is PROOF the worker is connected and answered, so it is + // the ONE failure on this path that carries no umbrella: it is real + // evidence about the worker, and folding it into "no route" would + // throw that evidence away. expectNotAbsence(out.err) + Expect(out.err).ToNot(MatchError(cluster.ErrNoRoute)) }) It("reports a broken tunnel held here as itself, not as a routing fact", func() { @@ -257,9 +304,8 @@ var _ = Describe("The worker dialer", func() { d := cluster.NewWorkerDialer(mine, nil) var out dialResult Eventually(dialAsync(d, ctx, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000"), "10s").Should(Receive(&out)) - Expect(out.err).To(HaveOccurred()) Expect(out.err).ToNot(MatchError(cluster.ErrNotOwner)) - expectNotAbsence(out.err) + expectNoRoute(out.err) }) }) @@ -376,7 +422,7 @@ var _ = Describe("The worker dialer", func() { var out dialResult Eventually(dialAsync(d, ctx, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000"), "20s").Should(Receive(&out)) Expect(out.err).To(MatchError(cluster.ErrPeerUnreachable)) - expectNotAbsence(out.err) + expectNoRoute(out.err) }) It("passes a stale ownership refusal back as the routing fact", func() { @@ -396,7 +442,7 @@ var _ = Describe("The worker dialer", func() { var out dialResult Eventually(dialAsync(d, ctx, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000"), "10s").Should(Receive(&out)) Expect(out.err).To(MatchError(cluster.ErrNotOwner)) - expectNotAbsence(out.err) + expectNoRoute(out.err) }) It("refuses rather than relaying to itself when the table names this replica", func() { @@ -410,7 +456,7 @@ var _ = Describe("The worker dialer", func() { var out dialResult Eventually(dialAsync(d, ctx, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000"), "10s").Should(Receive(&out)) Expect(out.err).To(MatchError(cluster.ErrNotOwner)) - expectNotAbsence(out.err) + expectNoRoute(out.err) }) It("reports having no way to relay as its own condition", func() { @@ -424,22 +470,35 @@ var _ = Describe("The worker dialer", func() { Expect(out.err).To(MatchError(cluster.ErrNoRelayPath)) Expect(out.err).ToNot(MatchError(cluster.ErrNotOwner)) Expect(out.err).ToNot(MatchError(cluster.ErrPeerUnreachable)) - expectNotAbsence(out.err) + expectNoRoute(out.err) }) }) Describe("when no live replica holds the tunnel", func() { - It("reports absence when the worker has no connection row at all", func() { - d := cluster.NewWorkerDialer(mine, &stubPeers{err: errors.New("no peer should be dialled")}) + // The rolling-upgrade case, and the one this phase must not get wrong. + // + // A worker's PRESENCE is its heartbeat, which lives in + // core/services/nodes. "No live replica holds this worker's tunnel" is + // a fact about tunnels and says nothing about the worker: a worker that + // has not dialled in yet after a frontend-first upgrade produces it on + // every request while it sits there heartbeating and serving models. + // A consumer told that is absence reclaims every one of those models. + It("answers no-route, never absence, for a worker with no connection row", func() { + peers := &stubPeers{err: errors.New("no peer should be dialled")} + d := cluster.NewWorkerDialer(mine, peers) var out dialResult Eventually(dialAsync(d, ctx, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000"), "10s").Should(Receive(&out)) - Expect(out.err).To(MatchError(cluster.ErrNoConnection)) + expectNoRoute(out.err) + // The cause still reaches a human. + Expect(fmt.Sprint(out.err)).To(ContainSubstring("no connection recorded")) + Expect(peers.peersDialled()).To(BeEmpty()) }) - It("reports absence when the row's owner has stopped heartbeating", func() { + It("answers no-route, never absence, when the row's owner has stopped heartbeating", func() { // End to end over the join Owner does: the row is there, the owner - // is not. A dialer built on the unjoined read would dial a corpse - // and report the worker as unreachable rather than as absent. + // is not. The join is what stops this replica dialling a process + // that is gone, which is why the spec asserts no peer was dialled + // as well as what came back. Expect(reg.Register(ctx, "ghost", "10.0.0.9:8080", "v1")).To(Succeed()) _, err := reg.Claim(ctx, "w1", "ghost") Expect(err).ToNot(HaveOccurred()) @@ -451,10 +510,28 @@ var _ = Describe("The worker dialer", func() { Expect(err).ToNot(HaveOccurred()) Expect(owner).To(Equal("ghost")) - d := cluster.NewWorkerDialer(mine, &stubPeers{err: errors.New("no peer should be dialled")}) + peers := &stubPeers{err: errors.New("no peer should be dialled")} + d := cluster.NewWorkerDialer(mine, peers) var out dialResult Eventually(dialAsync(d, ctx, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000"), "10s").Should(Receive(&out)) - Expect(out.err).To(MatchError(cluster.ErrNoConnection)) + expectNoRoute(out.err) + Expect(peers.peersDialled()).To(BeEmpty()) + }) + + It("keeps an owner swept mid-dial out of the chain as well", func() { + // The other absence sentinel. PeerPool resolves the owner's address + // through the registry, so a replica reaped between Owner and the + // dial comes back as ErrInstanceNotFound. That is absence of a + // REPLICA, and a consumer matching absence would read it as absence + // of the WORKER. + Expect(reg.Register(ctx, "owner", "10.0.0.2:8080", "v1")).To(Succeed()) + _, err := reg.Claim(ctx, "w1", "owner") + Expect(err).ToNot(HaveOccurred()) + + d := cluster.NewWorkerDialer(mine, &stubPeers{err: cluster.ErrInstanceNotFound}) + var out dialResult + Eventually(dialAsync(d, ctx, "w1", cluster.StreamTagGRPC, "127.0.0.1:41000"), "10s").Should(Receive(&out)) + expectNoRoute(out.err) }) }) diff --git a/core/services/cluster/relay.go b/core/services/cluster/relay.go index 9392834f4..61594c7ab 100644 --- a/core/services/cluster/relay.go +++ b/core/services/cluster/relay.go @@ -125,6 +125,16 @@ func ReadRelayRequest(r io.Reader) (string, time.Duration, error) { if err != nil { return "", 0, fmt.Errorf("reading a relay request for node %q: budget %q is not a number of milliseconds: %w", nodeID, budgetText, err) } + if millis > maxRelayBudgetMillis { + // time.Duration is nanoseconds in an int64, so multiplying by + // time.Millisecond overflows past about 2.9e11 ms. Overflow here is + // bounded in the safe direction (it can only produce a negative or a + // small value, and both shorten the declaring peer's OWN open), but a + // bound that holds by arithmetic accident is not a bound. Anything past + // the ceiling is clamped to it, because a caller claiming to wait + // longer than the relay's own backstop gets the backstop either way. + millis = maxRelayBudgetMillis + } if millis <= 0 { // A caller with nothing left to spend. Reported as such rather than // folded into "not stated", so the relay refuses at once instead of @@ -209,6 +219,12 @@ func ReadRelayReply(r io.Reader) error { } } +// maxRelayBudgetMillis is the largest budget a peer may declare, and exists so +// the conversion below cannot overflow. A day is many orders of magnitude past +// relayOpenTimeout, which is the only thing a budget is ever compared against, +// so clamping to it changes no honest caller's behaviour. +const maxRelayBudgetMillis = int64(24 * 60 * 60 * 1000) + const ( // relayHeaderTimeout bounds how long a peer stream may go without naming // the worker it is for. Without it, a dialler killed between OpenStream and diff --git a/core/services/nodes/backend_client_factory_test.go b/core/services/nodes/backend_client_factory_test.go index be20f5e34..15adeb4f3 100644 --- a/core/services/nodes/backend_client_factory_test.go +++ b/core/services/nodes/backend_client_factory_test.go @@ -5,6 +5,7 @@ package nodes import ( "context" "net" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -99,3 +100,34 @@ var _ = Describe("The backend client factory", func() { }) }) }) + +var _ = Describe("the host used to address a worker's own HTTP server", func() { + It("uses the registered address when the worker reports one", func() { + Expect(WorkerHTTPHost("node-1", "10.0.0.5:8080")).To(Equal("10.0.0.5:8080")) + }) + + It("still produces a host for a tunnel-only worker that reports none", func() { + // Task 7 removes the worker's inbound listeners, at which point a + // worker has no address to report. Refusing here would refuse exactly + // the workers the tunnel exists for, and the guards that used to do + // that returned 502 "node has no HTTP address". + host := WorkerHTTPHost("node-1", "") + Expect(host).ToNot(BeEmpty()) + Expect(host).To(ContainSubstring("node-1")) + }) + + It("produces a host that cannot resolve, so it can never become a dial", func() { + // The value fills a URL's host component and nothing else. Making it + // unresolvable is what stops a later refactor connecting to it by + // accident: .invalid is reserved by RFC 2606 and resolves nowhere. + host := WorkerHTTPHost("node-1", "") + hostname, _, err := net.SplitHostPort(host) + Expect(err).ToNot(HaveOccurred()) + Expect(hostname).To(HaveSuffix(".invalid")) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _, err = net.DefaultResolver.LookupHost(ctx, hostname) + Expect(err).To(HaveOccurred()) + }) +}) diff --git a/core/services/nodes/file_stager_http.go b/core/services/nodes/file_stager_http.go index bd286e5d3..825560c82 100644 --- a/core/services/nodes/file_stager_http.go +++ b/core/services/nodes/file_stager_http.go @@ -41,6 +41,15 @@ type HTTPFileStager struct { // clients caches one *http.Client per node. Caching is what keeps the // connection pool: a client built per request would open a fresh tunnel // stream for every chunk of a multi-gigabyte upload. + // + // Entries are never pruned, and that is judged acceptable rather than + // overlooked. The map is bounded by the number of distinct workers this + // frontend has ever staged to, which is bounded by the fleet; each entry is + // a transport whose idle connections the 90s IdleConnTimeout above reclaims, + // so a departed worker's entry holds a map slot and nothing else. It is the + // same shape as PeerPool.links and would need the same thing to fix + // properly: a signal that a node has left, which the deregistration path + // does not publish today. clientsMu sync.Mutex clients map[string]*http.Client responseTimeout time.Duration // timeout waiting for server response after upload diff --git a/core/services/nodes/health.go b/core/services/nodes/health.go index 44d79e0d8..8a26a042f 100644 --- a/core/services/nodes/health.go +++ b/core/services/nodes/health.go @@ -203,9 +203,23 @@ func (hm *HealthMonitor) doCheckAll(ctx context.Context) { mCheckCtx, mCancel := context.WithTimeout(ctx, 5*time.Second) ok, _ := mClient.HealthCheck(mCheckCtx) mCancel() + // Asked BEFORE the client is closed, because closing is what + // would discard the transport's record of why it failed. + unreached := unroutable(mClient) if closer, ok := mClient.(io.Closer); ok { closer.Close() } + if unreached != nil { + // The probe never reached a backend, so it observed + // nothing. The miss streak is left exactly as it was: + // neither advanced, which after three passes would delete + // this row and every other row in the fleet the moment a + // peer link blipped, nor cleared, which would forgive a + // backend that really has died. + xlog.Warn("Could not probe a model backend: no route to the worker", + "node", node.ID, "model", m.ModelName, "replica", m.ReplicaIndex, "error", unreached) + continue + } key := modelKey{NodeID: node.ID, ModelName: m.ModelName, ReplicaIndex: m.ReplicaIndex} hm.missesMu.Lock() diff --git a/core/services/nodes/health_mock_test.go b/core/services/nodes/health_mock_test.go index 281b876a7..737be51b1 100644 --- a/core/services/nodes/health_mock_test.go +++ b/core/services/nodes/health_mock_test.go @@ -133,8 +133,17 @@ func (f *fakeNodeHealthStore) RemoveNodeModel(_ context.Context, nodeID, modelNa type fakeBackendClient struct { healthy bool err error + // dialErr makes this client report that its TRANSPORT failed, which is what + // a real client whose tunnel dial failed does. It is the half of + // unroutability that a refusing factory cannot stand in for, and the likely + // one in production: the factory only fails when the wiring is absent. + dialErr error } +// LastDialError satisfies grpc.DialErrorReporter so a spec can drive the +// "reached no backend" branch without a real tunnel. +func (c *fakeBackendClient) LastDialError() error { return c.dialErr } + func (c *fakeBackendClient) IsBusy() bool { return false } func (c *fakeBackendClient) HealthCheck(_ context.Context) (bool, error) { return c.healthy, c.err @@ -391,4 +400,5 @@ func freshTime() time.Time { // Compile-time interface checks var _ NodeHealthStore = (*fakeNodeHealthStore)(nil) var _ BackendClientFactory = (*fakeBackendClientFactory)(nil) +var _ grpc.DialErrorReporter = (*fakeBackendClient)(nil) var _ grpc.Backend = (*fakeBackendClient)(nil) diff --git a/core/services/nodes/health_test.go b/core/services/nodes/health_test.go index 6f624b0ca..a20384737 100644 --- a/core/services/nodes/health_test.go +++ b/core/services/nodes/health_test.go @@ -9,6 +9,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/mudler/LocalAI/core/services/cluster" + "github.com/mudler/LocalAI/core/services/testutil" "gorm.io/gorm" ) @@ -325,6 +327,53 @@ var _ = Describe("HealthMonitor (mock-based)", func() { Expect(store.getNode("node-cut").Status).To(Equal(StatusHealthy)) }) + It("leaves a model row alone when the probe never reached the worker", func() { + // The sibling of the factory case above, and the likelier one. The + // client is built fine and the tunnel DIAL fails, which gRPC + // reports with the same code as a dead backend. Counted as a miss + // it would delete every model row in the fleet after three passes + // of a peer link blip, while the models kept serving. + store := newFakeNodeHealthStore() + factory := newFakeBackendClientFactory() + hm := newTestHealthMonitor(store, factory, true, staleThreshold) + hm.perModelHealthCheck = true + + node := makeTestNode("node-blip", "blip-worker", "10.0.0.22:50051", StatusHealthy, freshTime()) + store.addNode(node) + store.addNodeModel("node-blip", NodeModel{NodeID: "node-blip", ModelName: "m", Address: "10.0.0.22:50053"}) + factory.setClient("10.0.0.22:50053", &fakeBackendClient{ + healthy: false, + err: fmt.Errorf("connection error"), + dialErr: fmt.Errorf("%w: %w", cluster.ErrNoRoute, cluster.ErrPeerUnreachable), + }) + + for i := 0; i < perModelMissThreshold+2; i++ { + hm.doCheckAll(context.Background()) + } + Expect(store.getCalls()).NotTo(ContainElement(ContainSubstring("RemoveNodeModel"))) + }) + + It("still reaps a backend that died on a worker it CAN reach", func() { + // The other direction, so the new check cannot pass by never + // reaping. A dial that succeeded and an RPC that failed is a dead + // process, and its row must still go. + store := newFakeNodeHealthStore() + factory := newFakeBackendClientFactory() + hm := newTestHealthMonitor(store, factory, true, staleThreshold) + hm.perModelHealthCheck = true + + node := makeTestNode("node-dead", "dead-worker", "10.0.0.23:50051", StatusHealthy, freshTime()) + store.addNode(node) + store.addNodeModel("node-dead", NodeModel{NodeID: "node-dead", ModelName: "m", Address: "10.0.0.23:50053"}) + // No dialErr: the transport was fine. + factory.setClient("10.0.0.23:50053", &fakeBackendClient{healthy: false, err: fmt.Errorf("connection refused")}) + + for i := 0; i < perModelMissThreshold; i++ { + hm.doCheckAll(context.Background()) + } + Expect(store.getCalls()).To(ContainElement("RemoveNodeModel:node-dead:m:0")) + }) + It("preserves model row when an intermittent failure is followed by a success", func() { store := newFakeNodeHealthStore() factory := newFakeBackendClientFactory() diff --git a/core/services/nodes/interfaces.go b/core/services/nodes/interfaces.go index 1231c811f..d10289838 100644 --- a/core/services/nodes/interfaces.go +++ b/core/services/nodes/interfaces.go @@ -143,10 +143,15 @@ type NodeManager interface { // WorkerDialerFor hands back the dial function for one worker's backend // processes: the shape grpc.WithContextDialer wants, bound to a node. // -// It is declared here rather than taken as a core/services/cluster type so that -// this package keeps no dependency on that one. cluster is a leaf and imports -// neither this package nor core/http; the wiring in core/application supplies -// (*cluster.WorkerDialer).GRPCDialerFor, which has exactly this shape. +// A function type rather than a *cluster.WorkerDialer, so nothing here is bound +// to that concrete type and a spec can supply a dial without building a tunnel +// registry, a peer pool and a database. It is NOT to avoid a dependency: this +// package already imports core/services/cluster (registry.go, for Migrate), and +// an earlier version of this comment claimed otherwise. The dependency that +// does matter runs the other way, and cluster is held to it by go list -deps. +// +// core/application supplies (*cluster.WorkerDialer).GRPCDialerFor, which has +// exactly this shape. type WorkerDialerFor func(nodeID string) func(ctx context.Context, addr string) (net.Conn, error) // WorkerNetDialerFor hands back the dial function for one worker's own HTTP @@ -155,6 +160,26 @@ type WorkerDialerFor func(nodeID string) func(ctx context.Context, addr string) // has this shape. type WorkerNetDialerFor func(nodeID string) func(ctx context.Context, network, addr string) (net.Conn, error) +// ErrWorkerUnroutable reports that this frontend could not get a request to a +// worker's backend, and says NOTHING about whether that worker or its backend +// is alive. +// +// It is the fifth condition, on this side of the package boundary. A worker's +// presence is its HEARTBEAT, and this package owns that; a route to it is a +// separate fact owned by core/services/cluster, and the two now differ. A +// worker can be registered, heartbeating and serving every request another +// replica sends it while being unroutable from here: it has not dialled its +// tunnel yet after a frontend-first upgrade, the replica holding its tunnel is +// restarting, the ownership row is a moment stale, this replica has no peer +// mesh. Every one of those used to be indistinguishable from "the backend +// process died", because gRPC reports both as codes.Unavailable. +// +// Everything in this package that DELETES a node_models row must consult it +// first. That is the phase's stated catastrophe in its concrete form: a row +// deleted here is a model reclaimed and reloaded elsewhere, so mistaking a peer +// link blip for a dead backend evicts healthy work across the fleet at once. +var ErrWorkerUnroutable = errors.New("nodes: this frontend has no route to that worker") + // ErrNoWorkerDialer reports that something tried to reach a worker without a // way to reach it through the worker's tunnel. // @@ -162,9 +187,41 @@ type WorkerNetDialerFor func(nodeID string) func(ctx context.Context, network, a // advertised address. A worker that holds a tunnel need not listen on anything // and may be behind NAT with no address to dial, so the fallback would work // only where the tunnel was not needed: on a single-host developer setup, and -// nowhere the feature exists for. It is also not an absence error: nothing -// about it says the worker is gone. -var ErrNoWorkerDialer = errors.New("nodes: no worker tunnel dialer is configured, so this worker cannot be reached") +// nowhere the feature exists for. +// +// It is a SPECIALISATION of ErrWorkerUnroutable rather than a sibling, so the +// single check every reaping path makes covers both. The difference between +// them is only when they happen: this one is a boot-time misconfiguration, and +// the general one is a running deployment losing a route for a moment. Neither +// is a statement about the worker. +var ErrNoWorkerDialer = fmt.Errorf("%w: no worker tunnel dialer is configured", ErrWorkerUnroutable) + +// unroutable reports why a call on client never reached the backend, or nil +// when it did reach one. +// +// This is where core/services/cluster's five conditions cross the package +// boundary. They cannot cross on the RPC error: gRPC turns any dialer failure +// into codes.Unavailable with the cause flattened into a message, and +// codes.Unavailable is ALSO what a backend process that has died produces. +// pkg/grpc records the dialer's error VALUE instead, so cluster.ErrNoRoute and +// whatever sits under it are still matchable here. +// +// A client that reports nothing (no custom dialer, or a test double) yields +// nil, which means "the call reached a backend" and preserves the behaviour +// every non-distributed caller has always had. +func unroutable(client grpc.Backend) error { + reporter, ok := client.(grpc.DialErrorReporter) + if !ok { + return nil + } + dialErr := reporter.LastDialError() + if dialErr == nil { + return nil + } + // Multi-%w: the umbrella this package acts on, and the cluster condition + // underneath it, both stay matchable. + return fmt.Errorf("%w: %w", ErrWorkerUnroutable, dialErr) +} // BackendClientFactory creates the gRPC clients this frontend uses to reach // model backends running on worker nodes. @@ -233,3 +290,32 @@ func (f *tunnelClientFactory) NewClientForNode(nodeID, address string, parallel } return grpc.NewClientWithDialer(address, parallel, nil, false, f.token, dial), nil } + +// unroutableHostSuffix is appended to a node id to build a Host for a worker +// that reports no HTTP address. +// +// .invalid is reserved by RFC 2606 and resolves nowhere, which is the point: +// the string exists ONLY to fill the host component of a URL, and a value that +// could resolve would be one a future refactor could accidentally connect to. +const unroutableHostSuffix = ".worker.invalid:80" + +// WorkerHTTPHost is the host to put in a URL addressed to a worker's own HTTP +// server. +// +// A tunnel-only worker has no inbound address to report, and after this phase +// it does not need one: the `http` stream tag ignores the target entirely and +// the worker routes the stream to its own server wherever that bound. But an +// http.Request still needs a host, so refusing an empty HTTPAddress would +// refuse exactly the workers the tunnel exists for. This returns a name that +// identifies the node for logs and for the Host header, and that nothing can +// connect to. +// +// It is NOT a dial target and never becomes one. Every caller pairs it with a +// transport whose DialContext is that node's tunnel, so the host is read and +// discarded; see cluster.WorkerDialer.DialerFor and the `http` tag. +func WorkerHTTPHost(nodeID, httpAddress string) string { + if httpAddress != "" { + return httpAddress + } + return nodeID + unroutableHostSuffix +} diff --git a/core/services/nodes/probe_cache.go b/core/services/nodes/probe_cache.go index 422e36ede..fee71383e 100644 --- a/core/services/nodes/probe_cache.go +++ b/core/services/nodes/probe_cache.go @@ -73,22 +73,41 @@ func (c *probeCache) Invalidate(key string) { // probes invalidate the cache, so a transient miss doesn't pin every // subsequent request to a re-probe. func (c *probeCache) DoOrCached(key string, probe func() bool) bool { + alive, _ := c.DoOrCachedResult(key, func() (bool, error) { return probe(), nil }) + return alive +} + +// DoOrCachedResult is DoOrCached with a second result: the reason the probe +// never reached the backend, or nil when it did. +// +// The second result travels through the SINGLEFLIGHT, which is the whole reason +// it is not simply a variable the caller closes over. A closed-over variable is +// only written by the goroutine that actually runs the probe; every other +// caller coalesced into that flight adopts the leader's boolean and sees its own +// unset variable, so the leader would correctly decline to reap while its +// joiners reaped on the very same observation. Carrying it in singleflight's +// error slot hands every joiner the leader's reason as well as its answer. +// +// A probe that could not reach the backend is NOT cached either way. Caching it +// as fresh would hide a genuinely dead backend behind a network blip, and +// caching it as a failure is what Invalidate already does. +func (c *probeCache) DoOrCachedResult(key string, probe func() (bool, error)) (bool, error) { if c.IsFresh(key) { - return true + return true, nil } - v, _, _ := c.flight.Do(key, func() (any, error) { + v, unreached, _ := c.flight.Do(key, func() (any, error) { // Double-check after potentially waiting: another caller in this // flight may have just populated the cache. if c.IsFresh(key) { return true, nil } - ok := probe() + ok, unreached := probe() if ok { c.markFresh(key) } else { c.Invalidate(key) } - return ok, nil + return ok, unreached }) - return v.(bool) + return v.(bool), unreached } diff --git a/core/services/nodes/reconciler.go b/core/services/nodes/reconciler.go index d50b55cef..923559932 100644 --- a/core/services/nodes/reconciler.go +++ b/core/services/nodes/reconciler.go @@ -91,17 +91,34 @@ func (g grpcModelProber) Probe(ctx context.Context, nodeID, address string) Prob probeCtx, cancel := context.WithTimeout(ctx, probeTimeout) defer cancel() ok, err := client.HealthCheck(probeCtx) + if unreached := unroutable(client); unreached != nil { + // The RPC never reached a backend. classifyProbeOutcome cannot tell: + // gRPC hands it codes.Unavailable for a worker this frontend has no + // route to and for a backend process that has died, and the reaper + // deletes rows on the second. + xlog.Warn("Could not probe a model: no route to the worker", + "node", nodeID, "address", address, "error", unreached) + return ProbeUnknown + } return classifyProbeOutcome(ok, err) } // classifyProbeOutcome maps a HealthCheck result onto a ProbeOutcome. // +// It is only ever reached for a probe that DID reach the worker. That is a +// precondition and not an observation it can make for itself: its caller asks +// the transport first and answers ProbeUnknown when the dial failed. Without +// that step the Unavailable case below is wrong, because a worker this frontend +// cannot route to produces exactly the same code as a backend that has died, +// and only one of the two should cost a row. +// // The gRPC client is lazy, so connection failures surface on the RPC rather // than at dial time, and the status code tells the two cases apart: // // - DeadlineExceeded: the transport was fine but nothing serviced the RPC in // time. That is a backend stuck inside a long synchronous request. -// - Unavailable: nothing is listening. The process is gone. +// - Unavailable: the worker was reached and nothing is listening on that +// port. The process is gone. // // A blackholed network also yields DeadlineExceeded and is therefore treated as // busy. That is deliberate: whole-node failures are the health monitor's job diff --git a/core/services/nodes/reconciler_prober_test.go b/core/services/nodes/reconciler_prober_test.go new file mode 100644 index 000000000..2bacef554 --- /dev/null +++ b/core/services/nodes/reconciler_prober_test.go @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: MIT + +package nodes + +import ( + "context" + "errors" + "fmt" + "net" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/mudler/LocalAI/core/services/cluster" + grpc "github.com/mudler/LocalAI/pkg/grpc" +) + +// proberFactory hands the prober one client, and records what it was asked for. +type proberFactory struct { + client grpc.Backend + err error + asked []string +} + +func (f *proberFactory) NewClientForNode(nodeID, address string, _ bool) (grpc.Backend, error) { + f.asked = append(f.asked, nodeID+"|"+address) + if f.err != nil { + return nil, f.err + } + return f.client, nil +} + +var _ = Describe("the reconciler's gRPC model prober", func() { + // The two lines that decide whether a row survives, both previously + // untested. Everything else in the reaper is driven through fakeProber, + // which means the mapping from a real client to a ProbeOutcome had nothing + // holding it at all. + probe := func(f *proberFactory) ProbeOutcome { + GinkgoHelper() + return grpcModelProber{clients: f}.Probe(context.Background(), "node-1", "10.0.0.1:9001") + } + + It("answers ProbeUnknown when no client can be built for the node", func() { + Expect(probe(&proberFactory{err: ErrNoWorkerDialer})).To(Equal(ProbeUnknown)) + }) + + It("answers ProbeUnknown when the client was built and the tunnel dial failed", func() { + // The likelier half. ProbeUnreachable here would delete the row after + // probeFailuresBeforeReap passes of a peer link that was merely + // restarting, and the backend would still be running the model. + Expect(probe(&proberFactory{client: &fakeBackendClient{ + healthy: false, + err: fmt.Errorf("rpc error: code = Unavailable"), + dialErr: fmt.Errorf("%w: %w", cluster.ErrNoRoute, cluster.ErrPeerUnreachable), + }})).To(Equal(ProbeUnknown)) + }) + + It("asks for the client by NODE, not by address alone", func() { + f := &proberFactory{client: &fakeBackendClient{healthy: true}} + Expect(probe(f)).To(Equal(ProbeAlive)) + Expect(f.asked).To(ContainElement("node-1|10.0.0.1:9001")) + }) + + It("answers ProbeAlive for a healthy backend it reached", func() { + Expect(probe(&proberFactory{client: &fakeBackendClient{healthy: true}})).To(Equal(ProbeAlive)) + }) + + It("still answers ProbeUnreachable for a dead backend on a worker it reached", func() { + // The other direction. The new check must not turn the reaper off: a + // backend that answered "unhealthy" over a working transport is a ghost + // and its row should go. + Expect(probe(&proberFactory{client: &fakeBackendClient{healthy: false}})).To(Equal(ProbeUnreachable)) + }) + + It("does not report a transport that recovered", func() { + // LastDialError is cleared by a successful dial, so a client that + // failed once and then reconnected must not keep reading as + // unroutable; otherwise a row could never be reaped again after one + // blip on that client. + listener, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { _ = listener.Close() }) + + attempt := 0 + f, err := NewTunnelClientFactory("", func(string) func(ctx context.Context, addr string) (net.Conn, error) { + var d net.Dialer + return func(ctx context.Context, _ string) (net.Conn, error) { + attempt++ + if attempt == 1 { + return nil, errors.New("first dial fails") + } + return d.DialContext(ctx, "tcp", listener.Addr().String()) + } + }) + Expect(err).ToNot(HaveOccurred()) + client, err := f.NewClientForNode("node-1", "10.0.0.1:9001", false) + Expect(err).ToNot(HaveOccurred()) + + _, _ = client.HealthCheck(context.Background()) + Expect(unroutable(client)).ToNot(BeNil()) + + // gRPC re-dials on the next call; the listener now accepts. + Eventually(func() error { + _, _ = client.HealthCheck(context.Background()) + return unroutable(client) + }, "20s").Should(BeNil()) + }) +}) diff --git a/core/services/nodes/router.go b/core/services/nodes/router.go index b9731f4a3..2845d7f98 100644 --- a/core/services/nodes/router.go +++ b/core/services/nodes/router.go @@ -1941,11 +1941,18 @@ func (r *SmartRouter) stageOptionDir(ctx context.Context, node *BackendNode, dir // (DecrementInFlight + RemoveNodeModel) still triggers on the next request. // // The client is built OUTSIDE the memoized closure, which is what keeps an -// unreachable worker out of the cache entirely: DoOrCached only ever sees a -// real answer. Building it costs a struct and no I/O, since the gRPC client +// unreachable worker out of the cache entirely: DoOrCachedResult only ever sees +// a real answer. Building it costs a struct and no I/O, since the gRPC client // dials lazily on its first call. +// +// The client is the RAW factory client rather than buildClientForAddr's, on +// purpose. A health check stages no files, so the staging wrapper buys nothing +// here; and the wrapper hides the transport, because it embeds grpc.Backend and +// so does not carry LastDialError through. Wrapping would leave this function +// unable to tell a dead backend from an unreachable worker, which is the whole +// question it now answers. func (r *SmartRouter) probeHealth(ctx context.Context, node *BackendNode, addr string) (alive, probed bool) { - client, err := r.buildClientForAddr(node, addr, false) + client, err := r.clientFactory.NewClientForNode(node.ID, addr, false) if err != nil { xlog.Error("Cannot probe a model backend: no way to reach the worker", "node", node.ID, "address", addr, "error", err) @@ -1954,12 +1961,24 @@ func (r *SmartRouter) probeHealth(ctx context.Context, node *BackendNode, addr s defer closeClient(client) key := node.ID + "|" + addr - return r.probeCache.DoOrCached(key, func() bool { + alive, unreached := r.probeCache.DoOrCachedResult(key, func() (bool, error) { checkCtx, cancel := context.WithTimeout(ctx, 2*time.Second) defer cancel() ok, _ := client.HealthCheck(checkCtx) - return ok - }), true + if ok { + return true, nil + } + // The RPC failed. gRPC reports a dead backend and an unreachable + // worker with the same code, so the only way to tell them apart is to + // ask the transport whether it was the one that failed. + return false, unroutable(client) + }) + if unreached != nil { + xlog.Warn("Could not probe a model backend: no route to the worker", + "node", node.ID, "address", addr, "error", unreached) + return false, false + } + return alive, true } // closeClient closes a gRPC backend client if it implements io.Closer. diff --git a/core/services/nodes/router_unreachable_worker_test.go b/core/services/nodes/router_unreachable_worker_test.go index 943741396..206956e11 100644 --- a/core/services/nodes/router_unreachable_worker_test.go +++ b/core/services/nodes/router_unreachable_worker_test.go @@ -4,20 +4,38 @@ package nodes import ( "context" - "errors" + "fmt" + "net" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/mudler/LocalAI/core/services/cluster" grpc "github.com/mudler/LocalAI/pkg/grpc" ) // unreachableClientFactory cannot build a client for any node, standing in for -// a frontend whose worker tunnel dialer is missing or broken. +// a frontend whose worker tunnel dialer is missing or broken. This is the +// BOOT-TIME half of unroutability. type unreachableClientFactory struct{} func (unreachableClientFactory) NewClientForNode(_, _ string, _ bool) (grpc.Backend, error) { - return nil, errors.New("no way to reach that worker") + return nil, ErrNoWorkerDialer +} + +// deadDialFactory builds clients normally and fails the DIAL, which is the +// RUNNING half and by far the likelier one: the factory only fails when the +// wiring is absent, while the dial fails whenever the replica holding a +// worker's tunnel is momentarily unreachable, which one frontend restart +// produces for every worker that replica holds. +func deadDialFactory(cause error) BackendClientFactory { + GinkgoHelper() + f, err := NewTunnelClientFactory("", func(string) func(ctx context.Context, addr string) (net.Conn, error) { + return func(context.Context, string) (net.Conn, error) { return nil, cause } + }) + Expect(err).ToNot(HaveOccurred()) + return f } var _ = Describe("routing when the worker cannot be reached at all", func() { @@ -68,3 +86,99 @@ var _ = Describe("routing when the worker cannot be reached at all", func() { Expect(reg.decrementCalls).To(ContainElement("X:m")) }) }) + +var _ = Describe("routing when the worker's tunnel dial fails", func() { + // The reviewer's spec. It is the boundary test: the factory succeeds, the + // gRPC client is built, and the DIAL fails underneath with a + // cluster.ErrNoRoute. gRPC flattens that into codes.Unavailable, which is + // also what a dead backend produces, so without a way to carry the + // distinction past the package boundary a peer link blip is read as a dead + // process and the replica row is deleted after ONE miss. + loadedReg := func() *fakeModelRouter { + node := &BackendNode{ID: "X", Name: "node-x", Address: "10.0.0.1:50051"} + nm := &NodeModel{NodeID: "X", ModelName: "m", Address: "10.0.0.1:9001"} + return &fakeModelRouter{ + findAndLockNode: node, + findAndLockNM: nm, + loadedReplicaStatsByName: map[string][]ReplicaCandidate{"m": {{NodeID: "X", InFlight: 0}}}, + } + } + + route := func(reg *fakeModelRouter, cause error) error { + router := NewSmartRouter(reg, SmartRouterOptions{ + Unloader: &fakeUnloader{}, + ClientFactory: deadDialFactory(cause), + }) + _, err := router.Route(context.Background(), "m", "models/m.gguf", "llama-cpp", "", nil, false) + return err + } + + It("never reaps a replica whose OWNER replica is unreachable", func() { + reg := loadedReg() + Expect(route(reg, fmt.Errorf("through replica %q: %w: %w", "peer-2", cluster.ErrNoRoute, cluster.ErrPeerUnreachable))).To(HaveOccurred()) + Expect(reg.removeCalls).To(BeEmpty(), + "a worker whose OWNER replica is unreachable must never have its loaded models reaped") + }) + + It("never reaps a replica that has not dialled its tunnel yet", func() { + // The rolling-upgrade case end to end. A frontend-first upgrade puts + // every not-yet-restarted worker here at once, and every one of them is + // heartbeating and serving while it happens. + reg := loadedReg() + Expect(route(reg, fmt.Errorf("reaching node %q: %w", "X", cluster.ErrNoRoute))).To(HaveOccurred()) + Expect(reg.removeCalls).To(BeEmpty()) + }) + + It("still releases the reservation it took", func() { + reg := loadedReg() + Expect(route(reg, fmt.Errorf("%w", cluster.ErrNoRoute))).To(HaveOccurred()) + Expect(reg.decrementCalls).To(ContainElement("X:m")) + }) + + It("carries the cluster condition all the way across the package boundary", func() { + // Not just "something failed": the specific reason survives gRPC, which + // is what makes the five conditions usable on this side. If this ever + // reduces to a bare code, the consumers above are guessing again. + f := deadDialFactory(fmt.Errorf("through replica %q: %w: %w", "peer-2", cluster.ErrNoRoute, cluster.ErrPeerUnreachable)) + client, err := f.NewClientForNode("X", "10.0.0.1:9001", false) + Expect(err).ToNot(HaveOccurred()) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, _ = client.HealthCheck(ctx) + + unreached := unroutable(client) + Expect(unreached).To(MatchError(ErrWorkerUnroutable)) + Expect(unreached).To(MatchError(cluster.ErrNoRoute)) + Expect(unreached).To(MatchError(cluster.ErrPeerUnreachable)) + // And never absence, at either end of the trip. + Expect(unreached).ToNot(MatchError(cluster.ErrNoConnection)) + Expect(unreached).ToNot(MatchError(cluster.ErrInstanceNotFound)) + }) + + It("reports nothing for a client whose dial succeeded", func() { + // The other direction, so the seam cannot pass by always saying yes: a + // backend that genuinely died must still be reapable. + listener, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { _ = listener.Close() }) + + f, err := NewTunnelClientFactory("", func(string) func(ctx context.Context, addr string) (net.Conn, error) { + var d net.Dialer + return func(ctx context.Context, _ string) (net.Conn, error) { + return d.DialContext(ctx, "tcp", listener.Addr().String()) + } + }) + Expect(err).ToNot(HaveOccurred()) + client, err := f.NewClientForNode("X", "10.0.0.1:9001", false) + Expect(err).ToNot(HaveOccurred()) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + // The listener accepts and speaks no gRPC, so the RPC fails while the + // DIAL succeeds. That is exactly a dead-ish backend on a reachable + // worker, and it must not read as unroutable. + _, _ = client.HealthCheck(ctx) + Expect(unroutable(client)).To(BeNil()) + }) +}) diff --git a/docs/content/features/distributed-mode.md b/docs/content/features/distributed-mode.md index d73cdf6b6..4f10775e1 100644 --- a/docs/content/features/distributed-mode.md +++ b/docs/content/features/distributed-mode.md @@ -174,7 +174,16 @@ Four outcomes are kept apart on purpose, because they call for different actions Only the first is absence. The others are never reported as it, and that is not a stylistic preference: a scheduler told that a connected worker has gone away reclaims every model it is running. -Set `LOCALAI_WORKER_TUNNEL=false` on a worker to turn the tunnel off and go back to the frontend dialling the worker's advertised addresses. +#### There is no frontend-side fallback, and upgrade order matters + +`LOCALAI_WORKER_TUNNEL=false` still stops a worker dialling its tunnel, but it no longer has a frontend counterpart: after this change **no frontend path dials a worker's advertised address**, so a worker with the tunnel off is a worker the frontend cannot reach. Setting it is not a rollback. The rollback is to run the previous frontend release. + +That makes upgrade order matter, in one direction only: + +- **Upgrade the workers first, then the frontends.** A worker on the new build dials its tunnel and is reachable by frontends of either version, because the old frontend still dials its advertised address and the worker still listens. +- **Upgrading the frontends first** leaves every not-yet-restarted worker unroutable until it restarts. Those workers keep running their models and keep heartbeating, and the frontend reports them as unroutable rather than as gone: their `node_models` rows are left alone, nothing is rescheduled, and requests for those models fail loudly with "no route" until the worker reconnects. It is a degraded window, not an eviction, but it is a window, and doing it the other way round has none. + +A worker that cannot reach its frontend retries with exponential backoff and never gives up, so restarting a worker is all that is needed to close the window. ### The model load deadline scales with the checkpoint @@ -399,7 +408,7 @@ local-ai worker \ | `--registration-require-auth` | `LOCALAI_REGISTRATION_REQUIRE_AUTH` | `false` | Refuse to start the HTTP file-transfer server when no registration token is set (it would otherwise fail open) | | `--distributed-require-auth` | `LOCALAI_DISTRIBUTED_REQUIRE_AUTH` | `false` | Umbrella switch implying both `--registration-require-auth` and `--nats-require-auth` | | `--heartbeat-interval` | `LOCALAI_HEARTBEAT_INTERVAL` | `10s` | Interval between heartbeat pings | -| `--worker-tunnel` | `LOCALAI_WORKER_TUNNEL` | `true` | Hold one outbound multiplexed tunnel to the frontend and serve its requests over it, so this worker needs no inbound port (see [Worker tunnels](#worker-tunnels)) | +| `--worker-tunnel` | `LOCALAI_WORKER_TUNNEL` | `true` | Hold one outbound multiplexed tunnel to the frontend and serve its requests over it, so this worker needs no inbound port (see [Worker tunnels](#worker-tunnels)). Turning it off makes the worker unreachable: the frontend has no path that dials a worker's advertised address. | | `--nats-url` | `LOCALAI_NATS_URL` | *(required)* | NATS URL for backend installation and file staging | | `--nats-jwt` | `LOCALAI_NATS_JWT` | *(empty)* | Optional override for the `nats_jwt` returned at registration | | `--nats-user-seed` | `LOCALAI_NATS_USER_SEED` | *(empty)* | Optional override for `nats_user_seed` from registration | diff --git a/pkg/grpc/backend.go b/pkg/grpc/backend.go index e90bbb28f..2bb013de0 100644 --- a/pkg/grpc/backend.go +++ b/pkg/grpc/backend.go @@ -49,10 +49,29 @@ func NewClientWithDialer(address string, parallel bool, wd WatchDog, enableWatch // the address directly, which is the exact bypass this constructor exists // to close. c := buildClient(address, parallel, wd, enableWatchDog, token) - c.dialer = dialer + // Wrapped rather than stored bare, so every dial outcome is recorded. This + // is the seam that carries the reason a dial failed past gRPC, which + // flattens it into codes.Unavailable; see (*Client).LastDialError. + c.dialer = func(ctx context.Context, addr string) (net.Conn, error) { + conn, err := dialer(ctx, addr) + c.recordDialErr(err) + return conn, err + } return c } +// DialErrorReporter is implemented by a Backend that reaches its process +// through a custom transport and can say whether that transport, rather than +// the process, is what failed. +// +// It is a separate interface and NOT part of Backend on purpose: only the +// handful of callers that act on the difference need it, and widening Backend +// would make every wrapper and every test double implement a method they have +// no answer for. +type DialErrorReporter interface { + LastDialError() error +} + func buildClient(address string, parallel bool, wd WatchDog, enableWatchDog bool, token string) *Client { if !enableWatchDog { wd = nil diff --git a/pkg/grpc/client.go b/pkg/grpc/client.go index 6131d3801..e132086a0 100644 --- a/pkg/grpc/client.go +++ b/pkg/grpc/client.go @@ -40,6 +40,14 @@ type Client struct { // keeps gRPC's own TCP dial, which is what every non-distributed caller // wants. dialer func(ctx context.Context, addr string) (net.Conn, error) + + // dialErrMu guards lastDialErr. Its own mutex rather than the embedded one: + // the embedded Mutex guards inFlight and is taken on every call, and a + // dialer runs underneath gRPC's own machinery where reentering it is not + // something this type can reason about. + dialErrMu sync.Mutex + lastDialErr error + sync.Mutex opMutex sync.Mutex wd WatchDog @@ -1422,3 +1430,40 @@ func (c *Client) ModelMetadata(ctx context.Context, in *pb.ModelOptions, opts .. client := pb.NewBackendClient(conn) return client.ModelMetadata(ctx, in, opts...) } + +// LastDialError returns the error from the most recent attempt by this client's +// custom dialer, or nil when the last attempt succeeded or there is no custom +// dialer. +// +// It exists because gRPC destroys the distinction its callers need. A dialer +// failure reaches an RPC as codes.Unavailable with the cause flattened into a +// message string, and codes.Unavailable is ALSO what a backend process that +// died produces. Those two call for opposite actions: a dead backend's registry +// row should be reaped, and a transport that could not reach a live backend +// must never cause one to be. Recording the error here is what lets a caller +// tell them apart, with the original error VALUE intact, so +// core/services/cluster's sentinels survive the trip. +// +// Scope, stated exactly. This is the last dial on this CLIENT, not the last +// dial for a particular RPC. A client used for one probe and closed gives exact +// attribution, which is how every reaping path in core/services/nodes uses it. +// A client shared across concurrent RPCs can attribute a dial failure to the +// wrong one; both directions of that error are safe, because a caller consults +// this only when its RPC already failed, and the outcomes are "treat a dead +// backend as unreachable-for-now" (the row survives one extra round) or "treat +// a transport failure as a backend failure" (the behaviour before this +// existed). +func (c *Client) LastDialError() error { + c.dialErrMu.Lock() + defer c.dialErrMu.Unlock() + return c.lastDialErr +} + +// recordDialErr stores the outcome of one dial. A success CLEARS the previous +// failure rather than leaving it, so a client that recovered does not keep +// reporting a dial error that no longer describes anything. +func (c *Client) recordDialErr(err error) { + c.dialErrMu.Lock() + c.lastDialErr = err + c.dialErrMu.Unlock() +} diff --git a/pkg/model/loader.go b/pkg/model/loader.go index 322b11e36..1193a18d8 100644 --- a/pkg/model/loader.go +++ b/pkg/model/loader.go @@ -12,6 +12,7 @@ import ( "sync/atomic" "time" + grpc "github.com/mudler/LocalAI/pkg/grpc" pb "github.com/mudler/LocalAI/pkg/grpc/proto" "github.com/mudler/LocalAI/pkg/system" "github.com/mudler/LocalAI/pkg/utils" @@ -685,6 +686,20 @@ func (ml *ModelLoader) checkIsLoaded(s string) *Model { // Remote/distributed model — no local process to check. // Only evict on definitive connection errors (node is down). // Timeouts may mean the node is busy, so keep the model cached. + // + // "The node is down" is exactly what this can no longer conclude on + // its own. In distributed mode the client reaches the backend over + // the worker's tunnel, and a failure of THAT transport (the replica + // holding the tunnel is restarting, the worker has not dialled in + // yet after a frontend-first upgrade) arrives as the same + // codes.Unavailable a dead worker produces. Evicting on it would + // unload a model that is loaded and serving. The client records + // which of the two happened; see grpc.DialErrorReporter. + if dialErr := transportFailure(client); dialErr != nil { + xlog.Warn("Remote model health check could not reach the worker, keeping cached", + "model", s, "error", dialErr) + return m + } if isConnectionError(err) { xlog.Warn("Remote model unreachable (connection error), removing from cache", "model", s, "error", err) if delErr := ml.deleteProcess(cTimeout, s, false); delErr != nil { @@ -709,3 +724,20 @@ func (ml *ModelLoader) checkIsLoaded(s string) *Model { m.MarkHealthy() return m } + +// transportFailure reports why a call never reached the backend, or nil when it +// did reach one. +// +// It is the one question that separates "this backend is gone" from "this +// process cannot currently get to it", and gRPC does not answer it: a dialer +// failure and a dead listener both surface as codes.Unavailable. A client with +// no custom transport answers nil, which is right for every locally spawned +// backend, where the address IS a socket on this machine and a failed +// connection really does mean the process died. +func transportFailure(client grpc.Backend) error { + reporter, ok := client.(grpc.DialErrorReporter) + if !ok { + return nil + } + return reporter.LastDialError() +} diff --git a/pkg/model/remote_unroutable_internal_test.go b/pkg/model/remote_unroutable_internal_test.go new file mode 100644 index 000000000..25a440d80 --- /dev/null +++ b/pkg/model/remote_unroutable_internal_test.go @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: MIT + +package model + +import ( + "context" + "errors" + "net" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + grpc "github.com/mudler/LocalAI/pkg/grpc" + "github.com/mudler/LocalAI/pkg/system" +) + +var _ = Describe("the health check on a remote model whose transport failed", func() { + // The fourth site of the same shape as the reconciler, the health monitor + // and the router, found by sweeping rather than by being named. + // + // checkIsLoaded evicts a remote model on a "connection error", which used + // to mean exactly one thing: the worker's socket did not answer. In + // distributed mode the client reaches the backend over the worker's tunnel, + // and a failure of THAT transport arrives as the same codes.Unavailable. + // Evicting on it unloads a model that is loaded and serving, on a worker + // that is heartbeating. + var ml *ModelLoader + + BeforeEach(func() { + systemState, err := system.GetSystemState(system.WithModelPath(GinkgoT().TempDir())) + Expect(err).ToNot(HaveOccurred()) + ml = NewModelLoader(systemState) + }) + + It("keeps the model when the tunnel dial failed", func() { + client := grpc.NewClientWithDialer("10.0.0.1:9001", false, nil, false, "", + func(context.Context, string) (net.Conn, error) { + return nil, errors.New("cluster: no route from this replica to that worker") + }) + m := NewModelWithClient("remote-model", "10.0.0.1:9001", client) + ml.store.Set("remote-model", m) + + Expect(ml.checkIsLoaded("remote-model")).To(BeIdenticalTo(m), + "a model on a worker this frontend cannot route to must stay cached, not be unloaded") + _, stillThere := ml.store.Get("remote-model") + Expect(stillThere).To(BeTrue()) + }) + + It("still evicts a remote model whose worker WAS reached and did not answer", func() { + // The other direction, so the new check cannot pass by never evicting. + // No custom dialer, so the transport reports nothing and a connection + // error means what it always meant. + client := grpc.NewClientWithToken("127.0.0.1:1", false, nil, false, "") + m := NewModelWithClient("dead-model", "127.0.0.1:1", client) + ml.store.Set("dead-model", m) + + Expect(ml.checkIsLoaded("dead-model")).To(BeNil()) + _, stillThere := ml.store.Get("dead-model") + Expect(stillThere).To(BeFalse()) + }) +})