From 115aeb47c0f9ea4ca383c149059e68f10e6cf4a9 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 09:59:26 +0000 Subject: [PATCH 01/23] test(distributed): share suite containers, isolate specs by database Starting a Postgres and a NATS container per spec cost roughly 48 minutes of startup across the 213 specs behind SetupInfra, which is why this suite was never wired into CI. Containers move to BeforeSuite and isolation comes from CREATE DATABASE, which the dbName argument already described. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/dbname_test.go | 33 +++++ tests/e2e/distributed/testhelpers_test.go | 167 +++++++++++++++++----- 2 files changed, 166 insertions(+), 34 deletions(-) create mode 100644 tests/e2e/distributed/dbname_test.go diff --git a/tests/e2e/distributed/dbname_test.go b/tests/e2e/distributed/dbname_test.go new file mode 100644 index 000000000000..76f1030747fa --- /dev/null +++ b/tests/e2e/distributed/dbname_test.go @@ -0,0 +1,33 @@ +package distributed_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Test database naming", Label("Distributed"), func() { + Describe("sanitizeDBName", func() { + It("lowercases and replaces characters Postgres will not accept unquoted", func() { + Expect(sanitizeDBName("LocalAI-Test.Suite")).To(Equal("localai_test_suite")) + }) + + It("truncates to fit the 63-byte identifier limit with room for a suffix", func() { + long := "" + for i := 0; i < 100; i++ { + long += "a" + } + Expect(len(sanitizeDBName(long))).To(BeNumerically("<=", 50)) + }) + + It("never produces an empty name", func() { + Expect(sanitizeDBName("---")).ToNot(BeEmpty()) + }) + }) + + Describe("replaceDBName", func() { + It("swaps the database in a testcontainers DSN and keeps the query string", func() { + dsn := "postgres://test:test@127.0.0.1:32768/localai_suite?sslmode=disable" + Expect(replaceDBName(dsn, "spec_7")).To(Equal("postgres://test:test@127.0.0.1:32768/spec_7?sslmode=disable")) + }) + }) +}) diff --git a/tests/e2e/distributed/testhelpers_test.go b/tests/e2e/distributed/testhelpers_test.go index 68cf537e30bd..564e8c40603a 100644 --- a/tests/e2e/distributed/testhelpers_test.go +++ b/tests/e2e/distributed/testhelpers_test.go @@ -2,6 +2,10 @@ package distributed_test import ( "context" + "fmt" + "net/url" + "strings" + "sync/atomic" "time" "github.com/mudler/LocalAI/core/services/messaging" @@ -13,6 +17,9 @@ import ( tcnats "github.com/testcontainers/testcontainers-go/modules/nats" tcpostgres "github.com/testcontainers/testcontainers-go/modules/postgres" "github.com/testcontainers/testcontainers-go/wait" + "gorm.io/driver/postgres" + "gorm.io/gorm" + gormlogger "gorm.io/gorm/logger" ) // TestInfra holds shared test containers and connection strings. @@ -25,71 +32,167 @@ type TestInfra struct { NC *messaging.Client } -// SetupInfra starts PostgreSQL and NATS containers and connects a messaging client. -// Call in BeforeEach. Use DeferCleanup or call Teardown in AfterEach. -func SetupInfra(dbName string) *TestInfra { - GinkgoHelper() +// Containers are suite-scoped, not spec-scoped. Starting a Postgres (~10s) and a +// NATS (~3.5s) per spec cost roughly 48 minutes of pure startup across the 213 +// specs behind SetupInfra, which is why this suite was never wired into CI. +// Isolation now comes from a database per spec (~67ms), which is what the dbName +// argument was always describing. +// +// Plain BeforeSuite rather than SynchronizedBeforeSuite is deliberate: under +// `ginkgo -p` each process gets its own container pair, which keeps NATS subjects +// isolated per process. A single shared NATS across parallel processes would let +// specs on different processes see each other's messages on the same subject. +var ( + suitePG *tcpostgres.PostgresContainer + suiteNATS *tcnats.NATSContainer + suitePGDSN string + suiteNatsURL string + dbCounter atomic.Int64 +) - infra := &TestInfra{Ctx: context.Background()} +var _ = BeforeSuite(func() { + ctx := context.Background() var err error - // Start PostgreSQL container - infra.PGContainer, err = tcpostgres.Run(infra.Ctx, "postgres:16-alpine", - tcpostgres.WithDatabase(dbName), + suitePG, err = tcpostgres.Run(ctx, "postgres:16-alpine", + tcpostgres.WithDatabase("localai_suite"), tcpostgres.WithUsername("test"), tcpostgres.WithPassword("test"), testcontainers.WithWaitStrategy( wait.ForLog("database system is ready to accept connections"). WithOccurrence(2). - WithStartupTimeout(30*time.Second), + WithStartupTimeout(90*time.Second), ), ) Expect(err).ToNot(HaveOccurred()) - infra.PGURL, err = infra.PGContainer.ConnectionString(infra.Ctx, "sslmode=disable") + suitePGDSN, err = suitePG.ConnectionString(ctx, "sslmode=disable") + Expect(err).ToNot(HaveOccurred()) + + suiteNATS, err = tcnats.Run(ctx, "nats:2-alpine") + Expect(err).ToNot(HaveOccurred()) + + suiteNatsURL, err = suiteNATS.ConnectionString(ctx) Expect(err).ToNot(HaveOccurred()) +}) + +var _ = AfterSuite(func() { + ctx := context.Background() + if suitePG != nil { + _ = suitePG.Terminate(ctx) + } + if suiteNATS != nil { + _ = suiteNATS.Terminate(ctx) + } +}) + +// sanitizeDBName maps a spec-supplied label onto a legal unquoted Postgres +// identifier, leaving headroom for the uniqueness suffix appended by SetupInfra. +func sanitizeDBName(name string) string { + var b strings.Builder + for _, r := range strings.ToLower(name) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '_': + b.WriteRune(r) + default: + b.WriteRune('_') + } + } + out := strings.Trim(b.String(), "_") + if out == "" { + out = "spec" + } + // Postgres identifiers cap at 63 bytes; reserve the rest for "_". + if len(out) > 50 { + out = out[:50] + } + return out +} - // Start NATS container - infra.NATSContainer, err = tcnats.Run(infra.Ctx, "nats:2-alpine") +// replaceDBName swaps the database component of a DSN, preserving credentials, +// host, port and query parameters. +func replaceDBName(dsn, name string) string { + GinkgoHelper() + u, err := url.Parse(dsn) Expect(err).ToNot(HaveOccurred()) + u.Path = "/" + name + return u.String() +} - infra.NatsURL, err = infra.NATSContainer.ConnectionString(infra.Ctx) +// adminDB opens a short-lived connection to the suite's maintenance database. +// CREATE/DROP DATABASE cannot run inside a transaction or against the target +// database itself, so every call gets its own connection and closes it. +func adminDB() *gorm.DB { + GinkgoHelper() + db, err := gorm.Open(postgres.Open(suitePGDSN), &gorm.Config{Logger: gormlogger.Discard}) Expect(err).ToNot(HaveOccurred()) + return db +} + +func closeDB(db *gorm.DB) { + if db == nil { + return + } + if sqlDB, err := db.DB(); err == nil { + _ = sqlDB.Close() + } +} - // Connect messaging client +// SetupInfra provisions a dedicated database on the suite-scoped Postgres and +// returns a client connected to the suite-scoped NATS. Call in BeforeEach; +// cleanup is registered with DeferCleanup. +func SetupInfra(dbName string) *TestInfra { + GinkgoHelper() + Expect(suitePG).ToNot(BeNil(), "SetupInfra called before BeforeSuite started the shared containers") + + infra := &TestInfra{ + Ctx: context.Background(), + PGContainer: suitePG, + NATSContainer: suiteNATS, + NatsURL: suiteNatsURL, + } + + db := fmt.Sprintf("%s_%d", sanitizeDBName(dbName), dbCounter.Add(1)) + + admin := adminDB() + Expect(admin.Exec(fmt.Sprintf("CREATE DATABASE %q", db)).Error).To(Succeed()) + closeDB(admin) + + infra.PGURL = replaceDBName(suitePGDSN, db) + + var err error infra.NC, err = messaging.New(infra.NatsURL) Expect(err).ToNot(HaveOccurred()) - // Register cleanup in LIFO order DeferCleanup(func() { if infra.NC != nil { infra.NC.Close() } - if infra.PGContainer != nil { - infra.PGContainer.Terminate(context.Background()) - } - if infra.NATSContainer != nil { - infra.NATSContainer.Terminate(context.Background()) + // FORCE terminates any connection the spec left open (Postgres 13+). + // Failure to drop must not fail the spec: the container dies at AfterSuite. + drop := adminDB() + if err := drop.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %q WITH (FORCE)", db)).Error; err != nil { + AddReportEntry("drop database failed", fmt.Sprintf("%s: %v", db, err)) } + closeDB(drop) }) return infra } -// SetupNATSOnly starts only a NATS container and connects a messaging client. -// Useful for tests that don't need PostgreSQL. +// SetupNATSOnly returns a client on the suite-scoped NATS for specs that need no +// database. func SetupNATSOnly() *TestInfra { GinkgoHelper() + Expect(suiteNATS).ToNot(BeNil(), "SetupNATSOnly called before BeforeSuite started the shared containers") - infra := &TestInfra{Ctx: context.Background()} - var err error - - infra.NATSContainer, err = tcnats.Run(infra.Ctx, "nats:2-alpine") - Expect(err).ToNot(HaveOccurred()) - - infra.NatsURL, err = infra.NATSContainer.ConnectionString(infra.Ctx) - Expect(err).ToNot(HaveOccurred()) + infra := &TestInfra{ + Ctx: context.Background(), + NATSContainer: suiteNATS, + NatsURL: suiteNatsURL, + } + var err error infra.NC, err = messaging.New(infra.NatsURL) Expect(err).ToNot(HaveOccurred()) @@ -97,16 +200,12 @@ func SetupNATSOnly() *TestInfra { if infra.NC != nil { infra.NC.Close() } - if infra.NATSContainer != nil { - infra.NATSContainer.Terminate(context.Background()) - } }) return infra } // FlushNATS ensures all subscriptions are registered server-side before publishing. -// Replaces time.Sleep(100ms) after Subscribe calls. func FlushNATS(nc *messaging.Client) { GinkgoHelper() Expect(nc.Conn().Flush()).To(Succeed()) From 1974bc1ea0bda8ddb11fadfd55abcdacde3ef964 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 10:08:17 +0000 Subject: [PATCH 02/23] test(distributed): stop leaking admin pools when database setup fails A failed CREATE DATABASE panics out of the assertion before closeDB runs, leaking a pgx pool per attempt. With --flake-attempts 5 that exhausts postgres:16-alpine's 100 connection slots, at which point the cleanup path's own Expect fails the spec and one hiccup cascades across the suite. Scope the admin handle so the panic unwinds through defer closeDB, and let cleanup use a fallible tryAdminDB that reports rather than asserts. Register DeferCleanup immediately after CREATE so a later failure cannot leave the database behind, and warn on TestInfra that the container handles are now suite-wide. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/testhelpers_test.go | 54 +++++++++++++++++------ 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/tests/e2e/distributed/testhelpers_test.go b/tests/e2e/distributed/testhelpers_test.go index 564e8c40603a..17ee72ce2e89 100644 --- a/tests/e2e/distributed/testhelpers_test.go +++ b/tests/e2e/distributed/testhelpers_test.go @@ -23,6 +23,11 @@ import ( ) // TestInfra holds shared test containers and connection strings. +// +// PGContainer and NATSContainer are the SUITE-WIDE containers, shared by every +// spec. Never call Terminate or Stop on them from a spec: it ends the run for +// everything after it. They are exposed only because nats_jwt_helpers_test.go +// builds its own TestInfra around a dedicated NATS container. type TestInfra struct { Ctx context.Context PGContainer *tcpostgres.PostgresContainer @@ -119,12 +124,24 @@ func replaceDBName(dsn, name string) string { return u.String() } -// adminDB opens a short-lived connection to the suite's maintenance database. +// tryAdminDB opens a short-lived connection to the suite's maintenance +// database. Cleanup paths use this rather than adminDB: once connections are +// scarce, a fatal assertion here would convert one Postgres hiccup into a +// suite-wide cascade that buries the original failure. +// // CREATE/DROP DATABASE cannot run inside a transaction or against the target // database itself, so every call gets its own connection and closes it. +func tryAdminDB() (*gorm.DB, error) { + db, err := gorm.Open(postgres.Open(suitePGDSN), &gorm.Config{Logger: gormlogger.Discard}) + if err != nil { + return nil, fmt.Errorf("connecting to the suite maintenance database: %w", err) + } + return db, nil +} + func adminDB() *gorm.DB { GinkgoHelper() - db, err := gorm.Open(postgres.Open(suitePGDSN), &gorm.Config{Logger: gormlogger.Discard}) + db, err := tryAdminDB() Expect(err).ToNot(HaveOccurred()) return db } @@ -154,29 +171,38 @@ func SetupInfra(dbName string) *TestInfra { db := fmt.Sprintf("%s_%d", sanitizeDBName(dbName), dbCounter.Add(1)) - admin := adminDB() - Expect(admin.Exec(fmt.Sprintf("CREATE DATABASE %q", db)).Error).To(Succeed()) - closeDB(admin) - - infra.PGURL = replaceDBName(suitePGDSN, db) - - var err error - infra.NC, err = messaging.New(infra.NatsURL) - Expect(err).ToNot(HaveOccurred()) + // Scoped so a failed CREATE cannot leak the pool: the assertion panics, and a + // leaked pgx pool per failing spec exhausts the server's connection limit. + func() { + admin := adminDB() + defer closeDB(admin) + Expect(admin.Exec(fmt.Sprintf("CREATE DATABASE %q", db)).Error).To(Succeed()) + }() + // Registered before anything else can fail: a NATS connect error below would + // otherwise leave the database behind for the rest of the suite. DeferCleanup(func() { if infra.NC != nil { infra.NC.Close() } + drop, err := tryAdminDB() + if err != nil { + AddReportEntry("drop database skipped", fmt.Sprintf("%s: %v", db, err)) + return + } + defer closeDB(drop) // FORCE terminates any connection the spec left open (Postgres 13+). - // Failure to drop must not fail the spec: the container dies at AfterSuite. - drop := adminDB() if err := drop.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %q WITH (FORCE)", db)).Error; err != nil { AddReportEntry("drop database failed", fmt.Sprintf("%s: %v", db, err)) } - closeDB(drop) }) + infra.PGURL = replaceDBName(suitePGDSN, db) + + var err error + infra.NC, err = messaging.New(infra.NatsURL) + Expect(err).ToNot(HaveOccurred()) + return infra } From f0fa4a7b1fdd67e3bb0fa951a91af21b21820122 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 10:31:51 +0000 Subject: [PATCH 03/23] test(distributed): wait for the log subscriber instead of racing it The WebSocket log handler writes its "initial" batch before it calls Subscribe, so a line appended the instant that batch arrives lands in the circular buffer with no subscriber to receive it. Three backend-logs specs append exactly there and then wait out a 5s read deadline; once a gorilla read hits its deadline the connection is unusable, so the spec cannot retry. `--focus='Worker WebSocket log streaming' --repeat=25` failed on attempt 17 with nothing else running, which is far too often to wire into CI. Add BackendLogStore.SubscriberCount, resolving a model ID by the same exact-key and replica-prefix rules Subscribe uses, and have the specs poll it until the handler has attached. Nothing in production calls it and no assertion is weakened; the handler's own snapshot/subscribe window is left as it is, being a production streaming question rather than a test one. Verified with 60 repeats of the WebSocket specs and three consecutive --randomize-all runs of the whole distributed suite, all at --flake-attempts 1: 239 of 240 specs pass in about 80 seconds. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- pkg/model/backend_log_store.go | 39 ++++++++++++++++++++++ pkg/model/backend_log_store_test.go | 32 ++++++++++++++++++ tests/e2e/distributed/backend_logs_test.go | 18 ++++++++++ 3 files changed, 89 insertions(+) diff --git a/pkg/model/backend_log_store.go b/pkg/model/backend_log_store.go index c5b5253ddc40..30268fde47de 100644 --- a/pkg/model/backend_log_store.go +++ b/pkg/model/backend_log_store.go @@ -344,3 +344,42 @@ func (s *BackendLogStore) Subscribe(modelID string) (chan BackendLogLine, func() return ch, unsubscribe } + +// SubscriberCount reports how many live subscriptions exist for modelID, +// resolving the ID with the same exact-key / replica-prefix rules as Subscribe. +// +// Streaming handlers send a GetLines snapshot before they call Subscribe, so a +// line appended between those two calls reaches the buffer but no channel. A +// caller that has to observe a line it appends itself must therefore wait for +// the subscription to exist rather than assume the handler got there first. +func (s *BackendLogStore) SubscriberCount(modelID string) int { + s.mu.RLock() + exact, exactOK := s.buffers[modelID] + var replicas []*backendLogBuffer + if !strings.Contains(modelID, replicaSeparator) { + prefix := modelID + replicaSeparator + for k, b := range s.buffers { + if strings.HasPrefix(k, prefix) { + replicas = append(replicas, b) + } + } + } + s.mu.RUnlock() + + // Counted after releasing s.mu: no other path takes s.mu and a buffer lock + // together, and keeping it that way costs nothing here. + count := func(buf *backendLogBuffer) int { + buf.mu.Lock() + defer buf.mu.Unlock() + return len(buf.subscribers) + } + + total := 0 + if exactOK { + total += count(exact) + } + for _, b := range replicas { + total += count(b) + } + return total +} diff --git a/pkg/model/backend_log_store_test.go b/pkg/model/backend_log_store_test.go index 775e07cdb561..593bcf5a23fa 100644 --- a/pkg/model/backend_log_store_test.go +++ b/pkg/model/backend_log_store_test.go @@ -76,6 +76,38 @@ var _ = Describe("BackendLogStore", func() { }) }) + Describe("SubscriberCount", func() { + It("reports zero before anyone subscribes and drops back after unsubscribe", func() { + s.AppendLine("model-a", "stderr", "preload") + Expect(s.SubscriberCount("model-a")).To(Equal(0)) + + _, unsubscribe := s.Subscribe("model-a") + Expect(s.SubscriberCount("model-a")).To(Equal(1)) + + unsubscribe() + Expect(s.SubscriberCount("model-a")).To(Equal(0)) + }) + + // Subscribe resolves a bare model ID across every replica buffer, so the + // count has to follow the same rule or a caller waiting on it would give + // up while a perfectly good subscription was in place. + It("sums the replica buffers a bare model ID resolves to", func() { + s.AppendLine("model-a#0", "stderr", "preload-r0") + s.AppendLine("model-a#1", "stderr", "preload-r1") + + _, unsubscribe := s.Subscribe("model-a") + defer unsubscribe() + + Expect(s.SubscriberCount("model-a")).To(Equal(2)) + Expect(s.SubscriberCount("model-a#0")).To(Equal(1)) + Expect(s.SubscriberCount("model-b")).To(Equal(0)) + }) + + It("returns zero for a model that has no buffer at all", func() { + Expect(s.SubscriberCount("never-seen")).To(Equal(0)) + }) + }) + Describe("Subscribe", func() { // Confirms the WebSocket streaming path (the live tail UI) receives // lines from every replica when the caller subscribes by bare modelID. diff --git a/tests/e2e/distributed/backend_logs_test.go b/tests/e2e/distributed/backend_logs_test.go index 79dea3902d01..473dad54f111 100644 --- a/tests/e2e/distributed/backend_logs_test.go +++ b/tests/e2e/distributed/backend_logs_test.go @@ -25,6 +25,21 @@ import ( "gorm.io/gorm/logger" ) +// waitForLogSubscriber blocks until the worker's WebSocket log handler has +// registered its subscription on the store. +// +// The handler writes the "initial" batch first and subscribes only afterwards, +// so a line appended the instant that batch lands is buffered but never +// streamed, and the spec then waits out its full read deadline. Measured at +// roughly one run in seventeen with `--repeat`, which is far too often for CI. +// Waiting on the subscription removes the race from the spec; the handler's own +// snapshot/subscribe window is a separate production question. +func waitForLogSubscriber(logStore *model.BackendLogStore, modelID string) { + GinkgoHelper() + Eventually(func() int { return logStore.SubscriberCount(modelID) }, "10s", "5ms"). + Should(BeNumerically(">", 0), "the WebSocket handler never subscribed to %q", modelID) +} + var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func() { Context("Worker HTTP log endpoints", func() { @@ -212,6 +227,7 @@ var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func Expect(initialLines[1].Text).To(Equal("line-2")) // Now append a new line and verify it arrives via WebSocket + waitForLogSubscriber(logStore, "ws-model") logStore.AppendLine("ws-model", "stdout", "line-3-realtime") conn.SetReadDeadline(time.Now().Add(5 * time.Second)) @@ -280,6 +296,7 @@ var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func Expect(conn.ReadJSON(&initialMsg)).To(Succeed()) // Append line to a different model + waitForLogSubscriber(logStore, "ws-model") logStore.AppendLine("other-model", "stdout", "should not appear") // Append line to our model logStore.AppendLine("ws-model", "stdout", "should appear") @@ -475,6 +492,7 @@ var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func Expect(initialLines[0].Text).To(Equal("initial line from worker")) // Append a new line on the worker's log store + waitForLogSubscriber(logStore, "proxy-model") logStore.AppendLine("proxy-model", "stderr", "realtime via proxy") // Read the streamed line through the proxy From 53639c4df399506743b565d2fd0e17253fe8dca5 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 10:42:49 +0000 Subject: [PATCH 04/23] test(distributed): scope the log-subscriber wait and mark the race it works around MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three corrections from review of the previous commit. The lock-order comment on SubscriberCount claimed no path takes s.mu and a buffer lock together. Subscribe does exactly that, holding s.mu.RLock across replica registrations that take buf.mu. State the rule that is actually true — s.mu precedes any buffer lock, so counting after releasing it preserves the order — and say what follows from it: the total is a sample, not a snapshot. waitForLogSubscriber read as general-purpose but unblocks on the first registered subscription. Subscribe attaches the exact-key buffer and each replica buffer one at a time, so for a replicated model the count goes positive while later replicas are still unattached and the race survives. Rename it waitForSingleLogSubscriber, document that it holds only where Subscribe resolves to one buffer, and assert on exactly 1: misuse then fails loudly on the count rather than going quietly back to being flaky. Taking the expected count as a parameter was the alternative, but that makes callers predict a store-internal number and an under-count fails the same silent way as the original bug. The snapshot-then-subscribe race had no artifact outside a report, and review found a second site carrying it. Mark both handlers identically, including the point that swapping the two calls duplicates rather than drops and so is not the fix. The race itself is left alone; this branch stays test infrastructure. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- core/http/endpoints/localai/backend_logs.go | 7 ++++++ core/services/nodes/file_transfer_server.go | 7 ++++++ pkg/model/backend_log_store.go | 7 ++++-- tests/e2e/distributed/backend_logs_test.go | 24 +++++++++++++++------ 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/core/http/endpoints/localai/backend_logs.go b/core/http/endpoints/localai/backend_logs.go index 6072b8483708..8b5f99d66f78 100644 --- a/core/http/endpoints/localai/backend_logs.go +++ b/core/http/endpoints/localai/backend_logs.go @@ -121,6 +121,13 @@ func BackendLogsWebSocketEndpoint(ml *model.ModelLoader) echo.HandlerFunc { conn := &backendLogsConn{Conn: ws} + // KNOWN RACE: the snapshot is sent before the subscription is registered, so + // a line appended in that window is never streamed. A viewer attaching while + // a model loads (when a backend is at its noisiest) can silently miss lines; + // they stay in the buffer, so a reload shows them. Fixing it needs an atomic + // snapshot-plus-subscribe under the store lock, not a reorder of these two + // calls, which would duplicate instead of drop. + // Send existing lines as initial batch existingLines := ml.BackendLogs().GetLines(modelID) initialMsg := map[string]any{ diff --git a/core/services/nodes/file_transfer_server.go b/core/services/nodes/file_transfer_server.go index 0fc5ac6343f4..2d4bc03baaeb 100644 --- a/core/services/nodes/file_transfer_server.go +++ b/core/services/nodes/file_transfer_server.go @@ -839,6 +839,13 @@ func handleBackendLogsWS(w http.ResponseWriter, r *http.Request, logStore *model conn := &backendLogsWSConn{Conn: ws} + // KNOWN RACE: the snapshot is sent before the subscription is registered, so + // a line appended in that window is never streamed. A viewer attaching while + // a model loads (when a backend is at its noisiest) can silently miss lines; + // they stay in the buffer, so a reload shows them. Fixing it needs an atomic + // snapshot-plus-subscribe under the store lock, not a reorder of these two + // calls, which would duplicate instead of drop. + // Send existing lines as initial batch existingLines := logStore.GetLines(modelID) initialMsg := map[string]any{ diff --git a/pkg/model/backend_log_store.go b/pkg/model/backend_log_store.go index 30268fde47de..3c60f34a3736 100644 --- a/pkg/model/backend_log_store.go +++ b/pkg/model/backend_log_store.go @@ -366,8 +366,11 @@ func (s *BackendLogStore) SubscriberCount(modelID string) int { } s.mu.RUnlock() - // Counted after releasing s.mu: no other path takes s.mu and a buffer lock - // together, and keeping it that way costs nothing here. + // Lock order in this type is always s.mu before any buffer lock — Subscribe + // holds s.mu.RLock across its replica registrations, which take buf.mu — so + // counting after releasing s.mu keeps that order rather than inverting it. + // The total is therefore a sample, not a snapshot: a concurrent Subscribe + // can register a further buffer while this loop runs. count := func(buf *backendLogBuffer) int { buf.mu.Lock() defer buf.mu.Unlock() diff --git a/tests/e2e/distributed/backend_logs_test.go b/tests/e2e/distributed/backend_logs_test.go index 473dad54f111..82e8ac156401 100644 --- a/tests/e2e/distributed/backend_logs_test.go +++ b/tests/e2e/distributed/backend_logs_test.go @@ -25,7 +25,7 @@ import ( "gorm.io/gorm/logger" ) -// waitForLogSubscriber blocks until the worker's WebSocket log handler has +// waitForSingleLogSubscriber blocks until the worker's WebSocket log handler has // registered its subscription on the store. // // The handler writes the "initial" batch first and subscribes only afterwards, @@ -33,11 +33,21 @@ import ( // streamed, and the spec then waits out its full read deadline. Measured at // roughly one run in seventeen with `--repeat`, which is far too often for CI. // Waiting on the subscription removes the race from the spec; the handler's own -// snapshot/subscribe window is a separate production question. -func waitForLogSubscriber(logStore *model.BackendLogStore, modelID string) { +// snapshot/subscribe window is a separate production question, marked at both +// production sites. +// +// Only valid where BackendLogStore.Subscribe resolves modelID to exactly ONE +// buffer: a bare model ID with no "#N" replica buffers in the store, or +// a full process key. Subscribe registers the exact-key buffer and each replica +// buffer one at a time, so for a model that does have replicas the count goes +// positive while later replicas are still unattached and the race survives. +// Hence the assertion is on exactly 1 rather than "at least 1": a spec that +// misapplies this to a replicated model fails loudly on the count instead of +// going quietly back to being flaky. +func waitForSingleLogSubscriber(logStore *model.BackendLogStore, modelID string) { GinkgoHelper() Eventually(func() int { return logStore.SubscriberCount(modelID) }, "10s", "5ms"). - Should(BeNumerically(">", 0), "the WebSocket handler never subscribed to %q", modelID) + Should(Equal(1), "the WebSocket handler never subscribed to %q exactly once", modelID) } var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func() { @@ -227,7 +237,7 @@ var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func Expect(initialLines[1].Text).To(Equal("line-2")) // Now append a new line and verify it arrives via WebSocket - waitForLogSubscriber(logStore, "ws-model") + waitForSingleLogSubscriber(logStore, "ws-model") logStore.AppendLine("ws-model", "stdout", "line-3-realtime") conn.SetReadDeadline(time.Now().Add(5 * time.Second)) @@ -296,7 +306,7 @@ var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func Expect(conn.ReadJSON(&initialMsg)).To(Succeed()) // Append line to a different model - waitForLogSubscriber(logStore, "ws-model") + waitForSingleLogSubscriber(logStore, "ws-model") logStore.AppendLine("other-model", "stdout", "should not appear") // Append line to our model logStore.AppendLine("ws-model", "stdout", "should appear") @@ -492,7 +502,7 @@ var _ = Describe("Distributed Backend Log Streaming", Label("Distributed"), func Expect(initialLines[0].Text).To(Equal("initial line from worker")) // Append a new line on the worker's log store - waitForLogSubscriber(logStore, "proxy-model") + waitForSingleLogSubscriber(logStore, "proxy-model") logStore.AppendLine("proxy-model", "stderr", "realtime via proxy") // Read the streamed line through the proxy From 3257fc5cd866bdd9c391b16c4bedb4361326ff80 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 10:49:54 +0000 Subject: [PATCH 05/23] ci(distributed): run the distributed e2e suite on PRs The suite has never run in CI, so 239 specs across 32 files were verified only by hand. Path-filtered to distributed code, advisory until it earns a track record, and with flake retries at 1 rather than 5 so nondeterminism surfaces instead of being retried away. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .github/workflows/tests-e2e-distributed.yml | 73 +++++++++++++++++++++ Makefile | 8 ++- 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/tests-e2e-distributed.yml diff --git a/.github/workflows/tests-e2e-distributed.yml b/.github/workflows/tests-e2e-distributed.yml new file mode 100644 index 000000000000..4afc76dc05e1 --- /dev/null +++ b/.github/workflows/tests-e2e-distributed.yml @@ -0,0 +1,73 @@ +--- +name: 'E2E Distributed Tests' + +on: + pull_request: + paths: + - 'core/services/nodes/**' + - 'core/services/worker/**' + - 'core/services/messaging/**' + - 'core/services/syncstate/**' + - 'core/services/jobs/**' + - 'core/services/agents/**' + - 'core/services/agentpool/**' + - 'core/services/galleryop/**' + - 'core/http/routes/nodes.go' + - 'core/http/endpoints/localai/nodes.go' + - 'core/http/endpoints/openresponses/**' + - 'core/application/distributed.go' + - 'core/config/distributed_config.go' + - 'pkg/natsauth/**' + - 'tests/e2e/distributed/**' + - '.github/workflows/tests-e2e-distributed.yml' + - 'Makefile' + push: + branches: + - master + +concurrency: + group: ci-tests-e2e-distributed-${{ github.event.pull_request.number || github.sha }}-${{ github.repository }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + tests-e2e-distributed: + runs-on: ubuntu-latest + # Advisory while the suite builds a track record. Flip to a required check + # only after it has run clean for two weeks; a heavy suite made required on + # day one gets disabled instead of fixed. + continue-on-error: true + timeout-minutes: 45 + steps: + - name: Clone + uses: actions/checkout@v7 + with: + submodules: true + - name: Configure apt mirror on runner + uses: ./.github/actions/configure-apt-mirror + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version: '1.25.x' + cache: false + - name: Dependencies + run: | + sudo apt-get update + sudo apt-get install -y build-essential libopus-dev + - name: Proto Dependencies + run: | + curl -L -s https://github.com/protocolbuffers/protobuf/releases/download/v26.1/protoc-26.1-linux-x86_64.zip -o protoc.zip && \ + unzip -j -d /usr/local/bin protoc.zip bin/protoc && \ + rm protoc.zip + go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.34.2 + go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@1958fcbe2ca8bd93af633f11e97d44e567e945af + PATH="$PATH:$HOME/go/bin" make protogen-go + - name: Pre-pull test images + # Pulling here rather than inside the suite keeps container-start timing + # out of the spec timeouts and makes a registry outage read as a + # setup failure instead of a test failure. + run: | + docker pull postgres:16-alpine + docker pull nats:2-alpine + - name: Distributed E2E + run: | + PATH="$PATH:$HOME/go/bin" make test-e2e-distributed diff --git a/Makefile b/Makefile index ebedb2c98248..3df884de87bf 100644 --- a/Makefile +++ b/Makefile @@ -340,12 +340,18 @@ run-e2e-aio: protogen-go @echo 'Running e2e AIO tests' $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --flake-attempts $(TEST_FLAKES) -v -r ./tests/e2e-aio +# Flake retries for the distributed suite. Defaults to 1, unlike TEST_FLAKES: +# this suite exists to catch nondeterministic cluster behaviour, and retrying +# hides exactly the failures it is meant to surface. Raise it locally if you are +# bisecting something unrelated. +DISTRIBUTED_TEST_FLAKES?=1 + # Distributed architecture e2e (PostgreSQL + NATS via testcontainers). # Includes NatsJWT specs (JWT-enabled NATS). Requires Docker. # VLLMMultinode is excluded here; use test-e2e-vllm-multinode for that. test-e2e-distributed: protogen-go @echo 'Running distributed e2e tests (label Distributed, incl. NatsJWT)' - $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Distributed && !VLLMMultinode' --flake-attempts $(TEST_FLAKES) -v -r ./tests/e2e/distributed + $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Distributed && !VLLMMultinode && !Cluster' --flake-attempts $(DISTRIBUTED_TEST_FLAKES) --timeout=40m -v -r ./tests/e2e/distributed # vLLM multi-node DP smoke (CPU). Builds local-ai:tests and the # cpu-vllm backend from the current working tree, then drives a From 2d37ee10e66ecaf88191afd52035b0f4855c856d Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 11:02:13 +0000 Subject: [PATCH 06/23] ci(distributed): widen the trigger and drop the mid-suite image pull The path allowlist covered 13 of the 99 packages the suite reaches. Commit 1dc3aeef8 touched core/config, core/services/modeladmin and core/backend and matched no entry, so it would have merged without running the very specs that cover it. Use the paths-ignore denylist tests-e2e.yml already uses. Disable the testcontainers reaper: the runner is ephemeral, so the reaper buys nothing and its unpinned image was pulled mid-suite, defeating the pre-pull. Drop continue-on-error, which no other workflow uses and which reports a failed run as green. The job is advisory by staying out of branch protection instead. Pin Go to 1.26.0 to match go.mod, and add the tmate-on-failure step. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .github/workflows/tests-e2e-distributed.yml | 56 ++++++++++++--------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/.github/workflows/tests-e2e-distributed.yml b/.github/workflows/tests-e2e-distributed.yml index 4afc76dc05e1..4b41f72b5014 100644 --- a/.github/workflows/tests-e2e-distributed.yml +++ b/.github/workflows/tests-e2e-distributed.yml @@ -3,24 +3,15 @@ name: 'E2E Distributed Tests' on: pull_request: - paths: - - 'core/services/nodes/**' - - 'core/services/worker/**' - - 'core/services/messaging/**' - - 'core/services/syncstate/**' - - 'core/services/jobs/**' - - 'core/services/agents/**' - - 'core/services/agentpool/**' - - 'core/services/galleryop/**' - - 'core/http/routes/nodes.go' - - 'core/http/endpoints/localai/nodes.go' - - 'core/http/endpoints/openresponses/**' - - 'core/application/distributed.go' - - 'core/config/distributed_config.go' - - 'pkg/natsauth/**' - - 'tests/e2e/distributed/**' - - '.github/workflows/tests-e2e-distributed.yml' - - 'Makefile' + # The suite's dependency graph is 99 packages, so an allowlist of paths + # silently stops guarding the moment code moves. At ~75s the job is cheap + # enough to run unless the diff is confined to paths it provably cannot + # reach. See .agents/ci-caching.md. + paths-ignore: + - 'gallery/**' + - 'docs/**' + - 'examples/**' + - '**/*.md' push: branches: - master @@ -32,10 +23,11 @@ concurrency: jobs: tests-e2e-distributed: runs-on: ubuntu-latest - # Advisory while the suite builds a track record. Flip to a required check - # only after it has run clean for two weeks; a heavy suite made required on - # day one gets disabled instead of fixed. - continue-on-error: true + # Advisory because it is deliberately not in branch protection, so a failure + # is a visible red X rather than a blocked merge. Promoting it to a required + # check is a repository-settings change, to be made once it has a track + # record; a heavy suite made required on day one gets disabled instead of + # fixed. timeout-minutes: 45 steps: - name: Clone @@ -47,7 +39,7 @@ jobs: - name: Setup Go uses: actions/setup-go@v5 with: - go-version: '1.25.x' + go-version: '1.26.0' cache: false - name: Dependencies run: | @@ -64,10 +56,26 @@ jobs: - name: Pre-pull test images # Pulling here rather than inside the suite keeps container-start timing # out of the spec timeouts and makes a registry outage read as a - # setup failure instead of a test failure. + # setup failure instead of a test failure. These two are the only images + # the suite needs once the testcontainers reaper is disabled below. run: | docker pull postgres:16-alpine docker pull nats:2-alpine - name: Distributed E2E + # TESTCONTAINERS_RYUK_DISABLED keeps the pre-pull above meaningful. The + # reaper exists to clean up leaked containers on a long-lived host, but + # this runner is ephemeral and every container dies with the VM. Leaving + # it enabled would pull a third, unpinned image (testcontainers/ryuk) + # from Docker Hub mid-suite: exactly the registry dependency the + # pre-pull step exists to remove. + env: + TESTCONTAINERS_RYUK_DISABLED: "true" run: | PATH="$PATH:$HOME/go/bin" make test-e2e-distributed + - name: Setup tmate session if tests fail + if: ${{ failure() }} + uses: mxschmitt/action-tmate@v3.23 + with: + detached: true + connect-timeout-seconds: 180 + limit-access-to-actor: true From c0af66a7ebe5793f8e3837ff9ddbff0a1344ccbb Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 11:10:04 +0000 Subject: [PATCH 07/23] test(distributed): add a process-level cluster harness Runs local-ai as real child processes, one per frontend replica and one per worker, against containerised infrastructure. The in-process suites cannot express frontend-replica failure: there is no process to kill and no real HTTP boundary between a worker and the frontend it registered with. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/cluster.go | 329 ++++++++++++++++++ .../distributed/cluster/cluster_suite_test.go | 13 + tests/e2e/distributed/cluster/cluster_test.go | 37 ++ 3 files changed, 379 insertions(+) create mode 100644 tests/e2e/distributed/cluster/cluster.go create mode 100644 tests/e2e/distributed/cluster/cluster_suite_test.go create mode 100644 tests/e2e/distributed/cluster/cluster_test.go diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go new file mode 100644 index 000000000000..306980e00d2b --- /dev/null +++ b/tests/e2e/distributed/cluster/cluster.go @@ -0,0 +1,329 @@ +// Package cluster runs LocalAI as real child processes for end-to-end tests. +// +// The in-process suites cannot express frontend-replica failure: there is no +// process to kill, no second replica to race, and no real HTTP boundary between +// a worker and the frontend it registered with. This package starts the same +// binary an operator runs, one process per frontend replica and one per worker, +// against containerised Postgres and NATS. +package cluster + +import ( + "fmt" + "net/http" + "os" + "os/exec" + "path/filepath" + "time" + + "github.com/mudler/LocalAI/pkg/httpclient" + + "github.com/phayes/freeport" +) + +// Options configures a cluster. Every field without a default is required. +type Options struct { + // Binary is the path to a built local-ai. + Binary string + // MockBackend is the path to the mock-backend binary. When set it is copied + // into each worker's backends directory as "mock-backend", which is the name + // model YAML refers to (see tests/e2e/e2e_suite_test.go:75). + MockBackend string + // PGDSN and NatsURL point at infrastructure the caller already started. + PGDSN string + NatsURL string + // LogDir receives one file per process. Never empty: a cluster failure is + // unreadable without them. + LogDir string + + RegistrationToken string // default "e2e-token" + AdminEmail string // default "admin@e2e.local" + + Frontends int + Workers int +} + +// Process is one running local-ai. +type Process struct { + Name string + Cmd *exec.Cmd + Port int + LogPath string + + logFile *os.File + // exited closes once the reaper has collected the process. Only the reaper + // calls Cmd.Wait, so nothing else may: a second Wait on the same Cmd races + // the first and corrupts ProcessState. + exited chan struct{} + waitErr error +} + +// Cluster is a running set of frontend and worker processes. +type Cluster struct { + opts Options + frontends []*Process + workers []*Process + baseDir string +} + +const ( + defaultRegistrationToken = "e2e-token" + defaultAdminEmail = "admin@e2e.local" + readinessTimeout = 90 * time.Second + readinessPoll = 200 * time.Millisecond +) + +func (o *Options) applyDefaults() { + if o.RegistrationToken == "" { + o.RegistrationToken = defaultRegistrationToken + } + if o.AdminEmail == "" { + o.AdminEmail = defaultAdminEmail + } +} + +func (o Options) validate() error { + if o.Frontends < 1 { + return fmt.Errorf("cluster needs at least one frontend, got %d", o.Frontends) + } + if o.LogDir == "" { + return fmt.Errorf("cluster needs a LogDir: process logs are the only way to read a cluster failure") + } + if st, err := os.Stat(o.Binary); err != nil || st.IsDir() { + return fmt.Errorf("local-ai binary not found at %q (run: make build)", o.Binary) + } + return nil +} + +// Start brings up the cluster and blocks until every frontend answers /readyz +// and every worker has registered. +func Start(opts Options) (*Cluster, error) { + opts.applyDefaults() + if err := opts.validate(); err != nil { + return nil, err + } + + baseDir, err := os.MkdirTemp("", "localai-cluster-*") + if err != nil { + return nil, fmt.Errorf("creating cluster work dir: %w", err) + } + + c := &Cluster{opts: opts, baseDir: baseDir} + + for i := 0; i < opts.Frontends; i++ { + p, err := c.startFrontend(i) + if err != nil { + c.Stop() + return nil, err + } + c.frontends = append(c.frontends, p) + } + for i := 0; i < opts.Workers; i++ { + p, err := c.startWorker(i) + if err != nil { + c.Stop() + return nil, err + } + c.workers = append(c.workers, p) + } + return c, nil +} + +func (c *Cluster) startFrontend(i int) (*Process, error) { + port, err := freeport.GetFreePort() + if err != nil { + return nil, fmt.Errorf("allocating frontend port: %w", err) + } + name := fmt.Sprintf("frontend-%d", i) + dir := filepath.Join(c.baseDir, name) + if err := os.MkdirAll(filepath.Join(dir, "models"), 0o755); err != nil { + return nil, fmt.Errorf("creating %s dirs: %w", name, err) + } + if err := os.MkdirAll(filepath.Join(dir, "backends"), 0o755); err != nil { + return nil, fmt.Errorf("creating %s dirs: %w", name, err) + } + + cmd := exec.Command(c.opts.Binary, "run", + "--address", fmt.Sprintf("127.0.0.1:%d", port), + "--models-path", filepath.Join(dir, "models"), + "--backends-path", filepath.Join(dir, "backends"), + ) + // Cmd.Environ() is the parent environment this Cmd would already run with; + // the children need PATH, HOME and the Go/CI environment intact. + cmd.Env = append(cmd.Environ(), + "LOCALAI_DISTRIBUTED=true", + "LOCALAI_NATS_URL="+c.opts.NatsURL, + "LOCALAI_AUTH=true", + "LOCALAI_AUTH_DATABASE_URL="+c.opts.PGDSN, + "LOCALAI_ADMIN_EMAIL="+c.opts.AdminEmail, + "LOCALAI_REGISTRATION_TOKEN="+c.opts.RegistrationToken, + "LOCALAI_AUTO_APPROVE_NODES=true", + "DEBUG=true", + ) + + p, err := c.spawn(name, cmd, port) + if err != nil { + return nil, err + } + if err := waitReady(p, fmt.Sprintf("http://127.0.0.1:%d/readyz", port)); err != nil { + // The caller never sees this process, so Stop() will never reach it: + // reap it here or it outlives the suite holding a port and a log handle. + p.terminate() + return nil, fmt.Errorf("%s never became ready (see %s): %w", name, p.LogPath, err) + } + return p, nil +} + +func (c *Cluster) startWorker(i int) (*Process, error) { + // Two independent free ports: the worker's file-transfer server defaults to + // basePort-1, which freeport never reserved and which is basePort of another + // worker whenever two allocations land adjacent. + ports, err := freeport.GetFreePorts(2) + if err != nil { + return nil, fmt.Errorf("allocating worker ports: %w", err) + } + grpcPort, httpPort := ports[0], ports[1] + name := fmt.Sprintf("worker-%d", i) + dir := filepath.Join(c.baseDir, name) + backends := filepath.Join(dir, "backends") + if err := os.MkdirAll(filepath.Join(dir, "models"), 0o755); err != nil { + return nil, fmt.Errorf("creating %s dirs: %w", name, err) + } + if err := os.MkdirAll(backends, 0o755); err != nil { + return nil, fmt.Errorf("creating %s dirs: %w", name, err) + } + if c.opts.MockBackend != "" { + if err := copyExecutable(c.opts.MockBackend, filepath.Join(backends, "mock-backend")); err != nil { + return nil, fmt.Errorf("installing mock backend for %s: %w", name, err) + } + } + + cmd := exec.Command(c.opts.Binary, "worker", + "--models-path", filepath.Join(dir, "models"), + "--backends-path", backends, + ) + cmd.Env = append(cmd.Environ(), + fmt.Sprintf("LOCALAI_SERVE_ADDR=127.0.0.1:%d", grpcPort), + fmt.Sprintf("LOCALAI_ADVERTISE_ADDR=127.0.0.1:%d", grpcPort), + fmt.Sprintf("LOCALAI_HTTP_ADDR=127.0.0.1:%d", httpPort), + fmt.Sprintf("LOCALAI_ADVERTISE_HTTP_ADDR=127.0.0.1:%d", httpPort), + "LOCALAI_REGISTER_TO="+c.FrontendURL(0), + "LOCALAI_NODE_NAME="+name, + "LOCALAI_REGISTRATION_TOKEN="+c.opts.RegistrationToken, + "LOCALAI_NATS_URL="+c.opts.NatsURL, + "DEBUG=true", + ) + + return c.spawn(name, cmd, grpcPort) +} + +func (c *Cluster) spawn(name string, cmd *exec.Cmd, port int) (*Process, error) { + logPath := filepath.Join(c.opts.LogDir, name+".log") + f, err := os.Create(logPath) + if err != nil { + return nil, fmt.Errorf("creating log file for %s: %w", name, err) + } + cmd.Stdout = f + cmd.Stderr = f + if err := cmd.Start(); err != nil { + _ = f.Close() + return nil, fmt.Errorf("starting %s: %w", name, err) + } + p := &Process{Name: name, Cmd: cmd, Port: port, LogPath: logPath, logFile: f, exited: make(chan struct{})} + // One reaper per process, joined by terminate(): a child that dies on its own + // is collected immediately, so waiters learn about it instead of polling a + // dead port until the readiness timeout. + go func() { + p.waitErr = cmd.Wait() + close(p.exited) + }() + return p, nil +} + +// terminate kills the process, waits for the reaper, and releases the log +// handle. Safe to call more than once and on a process that already exited. +func (p *Process) terminate() { + if p == nil || p.Cmd == nil || p.Cmd.Process == nil { + return + } + _ = p.Cmd.Process.Kill() + <-p.exited + if p.logFile != nil { + _ = p.logFile.Close() + } +} + +// FrontendURL is the base URL of frontend i. +func (c *Cluster) FrontendURL(i int) string { + return fmt.Sprintf("http://127.0.0.1:%d", c.frontends[i].Port) +} + +// WorkerName is the node name worker i registered under. +func (c *Cluster) WorkerName(i int) string { + return c.workers[i].Name +} + +// Stop terminates every process and removes the work directory. Logs survive in +// LogDir, which the caller owns. +func (c *Cluster) Stop() { + for _, p := range append(append([]*Process{}, c.workers...), c.frontends...) { + p.terminate() + } + if c.baseDir != "" { + _ = os.RemoveAll(c.baseDir) + } +} + +// DumpLogs writes every process log to stdout. Call from an AfterEach guarded by +// CurrentSpecReport().Failed(). +func (c *Cluster) DumpLogs() { + for _, p := range append(append([]*Process{}, c.frontends...), c.workers...) { + if p == nil { + continue + } + data, err := os.ReadFile(p.LogPath) + if err != nil { + fmt.Printf("=== %s: log unreadable: %v\n", p.Name, err) + continue + } + fmt.Printf("=== %s (%s) ===\n%s\n", p.Name, p.LogPath, string(data)) + } +} + +func waitReady(p *Process, url string) error { + deadline := time.Now().Add(readinessTimeout) + client := httpclient.NewWithTimeout(2 * time.Second) + var last error + for time.Now().Before(deadline) { + select { + case <-p.exited: + if p.waitErr == nil { + return fmt.Errorf("process exited cleanly before becoming ready") + } + return fmt.Errorf("process exited before becoming ready: %w", p.waitErr) + default: + } + resp, err := client.Get(url) + if err == nil { + _ = resp.Body.Close() + if resp.StatusCode == http.StatusOK { + return nil + } + last = fmt.Errorf("status %d", resp.StatusCode) + } else { + last = err + } + time.Sleep(readinessPoll) + } + return fmt.Errorf("not ready within %s: %w", readinessTimeout, last) +} + +func copyExecutable(src, dst string) error { + data, err := os.ReadFile(src) + if err != nil { + return fmt.Errorf("reading %s: %w", src, err) + } + if err := os.WriteFile(dst, data, 0o755); err != nil { + return fmt.Errorf("writing %s: %w", dst, err) + } + return nil +} diff --git a/tests/e2e/distributed/cluster/cluster_suite_test.go b/tests/e2e/distributed/cluster/cluster_suite_test.go new file mode 100644 index 000000000000..69f785ba4cad --- /dev/null +++ b/tests/e2e/distributed/cluster/cluster_suite_test.go @@ -0,0 +1,13 @@ +package cluster_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestCluster(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Cluster Harness Suite") +} diff --git a/tests/e2e/distributed/cluster/cluster_test.go b/tests/e2e/distributed/cluster/cluster_test.go new file mode 100644 index 000000000000..4d48f1ecb246 --- /dev/null +++ b/tests/e2e/distributed/cluster/cluster_test.go @@ -0,0 +1,37 @@ +package cluster_test + +import ( + "os" + "path/filepath" + + "github.com/mudler/LocalAI/tests/e2e/distributed/cluster" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Cluster options", Label("Distributed"), func() { + It("rejects a binary path that does not exist", func() { + _, err := cluster.Start(cluster.Options{ + Binary: filepath.Join(os.TempDir(), "definitely-not-local-ai"), + PGDSN: "postgres://test:test@127.0.0.1:5432/x?sslmode=disable", + NatsURL: "nats://127.0.0.1:4222", + LogDir: GinkgoT().TempDir(), + Frontends: 1, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("local-ai binary")) + }) + + It("rejects a cluster with no frontends", func() { + _, err := cluster.Start(cluster.Options{ + Binary: "/bin/true", + PGDSN: "postgres://test:test@127.0.0.1:5432/x?sslmode=disable", + NatsURL: "nats://127.0.0.1:4222", + LogDir: GinkgoT().TempDir(), + Frontends: 0, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("at least one frontend")) + }) +}) From a7847b8a37198560f940c863eb274c9851ca30dd Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 11:18:49 +0000 Subject: [PATCH 08/23] test(distributed): make the cluster harness survive a restart Restarting a frontend replica must not move it: workers read LOCALAI_REGISTER_TO once at boot and never re-resolve it, so a replica that returns on a fresh port is unreachable by the workers that registered with it. startFrontend now takes the port, with <= 0 meaning "allocate". Process logs are opened for append rather than truncated, so a restarted process cannot erase the log of the instance that died, which is the log a failover post-mortem needs. The post-SIGKILL wait is bounded, so one stuck child no longer becomes a suite-wide timeout that names nothing. Stop is nil-safe because Start returns a nil cluster after stopping itself. Start's doc comment no longer claims to wait for worker registration; that needs an authenticated admin session, so it now says callers must poll /api/nodes themselves. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/cluster.go | 47 +++++++++++++++++++----- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go index 306980e00d2b..0445fe3868e9 100644 --- a/tests/e2e/distributed/cluster/cluster.go +++ b/tests/e2e/distributed/cluster/cluster.go @@ -44,7 +44,10 @@ type Options struct { // Process is one running local-ai. type Process struct { - Name string + Name string + // Cmd is exposed for signalling only. Never call Cmd.Wait on it: the reaper + // goroutine started in spawn owns it, a second Wait races the first, and + // waitErr is only safe to read after <-p.exited. Cmd *exec.Cmd Port int LogPath string @@ -70,6 +73,10 @@ const ( defaultAdminEmail = "admin@e2e.local" readinessTimeout = 90 * time.Second readinessPoll = 200 * time.Millisecond + // processExitTimeout bounds the post-SIGKILL wait in terminate. An unbounded + // wait turns one stuck child (D state, or a Wait that never returns) into a + // suite-wide Ginkgo timeout that names nothing. + processExitTimeout = 10 * time.Second ) func (o *Options) applyDefaults() { @@ -94,8 +101,10 @@ func (o Options) validate() error { return nil } -// Start brings up the cluster and blocks until every frontend answers /readyz -// and every worker has registered. +// Start brings up the cluster. It blocks until every frontend answers /readyz +// and every worker process has been spawned. It does NOT wait for workers to +// register: that needs an authenticated admin session, so a caller that depends +// on registration must poll /api/nodes itself. func Start(opts Options) (*Cluster, error) { opts.applyDefaults() if err := opts.validate(); err != nil { @@ -110,7 +119,7 @@ func Start(opts Options) (*Cluster, error) { c := &Cluster{opts: opts, baseDir: baseDir} for i := 0; i < opts.Frontends; i++ { - p, err := c.startFrontend(i) + p, err := c.startFrontend(i, 0) if err != nil { c.Stop() return nil, err @@ -128,10 +137,17 @@ func Start(opts Options) (*Cluster, error) { return c, nil } -func (c *Cluster) startFrontend(i int) (*Process, error) { - port, err := freeport.GetFreePort() - if err != nil { - return nil, fmt.Errorf("allocating frontend port: %w", err) +// startFrontend starts frontend i. A port <= 0 allocates a fresh one; a pinned +// port exists for restart: workers take LOCALAI_REGISTER_TO once at boot and +// never re-resolve it, so a replica that comes back on a new port is +// unreachable by exactly the workers that registered with it. +func (c *Cluster) startFrontend(i int, port int) (*Process, error) { + if port <= 0 { + allocated, err := freeport.GetFreePort() + if err != nil { + return nil, fmt.Errorf("allocating frontend port: %w", err) + } + port = allocated } name := fmt.Sprintf("frontend-%d", i) dir := filepath.Join(c.baseDir, name) @@ -218,7 +234,9 @@ func (c *Cluster) startWorker(i int) (*Process, error) { func (c *Cluster) spawn(name string, cmd *exec.Cmd, port int) (*Process, error) { logPath := filepath.Join(c.opts.LogDir, name+".log") - f, err := os.Create(logPath) + // Append rather than truncate: a restarted process reopens the same path, and + // the log of the instance that died is the one a failover post-mortem needs. + f, err := os.OpenFile(logPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if err != nil { return nil, fmt.Errorf("creating log file for %s: %w", name, err) } @@ -246,7 +264,11 @@ func (p *Process) terminate() { return } _ = p.Cmd.Process.Kill() - <-p.exited + select { + case <-p.exited: + case <-time.After(processExitTimeout): + fmt.Printf("warning: %s did not exit within %s after SIGKILL; continuing teardown\n", p.Name, processExitTimeout) + } if p.logFile != nil { _ = p.logFile.Close() } @@ -265,6 +287,11 @@ func (c *Cluster) WorkerName(i int) string { // Stop terminates every process and removes the work directory. Logs survive in // LogDir, which the caller owns. func (c *Cluster) Stop() { + // Start returns (nil, err) after stopping itself, so a spec that defers + // c.Stop before asserting the error would otherwise nil-deref. + if c == nil { + return + } for _, p := range append(append([]*Process{}, c.workers...), c.frontends...) { p.terminate() } From 53cd640a89eb68d71195e9a563255efaa89b4c9e Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 11:25:47 +0000 Subject: [PATCH 09/23] test(distributed): add admin session helper to the cluster harness The register handler answers 201 both for "user created, here is your session" and for "this email already exists", so the status code cannot tell a fresh registration from a repeat one. Key on the session cookie instead and fall through to login when it is absent. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/admin.go | 168 ++++++++++++++++++ tests/e2e/distributed/cluster/cluster_test.go | 22 +++ 2 files changed, 190 insertions(+) create mode 100644 tests/e2e/distributed/cluster/admin.go diff --git a/tests/e2e/distributed/cluster/admin.go b/tests/e2e/distributed/cluster/admin.go new file mode 100644 index 000000000000..b1371f532271 --- /dev/null +++ b/tests/e2e/distributed/cluster/admin.go @@ -0,0 +1,168 @@ +package cluster + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/cookiejar" + "net/url" + "time" + + "github.com/mudler/LocalAI/pkg/httpclient" +) + +const ( + // adminPassword must satisfy core/http/auth's policy (>= 12 chars and a + // zxcvbn score of 3 against hints that include "admin" and "localai"). + // This one scores 3 today; acknowledgeWeakPassword is sent alongside it so + // a future tightening of the policy cannot silently break every failover + // spec at setup time. + adminPassword = "e2e-admin-password" + // sessionCookieName mirrors the unexported constant in core/http/auth. + // The register handler returns 201 both for "user created, here is your + // session" and for "this email already exists" (a deliberate account + // enumeration defence), so the status code alone cannot tell the two + // apart: the presence of this cookie is the only reliable signal. + sessionCookieName = "session" + // authRequestTimeout bounds one register/login round trip. + authRequestTimeout = 30 * time.Second + // bodyExcerptLimit caps how much of an error response is quoted back. + bodyExcerptLimit = 512 +) + +// ForTestingEmpty returns a Cluster with no processes. It exists so the package's +// own argument-validation specs do not need to start anything. +func ForTestingEmpty() *Cluster { + return &Cluster{} +} + +// AdminSession registers the admin user on frontend i and returns a client +// carrying the resulting session cookie. The email matches LOCALAI_ADMIN_EMAIL, +// which core/http/auth exempts from the approval gate and assigns the admin +// role, so registration alone yields an active admin session. +// +// Call this ONCE per cluster and share the client. Two reasons: the auth +// endpoints are rate limited to 5 requests per minute per client IP, and every +// e2e request arrives from 127.0.0.1; and the returned client is already good +// for every frontend, because sessions live in the shared Postgres auth DB and +// Go's cookie jar keys cookies by host without the port. +func (c *Cluster) AdminSession(i int) (*http.Client, error) { + base, err := c.frontendBaseURL(i) + if err != nil { + return nil, err + } + + jar, err := cookiejar.New(nil) + if err != nil { + return nil, fmt.Errorf("creating cookie jar: %w", err) + } + // httpclient hardens the transport and refuses redirects; the jar is the one + // thing it does not configure, and a session cookie is the whole point here. + client := httpclient.NewWithTimeout(authRequestTimeout) + client.Jar = jar + + credentials := map[string]any{ + "email": c.opts.AdminEmail, + "password": adminPassword, + } + registration := map[string]any{ + "email": c.opts.AdminEmail, + "password": adminPassword, + "name": "E2E Admin", + "acknowledge_weak_password": true, + } + + registerStatus, registerBody, err := postJSON(client, base+"/api/auth/register", registration) + if err != nil { + return nil, fmt.Errorf("registering admin on frontend %d: %w", i, err) + } + if hasSessionCookie(jar, base) { + return client, nil + } + + // No cookie means the user already existed (a repeat call against the same + // Postgres), or registration was rejected. Log in; on failure the + // registration response is the diagnosis, so carry it into the error. + loginStatus, loginBody, err := postJSON(client, base+"/api/auth/login", credentials) + if err != nil { + return nil, fmt.Errorf("logging in admin on frontend %d: %w", i, err) + } + if loginStatus != http.StatusOK { + return nil, fmt.Errorf( + "admin login on frontend %d returned %d (%s); registration had returned %d (%s)", + i, loginStatus, loginBody, registerStatus, registerBody) + } + if !hasSessionCookie(jar, base) { + return nil, fmt.Errorf("admin login on frontend %d returned 200 but set no %q cookie: %s", i, sessionCookieName, loginBody) + } + return client, nil +} + +// GetJSON performs an authenticated GET against a frontend and decodes the body. +func (c *Cluster) GetJSON(client *http.Client, frontend int, path string, out any) error { + base, err := c.frontendBaseURL(frontend) + if err != nil { + return err + } + resp, err := client.Get(base + path) + if err != nil { + return fmt.Errorf("GET %s on frontend %d: %w", path, frontend, err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("GET %s on frontend %d returned %d: %s", path, frontend, resp.StatusCode, excerpt(resp.Body)) + } + if err := json.NewDecoder(resp.Body).Decode(out); err != nil { + return fmt.Errorf("decoding %s from frontend %d: %w", path, frontend, err) + } + return nil +} + +// frontendBaseURL validates the index before FrontendURL indexes the slice: a +// bare index panic in a helper every failover spec calls is far harder to read +// than a named error. +func (c *Cluster) frontendBaseURL(i int) (string, error) { + if i < 0 || i >= len(c.frontends) { + return "", fmt.Errorf("frontend %d out of range (cluster has %d)", i, len(c.frontends)) + } + return c.FrontendURL(i), nil +} + +// postJSON sends body as JSON and returns the status plus an excerpt of the +// response, closing the body in every path. +func postJSON(client *http.Client, endpoint string, body any) (int, string, error) { + encoded, err := json.Marshal(body) + if err != nil { + return 0, "", fmt.Errorf("marshalling request body: %w", err) + } + resp, err := client.Post(endpoint, "application/json", bytes.NewReader(encoded)) + if err != nil { + return 0, "", err + } + defer func() { _ = resp.Body.Close() }() + return resp.StatusCode, excerpt(resp.Body), nil +} + +// hasSessionCookie reports whether the jar holds a usable session for base. +func hasSessionCookie(jar *cookiejar.Jar, base string) bool { + u, err := url.Parse(base) + if err != nil { + return false + } + for _, cookie := range jar.Cookies(u) { + if cookie.Name == sessionCookieName && cookie.Value != "" { + return true + } + } + return false +} + +func excerpt(r io.Reader) string { + data, err := io.ReadAll(io.LimitReader(r, bodyExcerptLimit)) + if err != nil { + return fmt.Sprintf("", err) + } + return string(bytes.TrimSpace(data)) +} diff --git a/tests/e2e/distributed/cluster/cluster_test.go b/tests/e2e/distributed/cluster/cluster_test.go index 4d48f1ecb246..807c275c1c46 100644 --- a/tests/e2e/distributed/cluster/cluster_test.go +++ b/tests/e2e/distributed/cluster/cluster_test.go @@ -3,7 +3,9 @@ package cluster_test import ( "os" "path/filepath" + "time" + "github.com/mudler/LocalAI/pkg/httpclient" "github.com/mudler/LocalAI/tests/e2e/distributed/cluster" . "github.com/onsi/ginkgo/v2" @@ -35,3 +37,23 @@ var _ = Describe("Cluster options", Label("Distributed"), func() { Expect(err.Error()).To(ContainSubstring("at least one frontend")) }) }) + +// The HTTP flow inside AdminSession and GetJSON cannot run here: it needs a +// built local-ai plus real Postgres and NATS, which arrive with the failover +// suites. These specs cover the argument validation that would otherwise panic +// on an out-of-range slice index inside a helper every later spec calls. +var _ = Describe("Admin session", Label("Distributed"), func() { + It("reports a clear error when the frontend index is out of range", func() { + c := cluster.ForTestingEmpty() + _, err := c.AdminSession(3) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("frontend 3")) + }) + + It("reports a clear error when GetJSON names a frontend that does not exist", func() { + c := cluster.ForTestingEmpty() + err := c.GetJSON(httpclient.NewWithTimeout(time.Second), 1, "/api/nodes", &struct{}{}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("frontend 1")) + }) +}) From 23a2bd5f1bdbb279a64bc538cb1205cf470a2827 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 14:03:58 +0000 Subject: [PATCH 10/23] test(distributed): give each frontend its own data dir and one pinned secret Session rows are keyed by an HMAC of the token under a secret generated per instance into {DataPath}/.hmac_secret. The replicas shared that secret only because they shared a working directory, and that directory was the source tree. Give each frontend LOCALAI_DATA_PATH under its own baseDir and pin LOCALAI_AUTH_HMAC_SECRET, so a session minted at one replica resolves at every other one by construction. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/admin.go | 28 +++++++++++++++--------- tests/e2e/distributed/cluster/cluster.go | 25 +++++++++++++++++++-- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/tests/e2e/distributed/cluster/admin.go b/tests/e2e/distributed/cluster/admin.go index b1371f532271..617bcef99086 100644 --- a/tests/e2e/distributed/cluster/admin.go +++ b/tests/e2e/distributed/cluster/admin.go @@ -14,11 +14,12 @@ import ( ) const ( - // adminPassword must satisfy core/http/auth's policy (>= 12 chars and a - // zxcvbn score of 3 against hints that include "admin" and "localai"). - // This one scores 3 today; acknowledgeWeakPassword is sent alongside it so - // a future tightening of the policy cannot silently break every failover - // spec at setup time. + // adminPassword is sent with "acknowledge_weak_password": true, which sets + // PasswordPolicy{AllowWeak: true} and skips the length floor and the zxcvbn + // score entirely (core/http/auth/password.go). Only the technical + // invariants still apply: non-empty, at most 72 bytes, no NUL. The + // acknowledgement is deliberate rather than incidental, so a future + // tightening of the policy cannot break every failover spec at setup time. adminPassword = "e2e-admin-password" // sessionCookieName mirrors the unexported constant in core/http/auth. // The register handler returns 201 both for "user created, here is your @@ -43,11 +44,18 @@ func ForTestingEmpty() *Cluster { // which core/http/auth exempts from the approval gate and assigns the admin // role, so registration alone yields an active admin session. // -// Call this ONCE per cluster and share the client. Two reasons: the auth -// endpoints are rate limited to 5 requests per minute per client IP, and every -// e2e request arrives from 127.0.0.1; and the returned client is already good -// for every frontend, because sessions live in the shared Postgres auth DB and -// Go's cookie jar keys cookies by host without the port. +// Call this ONCE per cluster and share the client. Two reasons: +// +// One, a single rate limiter of 5 requests per minute per client IP guards +// POST /api/auth/token-login, POST /api/auth/register, POST /api/auth/login AND +// PUT /api/auth/password (core/http/routes/auth.go:190). They share one budget, +// and every e2e request arrives from 127.0.0.1, so a spec that changes a +// password spends from the same five. +// +// Two, the returned client is already good for every frontend: sessions live in +// the shared Postgres auth DB, the harness pins one HMAC secret across replicas +// so the session row resolves at any of them, and Go's cookie jar keys cookies +// by host without the port. func (c *Cluster) AdminSession(i int) (*http.Client, error) { base, err := c.frontendBaseURL(i) if err != nil { diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go index 0445fe3868e9..224018833da0 100644 --- a/tests/e2e/distributed/cluster/cluster.go +++ b/tests/e2e/distributed/cluster/cluster.go @@ -71,8 +71,11 @@ type Cluster struct { const ( defaultRegistrationToken = "e2e-token" defaultAdminEmail = "admin@e2e.local" - readinessTimeout = 90 * time.Second - readinessPoll = 200 * time.Millisecond + // testHMACSecret is shared by every frontend so a session minted at one + // replica validates at all of them. See the note in startFrontend. + testHMACSecret = "e2e-cluster-hmac-secret" + readinessTimeout = 90 * time.Second + readinessPoll = 200 * time.Millisecond // processExitTimeout bounds the post-SIGKILL wait in terminate. An unbounded // wait turns one stuck child (D state, or a Wait that never returns) into a // suite-wide Ginkgo timeout that names nothing. @@ -157,6 +160,14 @@ func (c *Cluster) startFrontend(i int, port int) (*Process, error) { if err := os.MkdirAll(filepath.Join(dir, "backends"), 0o755); err != nil { return nil, fmt.Errorf("creating %s dirs: %w", name, err) } + // Without an explicit LOCALAI_DATA_PATH every child resolves DataPath to + // ${cwd}/data (core/cli/run.go:48), which under `go test` is inside the + // source tree and shared by every replica: one collectiondb, one task and + // job store for processes that are meant to be independent. + dataPath := filepath.Join(dir, "data") + if err := os.MkdirAll(dataPath, 0o750); err != nil { + return nil, fmt.Errorf("creating %s dirs: %w", name, err) + } cmd := exec.Command(c.opts.Binary, "run", "--address", fmt.Sprintf("127.0.0.1:%d", port), @@ -171,6 +182,16 @@ func (c *Cluster) startFrontend(i int, port int) (*Process, error) { "LOCALAI_AUTH=true", "LOCALAI_AUTH_DATABASE_URL="+c.opts.PGDSN, "LOCALAI_ADMIN_EMAIL="+c.opts.AdminEmail, + "LOCALAI_DATA_PATH="+dataPath, + // Session rows are keyed by HMAC-SHA256(token, APIKeyHMACSecret), and + // the secret is generated per instance into {DataPath}/.hmac_secret + // unless pinned (core/application/startup.go:141-148). Now that each + // replica owns its data directory, an unpinned secret would differ per + // replica, so the cookie minted at frontend 0 would hash to a session + // row that does not exist at frontend 1 and every post-failover + // /api/nodes call would 401 with nothing in the logs to explain it. + // Pinning makes the cross-replica session a property of the harness. + "LOCALAI_AUTH_HMAC_SECRET="+testHMACSecret, "LOCALAI_REGISTRATION_TOKEN="+c.opts.RegistrationToken, "LOCALAI_AUTO_APPROVE_NODES=true", "DEBUG=true", From ce3f360219ff60cac22d36d130ac12cd312e6581 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 14:12:35 +0000 Subject: [PATCH 11/23] test(distributed): add kill and restart primitives to the cluster harness The point of running LocalAI as real child processes is to be able to take one away. Add KillFrontend (SIGKILL, the lost replica), StopFrontendGracefully (SIGTERM, the rolling update), KillWorker, RestartFrontend and FrontendAlive. RestartFrontend pins the dead replica's original port. Workers read LOCALAI_REGISTER_TO once at boot and never re-resolve it, so a replica that returns on a fresh port is unreachable by exactly the workers that registered with it and the failover under test never happens. It also wipes the replica's data directory, so the process comes back with empty local state and has to rehydrate node, session and job state from the shared Postgres and NATS. Reusing the directory would model a pod with a persistent volume and hide the class of bug these tests exist to find. That is only safe because the harness pins LOCALAI_AUTH_HMAC_SECRET; otherwise the wipe would take {DataPath}/.hmac_secret with it and every session minted before the restart would 401 afterwards. FrontendAlive consults the reaper's exited channel before signal 0: a child that has died but has not yet been waited on is a zombie, and signal 0 to a zombie succeeds, which would report a dead replica as alive. The new specs cover argument validation only. Killing, stopping and restarting a live process needs a built binary plus Postgres and NATS, so those paths stay unexecuted until the failover suites land. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/cluster.go | 6 +- tests/e2e/distributed/cluster/cluster_test.go | 28 ++++ tests/e2e/distributed/cluster/failure.go | 144 ++++++++++++++++++ 3 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 tests/e2e/distributed/cluster/failure.go diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go index 224018833da0..811a94c8c51b 100644 --- a/tests/e2e/distributed/cluster/cluster.go +++ b/tests/e2e/distributed/cluster/cluster.go @@ -152,8 +152,8 @@ func (c *Cluster) startFrontend(i int, port int) (*Process, error) { } port = allocated } - name := fmt.Sprintf("frontend-%d", i) - dir := filepath.Join(c.baseDir, name) + name := frontendName(i) + dir := c.frontendDir(i) if err := os.MkdirAll(filepath.Join(dir, "models"), 0o755); err != nil { return nil, fmt.Errorf("creating %s dirs: %w", name, err) } @@ -164,7 +164,7 @@ func (c *Cluster) startFrontend(i int, port int) (*Process, error) { // ${cwd}/data (core/cli/run.go:48), which under `go test` is inside the // source tree and shared by every replica: one collectiondb, one task and // job store for processes that are meant to be independent. - dataPath := filepath.Join(dir, "data") + dataPath := c.frontendDataDir(i) if err := os.MkdirAll(dataPath, 0o750); err != nil { return nil, fmt.Errorf("creating %s dirs: %w", name, err) } diff --git a/tests/e2e/distributed/cluster/cluster_test.go b/tests/e2e/distributed/cluster/cluster_test.go index 807c275c1c46..2f63d0a28e76 100644 --- a/tests/e2e/distributed/cluster/cluster_test.go +++ b/tests/e2e/distributed/cluster/cluster_test.go @@ -57,3 +57,31 @@ var _ = Describe("Admin session", Label("Distributed"), func() { Expect(err.Error()).To(ContainSubstring("frontend 1")) }) }) + +// Like the admin specs above, these cover argument validation only. Killing, +// stopping and restarting a real replica needs a built local-ai plus Postgres +// and NATS, so those paths stay unexecuted until the failover suites land. +var _ = Describe("Failure primitives", Label("Distributed"), func() { + It("rejects an out-of-range frontend index rather than panicking", func() { + c := cluster.ForTestingEmpty() + Expect(c.KillFrontend(0)).To(MatchError(ContainSubstring("frontend 0 out of range"))) + Expect(c.StopFrontendGracefully(2)).To(MatchError(ContainSubstring("frontend 2 out of range"))) + Expect(c.KillWorker(1)).To(MatchError(ContainSubstring("worker 1 out of range"))) + }) + + It("rejects a restart of a frontend index that does not exist", func() { + Expect(cluster.ForTestingEmpty().RestartFrontend(0)). + To(MatchError(ContainSubstring("frontend 0 out of range"))) + }) + + It("rejects a negative index without treating it as an offset from the end", func() { + c := cluster.ForTestingEmpty() + Expect(c.KillFrontend(-1)).To(MatchError(ContainSubstring("frontend -1 out of range"))) + Expect(c.KillWorker(-1)).To(MatchError(ContainSubstring("worker -1 out of range"))) + Expect(c.FrontendAlive(-1)).To(BeFalse()) + }) + + It("reports a frontend that was never started as not alive", func() { + Expect(cluster.ForTestingEmpty().FrontendAlive(0)).To(BeFalse()) + }) +}) diff --git a/tests/e2e/distributed/cluster/failure.go b/tests/e2e/distributed/cluster/failure.go new file mode 100644 index 000000000000..9e8141e316d4 --- /dev/null +++ b/tests/e2e/distributed/cluster/failure.go @@ -0,0 +1,144 @@ +package cluster + +import ( + "fmt" + "os" + "path/filepath" + "syscall" +) + +// KillFrontend SIGKILLs frontend i. This is the "replica died" case: no drain, +// no graceful deregistration, sockets drop without a FIN from the application. +// +// The signal is delivered but not waited on, because a spec that asserts on the +// cluster's reaction wants to observe the window between the death and the +// survivors noticing it. Poll FrontendAlive with Eventually to join the exit. +func (c *Cluster) KillFrontend(i int) error { + if err := c.checkFrontendIndex(i); err != nil { + return err + } + return signalProcess(c.frontends[i], syscall.SIGKILL) +} + +// StopFrontendGracefully SIGTERMs frontend i. This is the rolling-update case: +// the process gets a chance to drain and deregister. Like KillFrontend it does +// not wait; the point of the distinction between the two is what the process +// does with the time between the signal and its exit. +func (c *Cluster) StopFrontendGracefully(i int) error { + if err := c.checkFrontendIndex(i); err != nil { + return err + } + return signalProcess(c.frontends[i], syscall.SIGTERM) +} + +// KillWorker SIGKILLs worker i. +func (c *Cluster) KillWorker(i int) error { + if i < 0 || i >= len(c.workers) { + return fmt.Errorf("worker %d out of range (cluster has %d)", i, len(c.workers)) + } + return signalProcess(c.workers[i], syscall.SIGKILL) +} + +// RestartFrontend brings frontend i back on its original port with an empty +// data directory, modelling a replaced pod rather than a resumed one. +// +// The port is pinned rather than reallocated: workers read LOCALAI_REGISTER_TO +// once at boot and never re-resolve it, so a replica that returns on a new port +// is unreachable by exactly the workers that registered with it, and the +// failover the spec means to observe never happens. Rebinding is safe because +// the previous listener is fully closed before the new process starts (see the +// terminate below) and Go's listeners set SO_REUSEADDR, so a lingering +// TIME_WAIT on an accepted connection does not block the bind. +// +// The data directory is wiped so the replica must rehydrate node, session and +// job state from the shared Postgres and NATS. Keeping it would model a pod +// with a persistent volume and would hide the very class of bug these tests +// exist to find. This is only safe because startFrontend pins +// LOCALAI_AUTH_HMAC_SECRET: the secret otherwise lives at +// {DataPath}/.hmac_secret, and wiping it would make every session minted before +// the restart hash to a row the restarted replica cannot find, turning a +// failover assertion into an unexplained 401. +func (c *Cluster) RestartFrontend(i int) error { + if err := c.checkFrontendIndex(i); err != nil { + return err + } + old := c.frontends[i] + if old == nil { + return fmt.Errorf("frontend %d was never started, nothing to restart", i) + } + // The old process may still be running (a restart with no preceding kill) or + // already dead but unreaped. terminate is idempotent, bounds its wait, and + // releases the log handle the replacement is about to reopen; without it the + // replacement races the old listener for the port and leaks a file + // descriptor per restart. + old.terminate() + + if err := os.RemoveAll(c.frontendDataDir(i)); err != nil { + return fmt.Errorf("wiping data dir of frontend %d: %w", i, err) + } + + p, err := c.startFrontend(i, old.Port) + if err != nil { + return fmt.Errorf("restarting frontend %d: %w", i, err) + } + c.frontends[i] = p + return nil +} + +// FrontendAlive reports whether frontend i's process is still running. +func (c *Cluster) FrontendAlive(i int) bool { + if i < 0 || i >= len(c.frontends) { + return false + } + return c.frontends[i].alive() +} + +// alive reports whether the process is still running. The reaper's exited +// channel is authoritative and is consulted first: between a child's death and +// the reaper's Wait returning, the child is a zombie, and signal 0 to a zombie +// succeeds, which would report a dead replica as alive. +func (p *Process) alive() bool { + if p == nil || p.Cmd == nil || p.Cmd.Process == nil { + return false + } + select { + case <-p.exited: + return false + default: + } + // Signal 0 tests for existence without delivering anything. + return p.Cmd.Process.Signal(syscall.Signal(0)) == nil +} + +func signalProcess(p *Process, sig syscall.Signal) error { + if p == nil || p.Cmd == nil || p.Cmd.Process == nil { + return fmt.Errorf("process is not running") + } + if err := p.Cmd.Process.Signal(sig); err != nil { + return fmt.Errorf("signalling %s with %v: %w", p.Name, sig, err) + } + return nil +} + +// checkFrontendIndex keeps the out-of-range wording identical across every +// primitive, so a failing spec reads the same whichever one tripped. +func (c *Cluster) checkFrontendIndex(i int) error { + if i < 0 || i >= len(c.frontends) { + return fmt.Errorf("frontend %d out of range (cluster has %d)", i, len(c.frontends)) + } + return nil +} + +func frontendName(i int) string { + return fmt.Sprintf("frontend-%d", i) +} + +func (c *Cluster) frontendDir(i int) string { + return filepath.Join(c.baseDir, frontendName(i)) +} + +// frontendDataDir is LOCALAI_DATA_PATH for frontend i. RestartFrontend wipes it, +// so it must be the exact path startFrontend hands the child. +func (c *Cluster) frontendDataDir(i int) string { + return filepath.Join(c.frontendDir(i), "data") +} From fc0fce8b7d1db21f2a75cc48bfc1e42d8a87e537 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 14:22:35 +0000 Subject: [PATCH 12/23] test(distributed): correct the failure-primitive comments and guard the wipe Review round 1. Comments only, plus one guard. The note on Process.alive claimed the exited check closed the zombie window. It does not. The reaper closes exited only after Cmd.Wait returns, and Wait marks the os.Process done before returning, so exited being closed implies signal 0 already errors and the branch cannot fire earlier than the one it precedes. The window between the child exiting and waitid collecting it stays open in both versions, and the only real mitigation is for callers to poll with Eventually rather than sample once. Keep the check as hygiene, say what it actually does, and say it again on the exited field, so nobody reads the old claim and drops the Eventually. Record what the cold wipe destroys. The harness sets no LOCALAI_STORAGE_URL, so the object store is a directory under DataPath, and quantization and fine-tune outputs live there too. Postgres keeps the job row; the artifact it points at does not survive the restart. A spec that asserts otherwise will fail for a storage reason wearing a failover costume. Tell callers to let a graceful stop finish before restarting: RestartFrontend terminates with SIGKILL, so pairing it straight after StopFrontendGracefully cuts the drain short and silently converts the rolling-update case into the crash case. Refuse to wipe when the cluster has no work dir. frontendDataDir is relative when baseDir is empty, so a Cluster built by some future test helper without one would have RemoveAll walking frontend-N/data inside the source tree. The guard sits before terminate, so a refusal leaves the cluster as it was. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/cluster.go | 5 +++ tests/e2e/distributed/cluster/failure.go | 41 +++++++++++++++++++++--- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go index 811a94c8c51b..6cf15f349cdc 100644 --- a/tests/e2e/distributed/cluster/cluster.go +++ b/tests/e2e/distributed/cluster/cluster.go @@ -56,6 +56,11 @@ type Process struct { // exited closes once the reaper has collected the process. Only the reaper // calls Cmd.Wait, so nothing else may: a second Wait on the same Cmd races // the first and corrupts ProcessState. + // + // A closed exited proves the child is gone. An open one proves nothing: it + // is still open for the whole interval between the child exiting and waitid + // collecting it, during which the child is a zombie that signal 0 reports as + // alive. Anything asserting on a process being dead must poll, not sample. exited chan struct{} waitErr error } diff --git a/tests/e2e/distributed/cluster/failure.go b/tests/e2e/distributed/cluster/failure.go index 9e8141e316d4..3f2a1f42ba28 100644 --- a/tests/e2e/distributed/cluster/failure.go +++ b/tests/e2e/distributed/cluster/failure.go @@ -58,6 +58,25 @@ func (c *Cluster) KillWorker(i int) error { // {DataPath}/.hmac_secret, and wiping it would make every session minted before // the restart hash to a row the restarted replica cannot find, turning a // failover assertion into an unexplained 401. +// +// The wipe also destroys state that nothing can rebuild. This harness sets no +// LOCALAI_STORAGE_URL, so the distributed object store is a directory under +// {DataPath} (core/application/distributed.go:146), and quantization and +// fine-tune jobs write their outputs to {DataPath}/quantization and +// {DataPath}/fine-tune (core/services/{quantization,finetune}/service.go:95); +// agent state, router-corpus, the voiceprofile store and {DataPath}/traces go +// the same way. Postgres keeps the job row, the artifact it points at is gone. +// So a spec that finishes a quantization or fine-tune on a replica, restarts +// it, and then asserts the artifact is retrievable fails for a storage reason +// dressed up as a failover one. No spec does that today; this note is here so +// the first one that tries does not spend a day on it. +// +// After StopFrontendGracefully, wait for the process to actually go +// (Eventually(c.FrontendAlive).Should(BeFalse())) before restarting. Restart +// terminates whatever is still running with SIGKILL, so restarting straight +// after a SIGTERM cuts the drain short and quietly turns the rolling-update +// case into the crash case, which is the opposite of what pairing those two +// calls is meant to express. func (c *Cluster) RestartFrontend(i int) error { if err := c.checkFrontendIndex(i); err != nil { return err @@ -66,13 +85,18 @@ func (c *Cluster) RestartFrontend(i int) error { if old == nil { return fmt.Errorf("frontend %d was never started, nothing to restart", i) } + // frontendDataDir is relative when baseDir is empty, and this deletes it: + // a Cluster assembled by a future test helper without a work dir would have + // RemoveAll walking "frontend-N/data" under the package source directory. + if c.baseDir == "" { + return fmt.Errorf("refusing to wipe the data dir of frontend %d: cluster has no work dir", i) + } // The old process may still be running (a restart with no preceding kill) or // already dead but unreaped. terminate is idempotent, bounds its wait, and // releases the log handle the replacement is about to reopen; without it the // replacement races the old listener for the port and leaks a file // descriptor per restart. old.terminate() - if err := os.RemoveAll(c.frontendDataDir(i)); err != nil { return fmt.Errorf("wiping data dir of frontend %d: %w", i, err) } @@ -93,10 +117,17 @@ func (c *Cluster) FrontendAlive(i int) bool { return c.frontends[i].alive() } -// alive reports whether the process is still running. The reaper's exited -// channel is authoritative and is consulted first: between a child's death and -// the reaper's Wait returning, the child is a zombie, and signal 0 to a zombie -// succeeds, which would report a dead replica as alive. +// alive reports whether the process is still running. +// +// The exited check is cheap hygiene, not a fix for the zombie window. The +// reaper closes exited only after Cmd.Wait returns, and Wait marks the +// os.Process done before it returns (runtime/os pidfd path), so by the time +// exited is closed signal 0 already errors: this branch cannot fire earlier +// than the one it precedes. The window that stays open is the other one, +// between the child exiting and waitid collecting it: there the child is a +// zombie, signal 0 to a zombie succeeds, and alive reports true for a process +// that is already dead. There is no local fix; the caller's is to poll, +// Eventually(c.FrontendAlive).Should(BeFalse()), rather than assert once. func (p *Process) alive() bool { if p == nil || p.Cmd == nil || p.Cmd.Process == nil { return false From 1f241fb3105694d41c62f5b1dfa059bc9f508505 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 14:38:12 +0000 Subject: [PATCH 13/23] test(distributed): prove the cluster harness with a two-replica baseline Tasks 4 to 6 built a harness that runs local-ai as real child processes, but none of it had ever started a process: every spec so far returned inside argument validation. These two specs are the first to run it against a real binary, a real Postgres and a real NATS. Two frontends against one database both see a worker that registered through only one of them. Every failover spec assumes this, so it is asserted first. One admin session is minted at frontend 0 and reused for both replicas rather than registering per frontend. The auth routes share a five-per-minute-per-IP limiter and all e2e traffic is 127.0.0.1, so a session per frontend would exhaust the budget as soon as a spec needs a third one. Reuse is sound because sessions live in the shared Postgres and the harness pins one HMAC secret across replicas; frontend 1 answering /api/nodes with 200 on a cookie minted at frontend 0 is what proves it. The binaries are resolved before SetupInfra so a missing build skips without first provisioning a database the skip would then have to tear down. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .../e2e/distributed/cluster_baseline_test.go | 148 ++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 tests/e2e/distributed/cluster_baseline_test.go diff --git a/tests/e2e/distributed/cluster_baseline_test.go b/tests/e2e/distributed/cluster_baseline_test.go new file mode 100644 index 000000000000..45101aaec29a --- /dev/null +++ b/tests/e2e/distributed/cluster_baseline_test.go @@ -0,0 +1,148 @@ +package distributed_test + +import ( + "net/http" + "os" + "path/filepath" + + "github.com/mudler/LocalAI/tests/e2e/distributed/cluster" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// nodeRosterTimeout bounds the wait for a worker to appear healthy in +// /api/nodes. Registration is an HTTP call the worker retries, followed by a +// heartbeat that has to land before the frontend calls the node healthy, so the +// budget covers several retry intervals rather than a single round trip. +const ( + nodeRosterTimeout = "90s" + nodeRosterPoll = "1s" +) + +// node is the subset of the /api/nodes payload these specs assert on. +type node struct { + Name string `json:"name"` + Status string `json:"status"` +} + +// localAIBinary resolves the built binary, skipping rather than failing when it +// is absent so `make test-e2e-distributed` still runs for someone who has not +// built it. CI always builds it, so CI never skips. +func localAIBinary() string { + GinkgoHelper() + path := os.Getenv("LOCALAI_E2E_BINARY") + if path == "" { + wd, err := os.Getwd() + Expect(err).ToNot(HaveOccurred()) + path = filepath.Join(wd, "..", "..", "..", "local-ai") + } + if _, err := os.Stat(path); err != nil { + Skip("local-ai binary not found at " + path + "; run `make build` or set LOCALAI_E2E_BINARY") + } + return path +} + +func mockBackendBinary() string { + GinkgoHelper() + wd, err := os.Getwd() + Expect(err).ToNot(HaveOccurred()) + path := filepath.Join(wd, "..", "mock-backend", "mock-backend") + if _, err := os.Stat(path); err != nil { + Skip("mock-backend not found at " + path + "; run `make build-mock-backend`") + } + return path +} + +// startCluster brings up a cluster against a freshly provisioned database and +// registers cleanup, including a log dump on failure. +func startCluster(frontends, workers int) *cluster.Cluster { + GinkgoHelper() + + // Resolved before SetupInfra so a missing binary skips without having paid + // for a database that the skip would then leave to DeferCleanup. + binary := localAIBinary() + mockBackend := mockBackendBinary() + + infra := SetupInfra("cluster") + + // The log directory must be predictable so CI can upload it as an artifact. + // GinkgoT().TempDir() lands under TMPDIR, which on a GitHub runner is not + // /tmp, so an artifact glob would silently match nothing. + logDir := os.Getenv("LOCALAI_E2E_LOG_DIR") + if logDir == "" { + logDir = GinkgoT().TempDir() + } else { + logDir = filepath.Join(logDir, sanitizeDBName(CurrentSpecReport().LeafNodeText)) + Expect(os.MkdirAll(logDir, 0o755)).To(Succeed()) + } + + c, err := cluster.Start(cluster.Options{ + Binary: binary, + MockBackend: mockBackend, + PGDSN: infra.PGURL, + NatsURL: infra.NatsURL, + LogDir: logDir, + Frontends: frontends, + Workers: workers, + }) + Expect(err).ToNot(HaveOccurred()) + + DeferCleanup(func() { + if CurrentSpecReport().Failed() { + c.DumpLogs() + } + c.Stop() + }) + return c +} + +// healthyNodeNames polls one frontend's roster. It returns nil on any error so +// Eventually keeps retrying: the roster is unreachable for the first moments of +// a replica's life, and a hard failure there would only re-report a startup race. +func healthyNodeNames(c *cluster.Cluster, client *http.Client, frontend int) []string { + var nodes []node + if err := c.GetJSON(client, frontend, "/api/nodes", &nodes); err != nil { + return nil + } + names := []string{} + for _, n := range nodes { + if n.Status == "healthy" { + names = append(names, n.Name) + } + } + return names +} + +var _ = Describe("Cluster baseline", Label("Distributed"), Label("Cluster"), func() { + It("brings up a frontend and a worker, and the worker appears in the roster", func() { + c := startCluster(1, 1) + + client, err := c.AdminSession(0) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() []string { + return healthyNodeNames(c, client, 0) + }, nodeRosterTimeout, nodeRosterPoll).Should(ContainElement(c.WorkerName(0))) + }) + + It("runs two frontends against one database and both see the same worker", func() { + c := startCluster(2, 1) + + // One session for the whole cluster, minted at frontend 0. Registering or + // logging in again per frontend would spend from the same five-per-minute + // budget the auth routes share per client IP, and every request here comes + // from 127.0.0.1. The single client is valid at both replicas: sessions + // live in the shared Postgres, the harness pins one HMAC secret so the + // row resolves anywhere, and Go's cookie jar keys by host without port. + client, err := c.AdminSession(0) + Expect(err).ToNot(HaveOccurred()) + + for _, frontend := range []int{0, 1} { + Eventually(func() []string { + return healthyNodeNames(c, client, frontend) + }, nodeRosterTimeout, nodeRosterPoll).Should(ContainElement(c.WorkerName(0)), + "frontend %d should see the worker registered through frontend 0", frontend) + } + }) +}) From 2e4c731ea19b9c32ce7549a273df00c7b748326f Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 14:54:23 +0000 Subject: [PATCH 14/23] test(distributed): fail rather than skip the cluster specs in CI The Cluster label partition is these two specs and nothing else, so a missing binary skipped the entire job. Ginkgo exits 0 on skips, so a build step that broke or moved its output would have left the job reporting "0 Passed | 2 Skipped" and going green without ever starting a cluster: the silent pass this suite exists to make impossible. Skipping stays the local default, which is the right courtesy for someone who has not run `make build`, but LOCALAI_E2E_REQUIRE_BINARIES turns it into a failure that names the missing path and the target that builds it. A value that is set but unparseable counts as on, since reading it as off would restore the very skip it disables. Failures also name themselves now. The roster poll kept returning a bare nil on error, so a 401 at the second replica, a decode failure and "the worker never registered" all presented identically as an empty list. It now retains the last error and the last roster and reports whichever happened, through a lazily evaluated Gomega description that costs nothing until something fails. Finally, the two-frontend spec no longer depends on the harness to mean what it says. It asserts an unauthenticated GET /api/nodes at frontend 1 is refused, which observes the admin gate instead of assuming it, and it compares the worker's registration id across the two replicas rather than its name. A future harness that registered every worker with every frontend would have kept a name-only assertion green while it quietly stopped proving anything about shared state. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .../e2e/distributed/cluster_baseline_test.go | 173 +++++++++++++++--- 1 file changed, 143 insertions(+), 30 deletions(-) diff --git a/tests/e2e/distributed/cluster_baseline_test.go b/tests/e2e/distributed/cluster_baseline_test.go index 45101aaec29a..e25739b71ba4 100644 --- a/tests/e2e/distributed/cluster_baseline_test.go +++ b/tests/e2e/distributed/cluster_baseline_test.go @@ -1,34 +1,80 @@ package distributed_test import ( + "fmt" "net/http" "os" "path/filepath" + "strconv" + "strings" + "time" + "github.com/mudler/LocalAI/pkg/httpclient" "github.com/mudler/LocalAI/tests/e2e/distributed/cluster" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) -// nodeRosterTimeout bounds the wait for a worker to appear healthy in -// /api/nodes. Registration is an HTTP call the worker retries, followed by a -// heartbeat that has to land before the frontend calls the node healthy, so the -// budget covers several retry intervals rather than a single round trip. const ( + // nodeRosterTimeout bounds the wait for a worker to appear healthy in + // /api/nodes. Registration is an HTTP call the worker retries, followed by a + // heartbeat that has to land before the frontend calls the node healthy, so + // the budget covers several retry intervals rather than a single round trip. nodeRosterTimeout = "90s" nodeRosterPoll = "1s" + // authProbeTimeout bounds the single unauthenticated request that checks the + // admin gate is actually closed. One round trip against a ready local + // process; anything slower is a defect, not slowness. + authProbeTimeout = 30 * time.Second ) -// node is the subset of the /api/nodes payload these specs assert on. +// node is the subset of the /api/nodes payload these specs assert on. ID is the +// registration identity the worker minted, which is what distinguishes "the +// same node row seen from a second replica" from "a second registration that +// happens to share a name". type node struct { + ID string `json:"id"` Name string `json:"name"` Status string `json:"status"` } -// localAIBinary resolves the built binary, skipping rather than failing when it -// is absent so `make test-e2e-distributed` still runs for someone who has not -// built it. CI always builds it, so CI never skips. +// requireBinaries reports whether a missing binary must fail the spec instead of +// skipping it. +// +// Skipping is the right courtesy locally: someone who has not run `make build` +// should get a clear note, not a wall of red. In CI it is the opposite. The +// whole Cluster label partition is these two specs, so if the workflow's build +// step breaks or moves its output, a skip would leave the job reporting +// "0 Passed | 2 Skipped" and exiting 0. Ginkgo exits 0 on skips, so that job +// goes green having never started a cluster, which is precisely the silent pass +// this suite exists to make impossible. CI sets this variable (Task 9) and gets +// a failure instead. +func requireBinaries() bool { + value := strings.TrimSpace(os.Getenv("LOCALAI_E2E_REQUIRE_BINARIES")) + if value == "" { + return false + } + if parsed, err := strconv.ParseBool(value); err == nil { + return parsed + } + // Set but unparseable means someone meant to turn this on. Reading it as + // false would quietly restore the silent skip the flag exists to prevent. + return true +} + +// missingBinary skips or fails, naming the path and the target that builds it. +func missingBinary(what, path, makeTarget string) { + GinkgoHelper() + message := fmt.Sprintf("%s not found at %s; run `%s`", what, path, makeTarget) + if requireBinaries() { + Fail(message + " (LOCALAI_E2E_REQUIRE_BINARIES is set, so this fails rather than skips: " + + "in CI a skipped cluster spec is indistinguishable from a passing one)") + } + Skip(message) +} + +// localAIBinary resolves the built binary. func localAIBinary() string { GinkgoHelper() path := os.Getenv("LOCALAI_E2E_BINARY") @@ -38,7 +84,7 @@ func localAIBinary() string { path = filepath.Join(wd, "..", "..", "..", "local-ai") } if _, err := os.Stat(path); err != nil { - Skip("local-ai binary not found at " + path + "; run `make build` or set LOCALAI_E2E_BINARY") + missingBinary("local-ai binary", path, "make build") } return path } @@ -49,7 +95,7 @@ func mockBackendBinary() string { Expect(err).ToNot(HaveOccurred()) path := filepath.Join(wd, "..", "mock-backend", "mock-backend") if _, err := os.Stat(path); err != nil { - Skip("mock-backend not found at " + path + "; run `make build-mock-backend`") + missingBinary("mock-backend", path, "make build-mock-backend") } return path } @@ -97,16 +143,39 @@ func startCluster(frontends, workers int) *cluster.Cluster { return c } -// healthyNodeNames polls one frontend's roster. It returns nil on any error so -// Eventually keeps retrying: the roster is unreachable for the first moments of -// a replica's life, and a hard failure there would only re-report a startup race. -func healthyNodeNames(c *cluster.Cluster, client *http.Client, frontend int) []string { - var nodes []node - if err := c.GetJSON(client, frontend, "/api/nodes", &nodes); err != nil { +// rosterProbe polls one frontend's node roster. +// +// It keeps the last error and the last roster it saw so a failing Eventually can +// name the cause. Returning a bare nil on error makes a 401 at the second +// replica, a JSON decode failure and "the worker never registered" all present +// identically as an empty list, which is the least useful thing a failover +// suite can say when it goes red. +type rosterProbe struct { + cluster *cluster.Cluster + client *http.Client + frontend int + + lastErr error + lastSeen []node +} + +func newRosterProbe(c *cluster.Cluster, client *http.Client, frontend int) *rosterProbe { + return &rosterProbe{cluster: c, client: client, frontend: frontend} +} + +// healthyNames returns nil on any error so Eventually keeps retrying: the roster +// is unreachable for the first moments of a replica's life, and failing hard +// there would only re-report a startup race. +func (p *rosterProbe) healthyNames() []string { + var roster []node + if err := p.cluster.GetJSON(p.client, p.frontend, "/api/nodes", &roster); err != nil { + p.lastErr = err return nil } + p.lastErr = nil + p.lastSeen = roster names := []string{} - for _, n := range nodes { + for _, n := range roster { if n.Status == "healthy" { names = append(names, n.Name) } @@ -114,6 +183,27 @@ func healthyNodeNames(c *cluster.Cluster, client *http.Client, frontend int) []s return names } +// idOf returns the registration ID the roster last reported for a node name. +func (p *rosterProbe) idOf(name string) string { + for _, n := range p.lastSeen { + if n.Name == name { + return n.ID + } + } + return "" +} + +// describe is handed to Should as the failure message. Gomega calls a +// func() string description lazily, so this runs only on failure and reports +// whichever of the two distinct causes actually occurred. +func (p *rosterProbe) describe() string { + if p.lastErr != nil { + return fmt.Sprintf("frontend %d: the last GET /api/nodes failed: %v", p.frontend, p.lastErr) + } + return fmt.Sprintf("frontend %d: GET /api/nodes succeeded but the roster held %d node(s): %+v", + p.frontend, len(p.lastSeen), p.lastSeen) +} + var _ = Describe("Cluster baseline", Label("Distributed"), Label("Cluster"), func() { It("brings up a frontend and a worker, and the worker appears in the roster", func() { c := startCluster(1, 1) @@ -121,28 +211,51 @@ var _ = Describe("Cluster baseline", Label("Distributed"), Label("Cluster"), fun client, err := c.AdminSession(0) Expect(err).ToNot(HaveOccurred()) - Eventually(func() []string { - return healthyNodeNames(c, client, 0) - }, nodeRosterTimeout, nodeRosterPoll).Should(ContainElement(c.WorkerName(0))) + probe := newRosterProbe(c, client, 0) + Eventually(probe.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ContainElement(c.WorkerName(0)), probe.describe) }) It("runs two frontends against one database and both see the same worker", func() { c := startCluster(2, 1) + // Observe that frontend 1 really is gated before proving a session opens + // it. Without this, "the cookie minted at frontend 0 works here" is + // indistinguishable from "this endpoint needs no auth at all". The probe + // is free: it touches no auth route, so it spends nothing from the + // five-per-minute-per-IP budget those routes share. + anonymous := httpclient.NewWithTimeout(authProbeTimeout) + refused, err := anonymous.Get(c.FrontendURL(1) + "/api/nodes") + Expect(err).ToNot(HaveOccurred()) + defer func() { _ = refused.Body.Close() }() + Expect(refused.StatusCode).To(Equal(http.StatusUnauthorized), + "an unauthenticated GET /api/nodes must be refused, otherwise this spec proves nothing about sessions") + // One session for the whole cluster, minted at frontend 0. Registering or // logging in again per frontend would spend from the same five-per-minute - // budget the auth routes share per client IP, and every request here comes - // from 127.0.0.1. The single client is valid at both replicas: sessions - // live in the shared Postgres, the harness pins one HMAC secret so the - // row resolves anywhere, and Go's cookie jar keys by host without port. + // budget, and every request here comes from 127.0.0.1. The single client + // is valid at both replicas: sessions live in the shared Postgres, the + // harness pins one HMAC secret so the row resolves anywhere, and Go's + // cookie jar keys by host without port. client, err := c.AdminSession(0) Expect(err).ToNot(HaveOccurred()) - for _, frontend := range []int{0, 1} { - Eventually(func() []string { - return healthyNodeNames(c, client, frontend) - }, nodeRosterTimeout, nodeRosterPoll).Should(ContainElement(c.WorkerName(0)), - "frontend %d should see the worker registered through frontend 0", frontend) - } + // The worker is pointed at frontend 0 alone (the harness sets + // LOCALAI_REGISTER_TO to frontend 0), so read its identity there first. + at0 := newRosterProbe(c, client, 0) + Eventually(at0.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ContainElement(c.WorkerName(0)), at0.describe) + registeredID := at0.idOf(c.WorkerName(0)) + Expect(registeredID).ToNot(BeEmpty(), "frontend 0 reported the worker without a registration ID") + + // Then assert frontend 1 serves the same row, not merely a node with the + // same name. Comparing IDs pins the topology inside the spec: a future + // harness that registered every worker with every frontend would keep a + // name-only assertion green while it quietly stopped testing shared state. + at1 := newRosterProbe(c, client, 1) + Eventually(at1.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ContainElement(c.WorkerName(0)), at1.describe) + Expect(at1.idOf(c.WorkerName(0))).To(Equal(registeredID), + "frontend 1 must serve the same node row that registered through frontend 0, not a separate registration") }) }) From 737eb6c34cc71a255f0c25d5eac2fd3ed8ab563b Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 15:06:54 +0000 Subject: [PATCH 15/23] test(distributed): require the cluster binaries by default under CI The previous round made a missing binary fail instead of skip, but only when a workflow remembered to set LOCALAI_E2E_REQUIRE_BINARIES. That leaves the silent pass one forgotten line away: the Cluster label partition is two specs, Ginkgo exits 0 on skips, and a job that skips both reports "0 Passed | 2 Skipped" and goes green having never started a cluster. So the polarity is inverted. Binaries are required whenever CI is set, which GitHub Actions always does, and the flag now exists to force the requirement OFF rather than to be remembered ON. A local developer sees no change, since CI is unset in an ordinary shell and a missing binary still skips with a message naming the path and how to build it. off, no, n and disabled are honoured as off; ParseBool rejects them, and reading a word that unambiguous as its opposite would be a worse trap than the one this removes. Also correct a claim the previous commit message got wrong. Comparing the worker's registration id across the two replicas does not pin the topology: NodeRegistry.Register looks a node up by name and preserves the existing id, and both replicas read one Postgres, so registering the worker with every frontend would yield identical ids too. The assertion is still worth keeping for what it does catch, a replica answering from its own registry or database instead of the shared one, and the comment now says that and nothing more. The topology fact moves to where someone would break it: a note on LOCALAI_REGISTER_TO recording that workers register with frontend 0 only, that the cross-replica specs depend on it, and that nothing in those specs can detect a change to it. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/cluster.go | 12 ++++ .../e2e/distributed/cluster_baseline_test.go | 55 +++++++++++++------ 2 files changed, 50 insertions(+), 17 deletions(-) diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go index 6cf15f349cdc..0077c21cefe6 100644 --- a/tests/e2e/distributed/cluster/cluster.go +++ b/tests/e2e/distributed/cluster/cluster.go @@ -248,6 +248,18 @@ func (c *Cluster) startWorker(i int) (*Process, error) { fmt.Sprintf("LOCALAI_ADVERTISE_ADDR=127.0.0.1:%d", grpcPort), fmt.Sprintf("LOCALAI_HTTP_ADDR=127.0.0.1:%d", httpPort), fmt.Sprintf("LOCALAI_ADVERTISE_HTTP_ADDR=127.0.0.1:%d", httpPort), + // Workers register with frontend 0 ONLY, never with every replica, and + // the cross-replica session specs depend on that. They prove a session + // minted at frontend 0 resolves at frontend 1 by reading a node that + // only frontend 0 was ever told about; register the worker everywhere + // and they still pass while proving nothing. + // + // Nothing in those specs can detect the change. The registry keys nodes + // by name and preserves ids across the shared Postgres + // (core/services/nodes/registry.go:522-527), so a roster read at + // frontend 1 looks identical either way. Anyone pointing workers at + // more than one replica must revisit + // tests/e2e/distributed/cluster_baseline_test.go by hand. "LOCALAI_REGISTER_TO="+c.FrontendURL(0), "LOCALAI_NODE_NAME="+name, "LOCALAI_REGISTRATION_TOKEN="+c.opts.RegistrationToken, diff --git a/tests/e2e/distributed/cluster_baseline_test.go b/tests/e2e/distributed/cluster_baseline_test.go index e25739b71ba4..662f93b38835 100644 --- a/tests/e2e/distributed/cluster_baseline_test.go +++ b/tests/e2e/distributed/cluster_baseline_test.go @@ -40,7 +40,7 @@ type node struct { } // requireBinaries reports whether a missing binary must fail the spec instead of -// skipping it. +// skipping it. It defaults to ON under CI. // // Skipping is the right courtesy locally: someone who has not run `make build` // should get a clear note, not a wall of red. In CI it is the opposite. The @@ -48,28 +48,42 @@ type node struct { // step breaks or moves its output, a skip would leave the job reporting // "0 Passed | 2 Skipped" and exiting 0. Ginkgo exits 0 on skips, so that job // goes green having never started a cluster, which is precisely the silent pass -// this suite exists to make impossible. CI sets this variable (Task 9) and gets -// a failure instead. +// this suite exists to make impossible. +// +// Hence the polarity: the safe behaviour is the default, keyed off CI (GitHub +// Actions always sets it), and LOCALAI_E2E_REQUIRE_BINARIES exists to be forced +// OFF rather than to be remembered ON. A future workflow author cannot reach +// the green-on-nothing state by forgetting a line, only by writing one that +// explicitly asks for it. A local developer sees no change: CI is unset in an +// ordinary shell, so a missing binary still skips. func requireBinaries() bool { value := strings.TrimSpace(os.Getenv("LOCALAI_E2E_REQUIRE_BINARIES")) if value == "" { + return os.Getenv("CI") != "" + } + // ParseBool rejects these, and the fallback below reads anything it rejects + // as ON. Someone writing "off" plainly means off, and silently inverting + // them would be a worse trap than the one this flag removes. + switch strings.ToLower(value) { + case "off", "no", "n", "disabled": return false } if parsed, err := strconv.ParseBool(value); err == nil { return parsed } - // Set but unparseable means someone meant to turn this on. Reading it as - // false would quietly restore the silent skip the flag exists to prevent. + // Set to something meaningless means someone meant to turn this on. Reading + // it as false would quietly restore the silent skip the flag guards against. return true } -// missingBinary skips or fails, naming the path and the target that builds it. -func missingBinary(what, path, makeTarget string) { +// missingBinary skips or fails, naming the path and how to produce it. +func missingBinary(what, path, remedy string) { GinkgoHelper() - message := fmt.Sprintf("%s not found at %s; run `%s`", what, path, makeTarget) + message := fmt.Sprintf("%s not found at %s; %s", what, path, remedy) if requireBinaries() { - Fail(message + " (LOCALAI_E2E_REQUIRE_BINARIES is set, so this fails rather than skips: " + - "in CI a skipped cluster spec is indistinguishable from a passing one)") + Fail(message + " (binaries are required here, either under CI or via " + + "LOCALAI_E2E_REQUIRE_BINARIES, so this fails rather than skips: a skipped " + + "cluster spec is indistinguishable from a passing one)") } Skip(message) } @@ -84,7 +98,7 @@ func localAIBinary() string { path = filepath.Join(wd, "..", "..", "..", "local-ai") } if _, err := os.Stat(path); err != nil { - missingBinary("local-ai binary", path, "make build") + missingBinary("local-ai binary", path, "run `make build` or set LOCALAI_E2E_BINARY") } return path } @@ -95,7 +109,7 @@ func mockBackendBinary() string { Expect(err).ToNot(HaveOccurred()) path := filepath.Join(wd, "..", "mock-backend", "mock-backend") if _, err := os.Stat(path); err != nil { - missingBinary("mock-backend", path, "make build-mock-backend") + missingBinary("mock-backend", path, "run `make build-mock-backend`") } return path } @@ -248,14 +262,21 @@ var _ = Describe("Cluster baseline", Label("Distributed"), Label("Cluster"), fun registeredID := at0.idOf(c.WorkerName(0)) Expect(registeredID).ToNot(BeEmpty(), "frontend 0 reported the worker without a registration ID") - // Then assert frontend 1 serves the same row, not merely a node with the - // same name. Comparing IDs pins the topology inside the spec: a future - // harness that registered every worker with every frontend would keep a - // name-only assertion green while it quietly stopped testing shared state. + // Then assert frontend 1 serves the same row, by id and not merely by name. + // + // Be precise about what this proves. It does NOT pin the topology: + // NodeRegistry.Register looks a node up by name and preserves the + // existing id (core/services/nodes/registry.go:522-527), and both + // replicas read one Postgres, so a harness that registered the worker + // with every frontend would yield identical ids here too. What it does + // catch is a frontend answering from its own registry or its own + // database rather than the shared one, which is a different regression + // and just as silent. The topology fact is not asserted anywhere; it is + // recorded next to LOCALAI_REGISTER_TO in cluster.go. at1 := newRosterProbe(c, client, 1) Eventually(at1.healthyNames, nodeRosterTimeout, nodeRosterPoll). Should(ContainElement(c.WorkerName(0)), at1.describe) Expect(at1.idOf(c.WorkerName(0))).To(Equal(registeredID), - "frontend 1 must serve the same node row that registered through frontend 0, not a separate registration") + "frontend 1 must resolve the same node row as frontend 0; a differing id means it is not reading the shared state") }) }) From 875ff339ba92385efcfb447569c0d777c5633cad Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 16:28:10 +0000 Subject: [PATCH 16/23] test(distributed): cover replica death, rolling restart and racing replicas Four scenarios with no prior equivalent: killing a replica must not disturb a worker that never depended on it, a cold-restarted replica must rehydrate the roster from shared state and keep accepting the worker's heartbeats, a dead worker must settle to offline on every replica, and two replicas registering a worker each must converge on one roster. The timings are measured, not assumed. Node liveness is heartbeat freshness, so the only eviction path is StaleNodeThreshold (60s) plus one HealthCheckInterval tick (15s), and neither is reachable from the CLI. A worker whose registrar was killed was observed going offline at 74.2s. Every window here is sized to outlast that, because an assertion that expires before the system could have reacted proves nothing. Two assertions are deliberately unlike the obvious form. Statuses are compared for equality against a probe that returns a sentinel on error, rather than asserting a name is absent from the healthy list: the list probe returns nil on any error, and "does not contain" is satisfied by nil, so a 401 at the second replica would have passed while observing nothing. And a killed worker is required to settle to exactly offline, because it first flaps to unhealthy at ~8s and back to healthy at ~14s, which any not-healthy matcher would accept. SpreadWorkerRegistrations is new, off by default, and exists so the racing spec is a race: the harness otherwise points every worker at frontend 0, which would have left that scenario asserting on two sequential writes through one process. The default is unchanged because the baseline specs depend on it. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/cluster.go | 52 +++- .../e2e/distributed/cluster_baseline_test.go | 15 +- .../e2e/distributed/cluster_failover_test.go | 280 ++++++++++++++++++ 3 files changed, 335 insertions(+), 12 deletions(-) create mode 100644 tests/e2e/distributed/cluster_failover_test.go diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go index 0077c21cefe6..198ff5cae735 100644 --- a/tests/e2e/distributed/cluster/cluster.go +++ b/tests/e2e/distributed/cluster/cluster.go @@ -40,6 +40,17 @@ type Options struct { Frontends int Workers int + + // SpreadWorkerRegistrations sends worker i to frontend i%Frontends instead + // of sending every worker to frontend 0. + // + // Off by default, and deliberately so: the cross-replica session specs read + // a node at frontend 1 that only frontend 0 was ever told about, and + // spreading registrations would leave them passing while proving nothing. + // It exists for the racing-replicas spec, which is about two replicas + // writing to one roster concurrently and cannot express that at all while + // every worker registers through the same process. + SpreadWorkerRegistrations bool } // Process is one running local-ai. @@ -248,19 +259,25 @@ func (c *Cluster) startWorker(i int) (*Process, error) { fmt.Sprintf("LOCALAI_ADVERTISE_ADDR=127.0.0.1:%d", grpcPort), fmt.Sprintf("LOCALAI_HTTP_ADDR=127.0.0.1:%d", httpPort), fmt.Sprintf("LOCALAI_ADVERTISE_HTTP_ADDR=127.0.0.1:%d", httpPort), - // Workers register with frontend 0 ONLY, never with every replica, and - // the cross-replica session specs depend on that. They prove a session - // minted at frontend 0 resolves at frontend 1 by reading a node that - // only frontend 0 was ever told about; register the worker everywhere - // and they still pass while proving nothing. + // Workers register with frontend 0 ONLY unless the caller opts into + // SpreadWorkerRegistrations, and the cross-replica session specs depend + // on that default. They prove a session minted at frontend 0 resolves at + // frontend 1 by reading a node that only frontend 0 was ever told about; + // register the worker everywhere and they still pass while proving + // nothing. // // Nothing in those specs can detect the change. The registry keys nodes // by name and preserves ids across the shared Postgres // (core/services/nodes/registry.go:522-527), so a roster read at - // frontend 1 looks identical either way. Anyone pointing workers at - // more than one replica must revisit - // tests/e2e/distributed/cluster_baseline_test.go by hand. - "LOCALAI_REGISTER_TO="+c.FrontendURL(0), + // frontend 1 looks identical either way. Anyone changing the default + // here must revisit tests/e2e/distributed/cluster_baseline_test.go by + // hand. + // + // The registrar also fixes where this worker's heartbeats go for the + // rest of its life: the loop posts to the URL it was given at boot and + // never re-resolves it (core/cli/workerregistry/client.go), so killing a + // worker's registrar orphans that worker rather than failing it over. + "LOCALAI_REGISTER_TO="+c.FrontendURL(c.registrarFor(i)), "LOCALAI_NODE_NAME="+name, "LOCALAI_REGISTRATION_TOKEN="+c.opts.RegistrationToken, "LOCALAI_NATS_URL="+c.opts.NatsURL, @@ -322,6 +339,23 @@ func (c *Cluster) WorkerName(i int) string { return c.workers[i].Name } +// registrarFor is the frontend index worker i registers and heartbeats with. +// +// It is read at spawn time and baked into the worker's environment, so it is +// also the answer to "which replica's death orphans this worker". +func (c *Cluster) registrarFor(worker int) int { + if !c.opts.SpreadWorkerRegistrations || c.opts.Frontends < 1 { + return 0 + } + return worker % c.opts.Frontends +} + +// WorkerRegistrar is registrarFor, exported so a spec can say which replica it +// is about to kill relative to a worker instead of re-deriving the rule. +func (c *Cluster) WorkerRegistrar(worker int) int { + return c.registrarFor(worker) +} + // Stop terminates every process and removes the work directory. Logs survive in // LogDir, which the caller owns. func (c *Cluster) Stop() { diff --git a/tests/e2e/distributed/cluster_baseline_test.go b/tests/e2e/distributed/cluster_baseline_test.go index 662f93b38835..6cbaaa0ea915 100644 --- a/tests/e2e/distributed/cluster_baseline_test.go +++ b/tests/e2e/distributed/cluster_baseline_test.go @@ -116,7 +116,11 @@ func mockBackendBinary() string { // startCluster brings up a cluster against a freshly provisioned database and // registers cleanup, including a log dump on failure. -func startCluster(frontends, workers int) *cluster.Cluster { +// +// customise runs against the assembled Options immediately before Start, for +// the one spec that needs a non-default topology. It is variadic so every +// existing caller keeps the plain two-argument form and the default shape. +func startCluster(frontends, workers int, customise ...func(*cluster.Options)) *cluster.Cluster { GinkgoHelper() // Resolved before SetupInfra so a missing binary skips without having paid @@ -137,7 +141,7 @@ func startCluster(frontends, workers int) *cluster.Cluster { Expect(os.MkdirAll(logDir, 0o755)).To(Succeed()) } - c, err := cluster.Start(cluster.Options{ + options := cluster.Options{ Binary: binary, MockBackend: mockBackend, PGDSN: infra.PGURL, @@ -145,7 +149,12 @@ func startCluster(frontends, workers int) *cluster.Cluster { LogDir: logDir, Frontends: frontends, Workers: workers, - }) + } + for _, apply := range customise { + apply(&options) + } + + c, err := cluster.Start(options) Expect(err).ToNot(HaveOccurred()) DeferCleanup(func() { diff --git a/tests/e2e/distributed/cluster_failover_test.go b/tests/e2e/distributed/cluster_failover_test.go new file mode 100644 index 000000000000..d854baa66c7c --- /dev/null +++ b/tests/e2e/distributed/cluster_failover_test.go @@ -0,0 +1,280 @@ +package distributed_test + +import ( + "fmt" + + "github.com/mudler/LocalAI/core/services/nodes" + "github.com/mudler/LocalAI/tests/e2e/distributed/cluster" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// The two sentinels below are returned by statusOf in place of a node status. +// Neither can ever equal one of the nodes.Status* constants, which is the whole +// point: every assertion in this file compares against a real status with +// Equal, so an unreachable frontend or a vanished row fails the assertion and +// names itself rather than quietly satisfying it. +// +// This is not hypothetical. The obvious way to write "the dead worker is gone" +// is ShouldNot(ContainElement(name)) over a list of healthy names, and the +// probe returns an empty list on any error, so an expired session, a 401 at the +// second replica or a decode failure all satisfy that matcher. The spec would +// go green having observed nothing at all. +const ( + statusUnreachable = "" + statusAbsent = "" +) + +const ( + // orphanEvictionWindow is how long a spec watches a roster to be sure the + // system had a real chance to evict a node and chose not to. + // + // It is sized from the only eviction path there is. Node liveness is + // heartbeat freshness: the health monitor wakes every HealthCheckInterval + // (15s) and marks any node whose last heartbeat is older than + // StaleNodeThreshold (60s) offline (core/services/nodes/health.go, defaults + // in core/config/distributed_config.go). Neither is settable from the CLI, + // so 60s + one 15s tick = 75s is the worst case and cannot be shortened. + // + // Measured rather than assumed: a worker whose registrar was killed goes + // offline at both surviving replicas at t=74.2s. A window shorter than that + // would be the classic false green, a Consistently that passes because + // nothing has had time to happen yet. 100s clears the measured latency by a + // third. + orphanEvictionWindow = "100s" + rosterPollInterval = "2s" + + // workerDeathTimeout bounds the wait for a killed worker to be marked + // offline. Roughly twice the measured 74.3s, which absorbs a health tick + // landing just before the kill plus a slow CI runner. + workerDeathTimeout = "150s" + + // settledStatusWindow is how long an observed status has to hold before the + // spec believes it. A killed worker does not go straight to offline: it + // flaps to unhealthy at ~8s and back to healthy at ~14s (see the note in + // the worker-death spec), so a status has to outlast that transient and two + // further 15s health ticks to count as the settled state. + settledStatusWindow = "45s" + + // restartRehydrationTimeout bounds the wait for a cold-restarted replica to + // answer with the roster. The restart itself measured 1.0s; the budget is + // for a loaded CI runner, not for a slow code path. + restartRehydrationTimeout = "60s" + + // frontendExitTimeout bounds the wait for a signalled frontend to be + // collected. SIGTERM measured 0.2s. It is polled rather than sampled + // because FrontendAlive reports true for the zombie window between the + // child exiting and the reaper calling waitid. + frontendExitTimeout = "30s" + frontendExitPoll = "200ms" +) + +// statusOf refreshes the roster at the probe's frontend and returns the status +// that frontend reports for name. +// +// It shares rosterProbe's lastErr/lastSeen so describe() still explains a +// failure, but unlike healthyNames it never collapses an error into an empty +// result: the caller is comparing against an exact status, so an error has to +// be a value that no assertion can accept. +func (p *rosterProbe) statusOf(name string) string { + var roster []node + if err := p.cluster.GetJSON(p.client, p.frontend, "/api/nodes", &roster); err != nil { + p.lastErr = err + return statusUnreachable + } + p.lastErr = nil + p.lastSeen = roster + for _, n := range roster { + if n.Name == name { + return n.Status + } + } + return statusAbsent +} + +// explain builds a lazy failure description. +// +// Gomega formats a (string, args...) description as soon as the assertion is +// constructed, which for an Eventually or a Consistently is before anything has +// gone wrong; the roster it quoted would be the one from before the wait. A +// func() string is called only on failure, so describe() reports the last +// observation the assertion actually made. +func (p *rosterProbe) explain(format string, args ...any) func() string { + return func() string { + return fmt.Sprintf(format, args...) + ": " + p.describe() + } +} + +var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), func() { + It("keeps a healthy worker in the roster when a peer replica dies", func() { + // Two replicas, one worker. The worker registers and heartbeats with + // frontend 0 only (the harness default), so frontend 1 is a replica it + // has never spoken to. + c := startCluster(2, 1) + worker := c.WorkerName(0) + + // One session for the whole cluster: register/login/token-login/password + // share a five-per-minute-per-IP budget at every frontend + // (core/http/routes/auth.go:190) and everything here comes from + // 127.0.0.1. The cookie is valid at both replicas because sessions live + // in the shared Postgres and the harness pins one HMAC secret. + client, err := c.AdminSession(0) + Expect(err).ToNot(HaveOccurred()) + + survivor := newRosterProbe(c, client, 0) + Eventually(survivor.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ContainElement(worker), survivor.describe) + + // Kill the replica this worker never registered with. That choice is + // what makes the assertion below mean anything. + // + // Killing frontend 0 instead would sever the worker's only heartbeat + // path, because the heartbeat loop posts to the URL it was handed at + // boot and never re-resolves it; the worker is then genuinely orphaned + // and IS evicted, at a measured 74.2s. A spec written that way can only + // pass by watching for less time than the eviction takes. + Expect(c.WorkerRegistrar(0)).ToNot(Equal(1), + "this spec kills frontend 1 precisely because worker 0 does not depend on it") + Expect(c.KillFrontend(1)).To(Succeed()) + Eventually(func() bool { return c.FrontendAlive(1) }, frontendExitTimeout, frontendExitPoll). + Should(BeFalse(), "frontend 1 did not die, so nothing below is a failover assertion") + + // The survivor must keep answering, and must keep the worker healthy. + // + // A GET that returns 200 with a decodable roster is the "keeps serving" + // half; statusUnreachable would fail this matcher. The window outlasts + // the full 75s stale-plus-one-tick eviction path, so an implementation + // that reacted to a dead peer by sweeping its nodes, by resetting + // heartbeats, or by marking the whole roster stale would be caught + // whether it reacted immediately or on a health tick. + Consistently(survivor.statusOf, orphanEvictionWindow, rosterPollInterval). + WithArguments(worker). + Should(Equal(nodes.StatusHealthy), + survivor.explain("killing a peer replica must not disturb a worker that never depended on it")) + }) + + It("rediscovers the worker from shared state after a cold rolling restart", func() { + c := startCluster(2, 1) + worker := c.WorkerName(0) + + client, err := c.AdminSession(0) + Expect(err).ToNot(HaveOccurred()) + + probe := newRosterProbe(c, client, 0) + Eventually(probe.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ContainElement(worker), probe.describe) + registeredID := probe.idOf(worker) + Expect(registeredID).ToNot(BeEmpty(), "frontend 0 reported the worker without a registration ID") + + // The rolling-update shape: drain, wait for the process to actually go, + // then bring the replacement up. Restarting without that wait would + // SIGKILL the replica mid-drain and silently turn this into the crash + // case. + Expect(c.StopFrontendGracefully(0)).To(Succeed()) + Eventually(func() bool { return c.FrontendAlive(0) }, frontendExitTimeout, frontendExitPoll). + Should(BeFalse(), "frontend 0 ignored SIGTERM, so the restart below would be a SIGKILL mid-drain") + + // RestartFrontend wipes the replica's data directory, so the process + // that comes back has no local memory of the cluster. Everything the + // assertions below observe has to come out of the shared Postgres. + Expect(c.RestartFrontend(0)).To(Succeed()) + + restarted := newRosterProbe(c, client, 0) + Eventually(restarted.statusOf, restartRehydrationTimeout, nodeRosterPoll). + WithArguments(worker). + Should(Equal(nodes.StatusHealthy), + restarted.explain("a replica with an empty data directory must rehydrate the roster from shared state")) + Expect(restarted.idOf(worker)).To(Equal(registeredID), + "the restarted replica invented a new row for the worker instead of resolving the shared one") + + // Rehydration alone is a weak claim: the row was written before the + // restart and would still read healthy for up to 75s even if the + // replacement never accepted another heartbeat. Holding it past that + // window is what proves the worker's heartbeats are landing again, + // which is the part a restart can plausibly break (a replacement on a + // different port, or one that rejects the node id it did not issue). + Consistently(restarted.statusOf, orphanEvictionWindow, rosterPollInterval). + WithArguments(worker). + Should(Equal(nodes.StatusHealthy), + restarted.explain("the worker went stale after the restart, so its heartbeats are not reaching the replacement")) + }) + + It("settles a dead worker to offline on every replica", func() { + c := startCluster(2, 1) + worker := c.WorkerName(0) + + client, err := c.AdminSession(0) + Expect(err).ToNot(HaveOccurred()) + + at0 := newRosterProbe(c, client, 0) + at1 := newRosterProbe(c, client, 1) + Eventually(at0.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ContainElement(worker), at0.describe) + + Expect(c.KillWorker(0)).To(Succeed()) + + // Assert the settled status, not the absence of a healthy name. + // + // A killed worker does not move monotonically. Measured: healthy until + // ~8s, unhealthy at 8s, healthy again at 14s, offline from 74s. The + // unhealthy blip comes from a liveness probe; the health monitor's + // "heartbeat is still fresh" branch then marks it healthy again + // (core/services/nodes/health.go), and only the stale-heartbeat branch + // reaches MarkOffline. So requiring exactly offline is what pins this to + // the stale-detection path rather than to the transient, which any + // not-healthy or not-present matcher would accept at t=8s. + for _, probe := range []*rosterProbe{at0, at1} { + Eventually(probe.statusOf, workerDeathTimeout, rosterPollInterval). + WithArguments(worker). + Should(Equal(nodes.StatusOffline), + probe.explain("frontend %d never settled the dead worker to offline", probe.frontend)) + } + + // And it has to stay offline. Nothing may resurrect a row for a process + // that no longer exists, and this window covers three health ticks. + for _, probe := range []*rosterProbe{at0, at1} { + Consistently(probe.statusOf, settledStatusWindow, rosterPollInterval). + WithArguments(worker). + Should(Equal(nodes.StatusOffline), + probe.explain("frontend %d flipped the dead worker away from offline", probe.frontend)) + } + }) + + It("converges on one roster when two replicas register a worker each", func() { + // SpreadWorkerRegistrations is what makes this a race between replicas + // rather than two sequential writes through one: worker 0 registers with + // frontend 0 and worker 1 with frontend 1, both during Start, so two + // processes insert into the shared roster at the same moment. + c := startCluster(2, 2, func(o *cluster.Options) { + o.SpreadWorkerRegistrations = true + }) + Expect(c.WorkerRegistrar(0)).ToNot(Equal(c.WorkerRegistrar(1)), + "both workers registered through the same replica, so this spec is not testing a race") + + client, err := c.AdminSession(0) + Expect(err).ToNot(HaveOccurred()) + + at0 := newRosterProbe(c, client, 0) + at1 := newRosterProbe(c, client, 1) + expected := []string{c.WorkerName(0), c.WorkerName(1)} + + // ConsistOf, not ContainElements: it fails on a third entry, which is + // how a registration that raced into two rows would show up. + Eventually(at0.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ConsistOf(expected), at0.describe) + Eventually(at1.healthyNames, nodeRosterTimeout, nodeRosterPoll). + Should(ConsistOf(expected), at1.describe) + + // Same names is not the same roster. Compare the registration ids, which + // is the only way to tell "both replicas read one set of rows" from + // "each replica has its own row per worker that happens to share a + // name". + for _, name := range expected { + id := at0.idOf(name) + Expect(id).ToNot(BeEmpty(), fmt.Sprintf("frontend 0 reported %s without a registration ID", name)) + Expect(at1.idOf(name)).To(Equal(id), + fmt.Sprintf("the replicas disagree on the identity of %s, so they are not sharing one roster", name)) + } + }) +}) From 58232a3f0426d4ebf9396e99391d3d44723c479e Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 19:40:01 +0000 Subject: [PATCH 17/23] test(distributed): prove health checking was alive during the failover windows The two specs that assert a healthy worker stays healthy were pure negatives: they say nothing happened. A cluster whose health checking had wedged, by leaking the advisory lock the monitor takes at health.go:110, would freeze the roster and satisfy both while observing a corpse. Kill the worker once the window closes and require the roster to settle it to offline, so the preceding Consistently is a statement about behaviour rather than about a stopped clock. Applied to the cold-restart spec as well as the peer-death one: a restart is exactly the event that could leave a replacement unable to check anything. Document the hazard that can make an offline assertion hang. The staleness branch skips a node already marked unhealthy (health.go:153-155), a skip meant for nodes an operator took down, which also swallows the flap: an unhealthy mark landing after the heartbeat goes stale means MarkOffline is never called and the node stays unhealthy forever. Name the file and line at the assertion, and have the failure message say so when the roster shows a node stuck there, so a timeout sends the reader to LocalAI rather than to the harness. Stop calling the two-replica registration spec a race. Start spawns workers sequentially and the registrations land about a second apart; it is a shared-roster identity test, and saying otherwise invites someone to trust it for something it does not check. WorkerRegistrar now bound-checks its index like every other index-taking method here. It answered 0 for an out-of-range worker, and 0 is a real frontend index, so the failure mode was a spec killing the wrong replica. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- tests/e2e/distributed/cluster/cluster.go | 14 ++- tests/e2e/distributed/cluster/failure.go | 13 ++- .../e2e/distributed/cluster_failover_test.go | 102 ++++++++++++++++-- 3 files changed, 117 insertions(+), 12 deletions(-) diff --git a/tests/e2e/distributed/cluster/cluster.go b/tests/e2e/distributed/cluster/cluster.go index 198ff5cae735..1783d71e16f0 100644 --- a/tests/e2e/distributed/cluster/cluster.go +++ b/tests/e2e/distributed/cluster/cluster.go @@ -352,8 +352,18 @@ func (c *Cluster) registrarFor(worker int) int { // WorkerRegistrar is registrarFor, exported so a spec can say which replica it // is about to kill relative to a worker instead of re-deriving the rule. -func (c *Cluster) WorkerRegistrar(worker int) int { - return c.registrarFor(worker) +// +// It returns an error rather than indexing blindly, like every other exported +// method here that takes an index. Gomega treats the trailing error as one that +// must be nil, so Expect(c.WorkerRegistrar(0)).To(...) reads unchanged at the +// call site while an out-of-range index fails the spec by name instead of +// silently answering 0, which is a real frontend index and would send a spec +// off to kill the wrong replica. +func (c *Cluster) WorkerRegistrar(worker int) (int, error) { + if err := c.checkWorkerIndex(worker); err != nil { + return 0, err + } + return c.registrarFor(worker), nil } // Stop terminates every process and removes the work directory. Logs survive in diff --git a/tests/e2e/distributed/cluster/failure.go b/tests/e2e/distributed/cluster/failure.go index 3f2a1f42ba28..919376f4a550 100644 --- a/tests/e2e/distributed/cluster/failure.go +++ b/tests/e2e/distributed/cluster/failure.go @@ -33,8 +33,8 @@ func (c *Cluster) StopFrontendGracefully(i int) error { // KillWorker SIGKILLs worker i. func (c *Cluster) KillWorker(i int) error { - if i < 0 || i >= len(c.workers) { - return fmt.Errorf("worker %d out of range (cluster has %d)", i, len(c.workers)) + if err := c.checkWorkerIndex(i); err != nil { + return err } return signalProcess(c.workers[i], syscall.SIGKILL) } @@ -160,6 +160,15 @@ func (c *Cluster) checkFrontendIndex(i int) error { return nil } +// checkWorkerIndex is checkFrontendIndex for workers, and exists for the same +// reason: one wording, whichever primitive tripped. +func (c *Cluster) checkWorkerIndex(i int) error { + if i < 0 || i >= len(c.workers) { + return fmt.Errorf("worker %d out of range (cluster has %d)", i, len(c.workers)) + } + return nil +} + func frontendName(i int) string { return fmt.Sprintf("frontend-%d", i) } diff --git a/tests/e2e/distributed/cluster_failover_test.go b/tests/e2e/distributed/cluster_failover_test.go index d854baa66c7c..3e980cb3de75 100644 --- a/tests/e2e/distributed/cluster_failover_test.go +++ b/tests/e2e/distributed/cluster_failover_test.go @@ -106,6 +106,54 @@ func (p *rosterProbe) explain(format string, args ...any) func() string { } } +// explainStuckOffline is explain with one extra diagnosis attached. +// +// An assertion waiting for offline has a failure mode that looks like a harness +// bug and is not one, so the message names it rather than leaving the reader to +// find it. See proveHealthCheckingIsAlive and the comment on the dead-worker +// spec for the mechanism. +func (p *rosterProbe) explainStuckOffline(worker, format string, args ...any) func() string { + return func() string { + message := fmt.Sprintf(format, args...) + ": " + p.describe() + for _, n := range p.lastSeen { + if n.Name != worker || n.Status != nodes.StatusUnhealthy { + continue + } + message += "\n\nThe node is stuck at unhealthy, which is a LocalAI defect rather than " + + "a harness one: core/services/nodes/health.go:153-155 skips MarkOffline for a node " + + "already marked unhealthy, so a node whose unhealthy mark lands after its heartbeat " + + "has gone stale never reaches offline at all. Start there, not here." + } + return message + } +} + +// proveHealthCheckingIsAlive kills a worker and waits for the roster to settle +// it to offline. +// +// It is the terminating positive control for the two specs that assert a +// healthy worker STAYS healthy. On their own those are pure negative +// assertions: a cluster whose health checking had wedged entirely, say by +// leaking the Postgres advisory lock the monitor takes (health.go:110), would +// freeze the roster and satisfy them while observing a corpse. Killing a worker +// afterwards and requiring the roster to react proves the monitor was running +// for the whole window, which is the only thing that makes the preceding +// Consistently a statement about behaviour rather than about a stopped clock. +// +// It costs a full detection cycle, which is why it is a shared helper: the +// wall-clock price should be paid once per spec and explained once. +func proveHealthCheckingIsAlive(c *cluster.Cluster, probe *rosterProbe, workerIndex int) { + GinkgoHelper() + worker := c.WorkerName(workerIndex) + Expect(c.KillWorker(workerIndex)).To(Succeed()) + Eventually(probe.statusOf, workerDeathTimeout, rosterPollInterval). + WithArguments(worker). + Should(Equal(nodes.StatusOffline), + probe.explainStuckOffline(worker, + "frontend %d never reacted to a killed worker, so health checking was not running during the window above and the assertion before this one proved nothing", + probe.frontend)) +} + var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), func() { It("keeps a healthy worker in the roster when a peer replica dies", func() { // Two replicas, one worker. The worker registers and heartbeats with @@ -152,6 +200,10 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun WithArguments(worker). Should(Equal(nodes.StatusHealthy), survivor.explain("killing a peer replica must not disturb a worker that never depended on it")) + + // Everything above is a negative: nothing happened. Prove that the + // survivor was capable of making something happen the whole time. + proveHealthCheckingIsAlive(c, survivor, 0) }) It("rediscovers the worker from shared state after a cold rolling restart", func() { @@ -198,6 +250,12 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun WithArguments(worker). Should(Equal(nodes.StatusHealthy), restarted.explain("the worker went stale after the restart, so its heartbeats are not reaching the replacement")) + + // Same hole as the peer-death spec, and it is worse here: a cold + // restart is exactly the event that could leave a replacement unable to + // run health checks at all, and a frozen roster reads identically to a + // healthy one. This is the assertion that tells the two apart. + proveHealthCheckingIsAlive(c, restarted, 0) }) It("settles a dead worker to offline on every replica", func() { @@ -224,11 +282,26 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun // reaches MarkOffline. So requiring exactly offline is what pins this to // the stale-detection path rather than to the transient, which any // not-healthy or not-present matcher would accept at t=8s. + // + // KNOWN HAZARD, read this before blaming the harness for a timeout here. + // The staleness branch skips a node that is already unhealthy + // (core/services/nodes/health.go:153-155, `if node.Status == + // StatusOffline || node.Status == StatusUnhealthy { continue }`). The + // skip exists to stop the monitor re-logging nodes an operator took + // down, but it applies to the flap too: if the transient unhealthy mark + // lands AFTER the heartbeat has already gone stale, rather than at the + // ~8s observed here, MarkOffline is never called and this node stays + // unhealthy forever. This spec would then hang to workerDeathTimeout + // and fail with a roster that looks perfectly ordinary. The ordering + // that triggers it did not occur in any run so far, but nothing + // prevents it, so explainStuckOffline says so in the failure message + // when it sees a node stuck at unhealthy. Fixing it is LocalAI work, + // not test work. for _, probe := range []*rosterProbe{at0, at1} { Eventually(probe.statusOf, workerDeathTimeout, rosterPollInterval). WithArguments(worker). Should(Equal(nodes.StatusOffline), - probe.explain("frontend %d never settled the dead worker to offline", probe.frontend)) + probe.explainStuckOffline(worker, "frontend %d never settled the dead worker to offline", probe.frontend)) } // And it has to stay offline. Nothing may resurrect a row for a process @@ -242,15 +315,28 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun }) It("converges on one roster when two replicas register a worker each", func() { - // SpreadWorkerRegistrations is what makes this a race between replicas - // rather than two sequential writes through one: worker 0 registers with - // frontend 0 and worker 1 with frontend 1, both during Start, so two - // processes insert into the shared roster at the same moment. + // SpreadWorkerRegistrations sends worker 0 to frontend 0 and worker 1 to + // frontend 1, so the roster is written through two different replicas. + // + // This is a shared-roster identity test, NOT a concurrency test, and the + // distinction matters because the obvious reading of the spec name is + // the wrong one. Start spawns workers one after another and waits for + // neither, and the registrations land about a second apart in practice; + // there is no synchronisation point and nothing here is tuned to make + // the two writes collide. What it does establish is that a roster + // written through two replicas is one roster and not two: same rows, + // same identities, read back from either process. A genuine concurrent + // registration test would need workers released together against a + // shared barrier, and does not exist yet. c := startCluster(2, 2, func(o *cluster.Options) { o.SpreadWorkerRegistrations = true }) - Expect(c.WorkerRegistrar(0)).ToNot(Equal(c.WorkerRegistrar(1)), - "both workers registered through the same replica, so this spec is not testing a race") + registrar0, err := c.WorkerRegistrar(0) + Expect(err).ToNot(HaveOccurred()) + registrar1, err := c.WorkerRegistrar(1) + Expect(err).ToNot(HaveOccurred()) + Expect(registrar0).ToNot(Equal(registrar1), + "both workers registered through the same replica, so nothing below says anything about two replicas sharing a roster") client, err := c.AdminSession(0) Expect(err).ToNot(HaveOccurred()) @@ -260,7 +346,7 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun expected := []string{c.WorkerName(0), c.WorkerName(1)} // ConsistOf, not ContainElements: it fails on a third entry, which is - // how a registration that raced into two rows would show up. + // how a duplicated row for one worker would show up. Eventually(at0.healthyNames, nodeRosterTimeout, nodeRosterPoll). Should(ConsistOf(expected), at0.describe) Eventually(at1.healthyNames, nodeRosterTimeout, nodeRosterPoll). From 6620aa0937b9fda470f65b87b29e096f99069475 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 19:49:19 +0000 Subject: [PATCH 18/23] ci(distributed): run the process-level cluster suite Add test-e2e-cluster and a second CI job that runs it. The cluster specs spawn local-ai as real child processes and kill them, so they need a built binary; keeping them in their own job means the fast in-process suite is not held behind that build. The binary is built with a stubbed core/http/react-ui/dist. A single index.html satisfies the go:embed in core/http/app.go, and this suite drives the HTTP API only, so the job skips a Node and Vite install entirely. The job runs serial and pins --flake-attempts 1. Each Ginkgo process would otherwise get its own PostgreSQL and NATS container while every spec spawns two or three children, and a retry would hide exactly the nondeterminism the suite exists to catch. Measured at 8m39s over three runs, hence a 25 minute job timeout and a 20 minute Ginkgo timeout. LOCALAI_E2E_LOG_DIR points inside the workspace so the per-process logs upload as an artifact on failure; they are the only way to read a cluster failure. LOCALAI_E2E_REQUIRE_BINARIES is set explicitly even though CI already implies it, because a skipped cluster spec is indistinguishable from a passing one and this job's whole value is that it cannot go green without starting a cluster. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .github/workflows/tests-e2e-distributed.yml | 102 ++++++++++++++++++++ Makefile | 23 +++++ 2 files changed, 125 insertions(+) diff --git a/.github/workflows/tests-e2e-distributed.yml b/.github/workflows/tests-e2e-distributed.yml index 4b41f72b5014..3cb4268063d8 100644 --- a/.github/workflows/tests-e2e-distributed.yml +++ b/.github/workflows/tests-e2e-distributed.yml @@ -79,3 +79,105 @@ jobs: detached: true connect-timeout-seconds: 180 limit-access-to-actor: true + + tests-e2e-cluster: + runs-on: ubuntu-latest + # Advisory for the same reason as the job above: master has no branch + # protection, so a failure here is a visible red X rather than a blocked + # merge. That is a repository-settings property, not a YAML key. The key + # that looks like it says "advisory" instead flips the run's conclusion to + # success, which hides the failure rather than flagging it, so it appears in + # none of this repo's workflows and must not be added here. + # + # Separate job from tests-e2e-distributed so the fast in-process suite is + # not held behind a Go build of local-ai. Serial on purpose: each Ginkgo + # process would get its own PostgreSQL and NATS container and each spec + # spawns two or three local-ai children, so --procs on an unmeasured runner + # is a change to make with numbers, not by default. + # + # Measured at 8m39s over three consecutive runs (509.1s / 509.8s / 512.3s). + # Three specs sit at ~167s each because they wait out a 60s staleness + # threshold plus a 15s reconcile tick. Do not shorten those windows to make + # this job faster: the wait is what stops the assertions from passing before + # the system could have reacted, which was a real false green earlier on. + timeout-minutes: 25 + steps: + - name: Clone + uses: actions/checkout@v7 + with: + submodules: true + - name: Configure apt mirror on runner + uses: ./.github/actions/configure-apt-mirror + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version: '1.26.0' + cache: false + - name: Dependencies + run: | + sudo apt-get update + sudo apt-get install -y build-essential libopus-dev + - name: Proto Dependencies + run: | + curl -L -s https://github.com/protocolbuffers/protobuf/releases/download/v26.1/protoc-26.1-linux-x86_64.zip -o protoc.zip && \ + unzip -j -d /usr/local/bin protoc.zip bin/protoc && \ + rm protoc.zip + go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.34.2 + go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@1958fcbe2ca8bd93af633f11e97d44e567e945af + PATH="$PATH:$HOME/go/bin" make protogen-go + - name: Stub the embedded React UI + # core/http/react-ui/dist is gitignored and built by Node, but this + # suite drives the HTTP API and never the UI, which has its own e2e + # suite. A single index.html satisfies the //go:embed react-ui/dist/* + # in core/http/app.go, so the job skips a full Node and Vite install. + run: | + mkdir -p core/http/react-ui/dist + printf 'stub\n' > core/http/react-ui/dist/index.html + - name: Build local-ai + # Not `make build`: that target pulls in the React UI build. The specs + # exec this binary directly via LOCALAI_E2E_BINARY. + run: | + PATH="$PATH:$HOME/go/bin" go build -o local-ai ./cmd/local-ai + - name: Pre-pull test images + # Same reasoning as the job above: pulling here keeps container-start + # timing out of the spec timeouts and makes a registry outage read as a + # setup failure rather than a test failure. + run: | + docker pull postgres:16-alpine + docker pull nats:2-alpine + - name: Cluster E2E + env: + LOCALAI_E2E_BINARY: ${{ github.workspace }}/local-ai + # Must live under the workspace so the upload step below can reach it. + # The harness defaults to GinkgoT().TempDir(), which lands under + # TMPDIR and would leave the artifact glob matching nothing. + LOCALAI_E2E_LOG_DIR: ${{ github.workspace }}/cluster-logs + # Belt and braces: the harness already fails rather than skips when CI + # is set, and GitHub Actions always sets CI. Stating it here means a + # future edit to that default cannot silently turn this job into one + # that passes without ever starting a cluster, since a skipped cluster + # spec is indistinguishable from a passing one. + LOCALAI_E2E_REQUIRE_BINARIES: "true" + # See the job above: the runner is ephemeral, so the reaper buys + # nothing and would pull a third, unpinned Docker Hub image mid-suite. + TESTCONTAINERS_RYUK_DISABLED: "true" + run: | + PATH="$PATH:$HOME/go/bin" make test-e2e-cluster + - name: Upload process logs + # The per-process logs are the only way to read a cluster failure: the + # Ginkgo output says which assertion failed, not what the four child + # processes were doing. Without this a red job is undebuggable. + if: ${{ failure() }} + uses: actions/upload-artifact@v4 + with: + name: cluster-process-logs + path: cluster-logs/**/*.log + if-no-files-found: ignore + retention-days: 7 + - name: Setup tmate session if tests fail + if: ${{ failure() }} + uses: mxschmitt/action-tmate@v3.23 + with: + detached: true + connect-timeout-seconds: 180 + limit-access-to-actor: true diff --git a/Makefile b/Makefile index 3df884de87bf..b3252dba9f9e 100644 --- a/Makefile +++ b/Makefile @@ -349,10 +349,33 @@ DISTRIBUTED_TEST_FLAKES?=1 # Distributed architecture e2e (PostgreSQL + NATS via testcontainers). # Includes NatsJWT specs (JWT-enabled NATS). Requires Docker. # VLLMMultinode is excluded here; use test-e2e-vllm-multinode for that. +# Cluster is excluded too and runs in test-e2e-cluster below, which needs a +# built binary. The argument-validation specs under tests/e2e/distributed/cluster +# carry Label("Distributed") only, so they run here and not there, on purpose. test-e2e-distributed: protogen-go @echo 'Running distributed e2e tests (label Distributed, incl. NatsJWT)' $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Distributed && !VLLMMultinode && !Cluster' --flake-attempts $(DISTRIBUTED_TEST_FLAKES) --timeout=40m -v -r ./tests/e2e/distributed +# Cluster e2e: runs local-ai as real child processes (frontend replicas + +# workers) against PostgreSQL and NATS, and kills them to assert failover. +# Needs a built ./local-ai (or LOCALAI_E2E_BINARY) plus the mock backend. +# +# The label split is deliberate and asymmetric. --label-filter='Cluster' +# selects only the specs that spawn processes: the baseline and failover specs +# in tests/e2e/distributed. The argument-validation specs in +# tests/e2e/distributed/cluster carry Label("Distributed") alone, so they stay +# in test-e2e-distributed above, where they belong: they exercise the harness's +# own option handling, need no binary, no PostgreSQL and no NATS, and run in +# milliseconds. Ginkgo therefore reports 0 of 8 specs run for that package here. +# That zero is correct, not a filter bug. +# +# --flake-attempts is pinned to 1 rather than $(DISTRIBUTED_TEST_FLAKES), and +# should stay there: this suite exists to catch nondeterministic cluster +# behaviour, and a retry turns exactly that signal into a green run. +test-e2e-cluster: protogen-go build-mock-backend + @echo 'Running cluster e2e tests (label Cluster, real local-ai processes)' + $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Cluster' --flake-attempts 1 --timeout=20m -v -r ./tests/e2e/distributed + # vLLM multi-node DP smoke (CPU). Builds local-ai:tests and the # cpu-vllm backend from the current working tree, then drives a # head + headless follower via testcontainers-go and asserts a chat From 73e7ef85b65a3b5db5ab3807a2a802f11ae19a01 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 20:00:14 +0000 Subject: [PATCH 19/23] ci(distributed): fail the cluster job when it selects no specs Ginkgo exits 0 when a label filter matches nothing, so a refactor that renamed or dropped Label("Cluster") would have left the job reporting "Test Suite Passed" having started no cluster. LOCALAI_E2E_REQUIRE_BINARIES does not cover that case: it only fires inside a spec that is already running. Add --fail-on-empty to both distributed targets. Drop -r from test-e2e-cluster while here. All six Cluster specs live in the top-level package, and the cluster subpackage contributes nothing under this filter by design, so recursing only widened the blast radius. test-e2e- distributed keeps -r: it must reach the eight argument-validation specs in that subpackage. Raise the cluster job to 45 minutes, matching its sibling. The 20 minute Ginkgo timeout bounds the suite alone; the job timeout must also cover setup, which is the larger and more variable half here: cold-cache module download, protoc and protogen-go, a full build of ./cmd/local-ai and a separate test compile, realistically 8-12 minutes on a 4-vCPU runner. At 25 minutes the runner would have hard-killed the job before Ginkgo could report which spec hung, which is the red-with-no-evidence outcome that gets suites disabled. Also move upload-artifact to @v7 with the rest of the repo, and note on the react-ui stub step that it must go if a spec ever asserts on a UI asset, since a developer box has a real dist/ and would not catch that locally. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .github/workflows/tests-e2e-distributed.yml | 32 ++++++++++++++++----- Makefile | 25 +++++++++------- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/.github/workflows/tests-e2e-distributed.yml b/.github/workflows/tests-e2e-distributed.yml index 3cb4268063d8..c69ae7924553 100644 --- a/.github/workflows/tests-e2e-distributed.yml +++ b/.github/workflows/tests-e2e-distributed.yml @@ -95,12 +95,26 @@ jobs: # spawns two or three local-ai children, so --procs on an unmeasured runner # is a change to make with numbers, not by default. # - # Measured at 8m39s over three consecutive runs (509.1s / 509.8s / 512.3s). - # Three specs sit at ~167s each because they wait out a 60s staleness - # threshold plus a 15s reconcile tick. Do not shorten those windows to make - # this job faster: the wait is what stops the assertions from passing before - # the system could have reacted, which was a real false green earlier on. - timeout-minutes: 25 + # The two timeouts bound different things and are not alternatives. Ginkgo's + # --timeout=20m bounds the SUITE only; this job timeout must additionally + # cover setup, which here is the larger and more variable half: submodule + # checkout, apt, protoc plus two go installs plus protogen-go, a cold-cache + # module download (cache: false), a full go build of ./cmd/local-ai, and a + # separate ginkgo test compile. That build alone is ~316s of CPU, so on a + # 4-vCPU runner setup is realistically 8-12 minutes. + # + # 45 minutes therefore, matching the sibling job. A tighter number does not + # make a hang fail faster, it just moves the kill from Ginkgo, which prints + # which spec hung, to the runner, which prints nothing: a red job with no + # evidence, which is how a suite gets disabled rather than fixed. + # + # The suite itself is 8m39s over three consecutive runs (509.1s / 509.8s / + # 512.3s) on a developer box, and will be slower here. Three specs sit at + # ~167s each because they wait out a 60s staleness threshold plus a 15s + # reconcile tick. Do not shorten those windows to make this job faster: the + # wait is what stops the assertions from passing before the system could + # have reacted, which was a real false green earlier on. + timeout-minutes: 45 steps: - name: Clone uses: actions/checkout@v7 @@ -130,6 +144,10 @@ jobs: # suite drives the HTTP API and never the UI, which has its own e2e # suite. A single index.html satisfies the //go:embed react-ui/dist/* # in core/http/app.go, so the job skips a full Node and Vite install. + # If a cluster spec ever asserts on a UI asset, this step must go and + # the real build come back: a developer box has a real dist/, so such a + # spec would pass locally and fail only here, or worse be served the + # stub and pass in both places. run: | mkdir -p core/http/react-ui/dist printf 'stub\n' > core/http/react-ui/dist/index.html @@ -168,7 +186,7 @@ jobs: # Ginkgo output says which assertion failed, not what the four child # processes were doing. Without this a red job is undebuggable. if: ${{ failure() }} - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: cluster-process-logs path: cluster-logs/**/*.log diff --git a/Makefile b/Makefile index b3252dba9f9e..b828390752ee 100644 --- a/Makefile +++ b/Makefile @@ -352,29 +352,34 @@ DISTRIBUTED_TEST_FLAKES?=1 # Cluster is excluded too and runs in test-e2e-cluster below, which needs a # built binary. The argument-validation specs under tests/e2e/distributed/cluster # carry Label("Distributed") only, so they run here and not there, on purpose. +# -r stays because of those: they are in a subpackage this target must reach. +# --fail-on-empty because ginkgo exits 0 when a label filter matches nothing, so +# without it a rename of the label would turn this target into a silent no-op +# that still reports "Test Suite Passed". test-e2e-distributed: protogen-go @echo 'Running distributed e2e tests (label Distributed, incl. NatsJWT)' - $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Distributed && !VLLMMultinode && !Cluster' --flake-attempts $(DISTRIBUTED_TEST_FLAKES) --timeout=40m -v -r ./tests/e2e/distributed + $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Distributed && !VLLMMultinode && !Cluster' --fail-on-empty --flake-attempts $(DISTRIBUTED_TEST_FLAKES) --timeout=40m -v -r ./tests/e2e/distributed # Cluster e2e: runs local-ai as real child processes (frontend replicas + # workers) against PostgreSQL and NATS, and kills them to assert failover. # Needs a built ./local-ai (or LOCALAI_E2E_BINARY) plus the mock backend. # -# The label split is deliberate and asymmetric. --label-filter='Cluster' -# selects only the specs that spawn processes: the baseline and failover specs -# in tests/e2e/distributed. The argument-validation specs in -# tests/e2e/distributed/cluster carry Label("Distributed") alone, so they stay -# in test-e2e-distributed above, where they belong: they exercise the harness's -# own option handling, need no binary, no PostgreSQL and no NATS, and run in -# milliseconds. Ginkgo therefore reports 0 of 8 specs run for that package here. -# That zero is correct, not a filter bug. +# The argument-validation specs in tests/e2e/distributed/cluster deliberately +# stay in test-e2e-distributed above: they need no binary, no PostgreSQL and no +# NATS, so no -r here and that package is simply out of scope. +# +# --fail-on-empty is load-bearing, not tidiness. Ginkgo exits 0 when a label +# filter selects nothing, so without it a refactor that renames or drops +# Label("Cluster") leaves this target reporting "Test Suite Passed" having +# started no cluster at all. LOCALAI_E2E_REQUIRE_BINARIES does not cover this: +# it only fires inside a spec that is actually running. # # --flake-attempts is pinned to 1 rather than $(DISTRIBUTED_TEST_FLAKES), and # should stay there: this suite exists to catch nondeterministic cluster # behaviour, and a retry turns exactly that signal into a green run. test-e2e-cluster: protogen-go build-mock-backend @echo 'Running cluster e2e tests (label Cluster, real local-ai processes)' - $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Cluster' --flake-attempts 1 --timeout=20m -v -r ./tests/e2e/distributed + $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --label-filter='Cluster' --fail-on-empty --flake-attempts 1 --timeout=20m -v ./tests/e2e/distributed # vLLM multi-node DP smoke (CPU). Builds local-ai:tests and the # cpu-vllm backend from the current working tree, then drives a From b0e01e9814224d15af2310a67274d64632ac0044 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 20:11:49 +0000 Subject: [PATCH 20/23] docs: document the distributed and cluster e2e suites Two Make targets, a flake-budget variable and two environment variables landed with no way to discover them. CONTRIBUTING.md now tells a contributor how to run both suites, what each costs and which variables steer the cluster one. .agents/building-and-testing.md records the decisions that are easy to undo by accident: suite-scoped containers, the shared NATS bus and what that means for a new spec, BeforeSuite over SynchronizedBeforeSuite, the label split, --fail-on-empty, the binary gate, the flake budget of 1, the coverage exclusion, and why the cluster suite's long waits must not be shortened. .agents/ci-caching.md lists tests-e2e-distributed.yml in its paths-ignore inventory; the workflow already pointed readers there, so the cross-reference was dangling. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .agents/building-and-testing.md | 19 +++++++++++++++++++ .agents/ci-caching.md | 4 ++-- CONTRIBUTING.md | 31 +++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/.agents/building-and-testing.md b/.agents/building-and-testing.md index 021d555ec993..f82445f7e5fa 100644 --- a/.agents/building-and-testing.md +++ b/.agents/building-and-testing.md @@ -45,3 +45,22 @@ Rules (both gates): - **Don't weaken the gate:** never hand-lower a baseline or widen a tolerance to turn a red gate green. The ratchet only moves up. - If a change drops coverage, **add tests** (sort `coverage-summary.json` by line% ascending to find untested code) rather than editing the baseline. When coverage legitimately rises, commit the regenerated baseline (`make test-coverage-baseline` / `test-ui-coverage-baseline`). - The Go gate is **strict — no tolerance**; `covermode=atomic` keeps it deterministic. The UI gate keeps a small tolerance only because its e2e coverage isn't. + +## Distributed-mode test suites + +Two suites cover distributed mode (frontend replicas, worker nodes, PostgreSQL, NATS), split by a Ginkgo label: + +- `make test-e2e-distributed` runs `Distributed && !VLLMMultinode && !Cluster` over `./tests/e2e/distributed` recursively. Services are wired directly into the test binary. ~240 specs, ~75s. +- `make test-e2e-cluster` runs `Cluster` and spawns real `local-ai` child processes through the `tests/e2e/distributed/cluster` helper package. 6 specs, 8m39s measured over three consecutive runs (509.1s / 509.8s / 512.3s). + +Both jobs live in `.github/workflows/tests-e2e-distributed.yml`, PR-triggered with a `paths-ignore` filter (see [.agents/ci-caching.md](ci-caching.md)) and `timeout-minutes: 45` each. They are advisory only because `master` carries no branch protection, which is a repository setting and not a YAML key: `continue-on-error: true` would flip the run's *conclusion* to success and hide the failure, so it is not used. + +- **Containers are suite-scoped, not spec-scoped.** `SetupInfra` used to start a PostgreSQL (~10s) and a NATS (~3.5s) per spec. Across the 213 specs behind it that was roughly **48 minutes of pure container startup per run**, which is why this suite was never in CI. Containers now start once in `BeforeSuite` and each spec gets its own database via `CREATE DATABASE` (~67ms), which is what the `dbName` argument was always describing. Adding a spec needs no change: call `SetupInfra("some-name")` as before, the name is a prefix and a counter keeps it unique. +- **Consequence for new specs:** the NATS bus is now *shared* within a Ginkgo process, so a wildcard subscriber can observe another spec's traffic. Filter assertions on an identifier your spec owns (a node ID, a job ID) instead of counting everything on `jobs.*.progress`, and verify the spec with `--randomize-all`. +- **`BeforeSuite`, not `SynchronizedBeforeSuite`.** Under `ginkgo -p` each process then gets its own container pair, keeping NATS subjects isolated per process. A single shared NATS across parallel processes would let specs on different processes see each other's messages on the same subject. +- **The label split.** The 8 argument-validation specs under `tests/e2e/distributed/cluster/` carry `Label("Distributed")` only, on purpose: they need no binary, no PostgreSQL and no NATS, so they belong in the fast job. That is why `test-e2e-distributed` keeps `-r` (it must reach the subpackage) and `test-e2e-cluster` deliberately does **not** (the subpackage is out of its scope). +- **`--fail-on-empty` is load-bearing on both targets.** Ginkgo exits 0 when a label filter selects nothing, so without it a refactor that renames or drops `Label("Cluster")` leaves the target reporting "Test Suite Passed" having started no cluster at all. `LOCALAI_E2E_REQUIRE_BINARIES` does not cover this case: it only fires inside a spec that is actually running. +- **The binary gate.** `localAIBinary()` and `mockBackendBinary()` **fail** rather than skip when `CI` is set, or when `LOCALAI_E2E_REQUIRE_BINARIES` is truthy; `LOCALAI_E2E_REQUIRE_BINARIES=0` (also `off`/`no`/`disabled`) forces skipping even under CI. The polarity is deliberate: in CI a skipped cluster spec is indistinguishable from a passing one, because Ginkgo exits 0 on skips. Locally a missing binary still just skips, since `CI` is unset in an ordinary shell. +- **Flake budget:** `DISTRIBUTED_TEST_FLAKES` defaults to **1**, not the repo-wide `TEST_FLAKES=5`, and `test-e2e-cluster` pins `--flake-attempts 1` outright. These suites exist to surface nondeterminism, so a retry converts exactly that signal into a green run. Raise it locally when bisecting something unrelated, not in the Makefile. +- **Coverage:** `tests/e2e/distributed` is excluded from the coverage roots (`COVERAGE_E2E_ROOTS = ./tests/e2e`, run non-recursively), and so is the `cluster` helper package beneath it. Neither suite moves the baseline, so production code that these suites are the only cover for reads as **uncovered**. Unit tests for such code belong under `./core/...` with `testutil.SetupTestDB()`. +- **Do not shorten the cluster suite's waits.** Three of its six specs sit at ~167s each because they wait out a 60s staleness threshold plus a 15s health-check tick. That wait is what stops the assertions from passing before the system could have reacted, which was a real false green earlier on. If the job has to get faster, the levers are CI concurrency or making the thresholds configurable, not shorter waits. diff --git a/.agents/ci-caching.md b/.agents/ci-caching.md index 6742049e68ff..244bb4549fcb 100644 --- a/.agents/ci-caching.md +++ b/.agents/ci-caching.md @@ -153,7 +153,7 @@ This is worth more than it looks. Measured over the week to 2026-07-30, **97% of The volume is real: 13 gallery-only PRs merged that week with 10 open at once, and 78 of the 137 PRs opened were bot-generated. -`paths-ignore` on the PR trigger of `image-pr.yml` (7 jobs), `build-test.yaml` (3), `lint.yml` (2) and `tests-e2e.yml` (1) drops 13 of those 20. The excluded set: +`paths-ignore` on the PR trigger of `image-pr.yml` (7 jobs), `build-test.yaml` (3), `lint.yml` (2) and `tests-e2e.yml` (1) drops 13 of those 20. `tests-e2e-distributed.yml` (2 jobs) landed after that measurement and carries the same exclusion set for the same reason: its dependency graph is 99 packages, so an allowlist of paths would silently stop guarding the moment code moved, while a diff confined to the paths below provably cannot reach it. The excluded set: | Path | Why no image or Go build can see it | |---|---| @@ -192,7 +192,7 @@ What still runs, and why it has to: Two properties this relies on: - `paths-ignore` skips a run only when **every** changed file matches, so a PR touching the gallery *and* Go code still runs everything. That is what makes the exclusion safe rather than a hole. -- `master` carries no branch protection and no rulesets, so a skipped workflow reports no status and nothing waits on it. If required status checks are ever introduced, these four entries must be excluded from the required set or PRs will hang on "Expected — Waiting for status to be reported". +- `master` carries no branch protection and no rulesets, so a skipped workflow reports no status and nothing waits on it. If required status checks are ever introduced, these five entries must be excluded from the required set or PRs will hang on "Expected — Waiting for status to be reported". ### `image.yml` on master push is gated too, by a job rather than a path filter diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d87db37eae63..9c1b176af009 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -265,6 +265,37 @@ The e2e tests run LocalAI in a Docker container and exercise the API: make test-e2e ``` +### Running distributed-mode tests + +Distributed mode (several frontend replicas, worker nodes, PostgreSQL and NATS) has two suites. Both bring up their PostgreSQL and NATS with testcontainers, so Docker has to be available: + +```bash +make test-e2e-distributed # in-process: services wired directly into the test binary +make test-e2e-cluster # process-level: real local-ai child processes +``` + +`make test-e2e-distributed` is the fast one (around 240 specs in roughly 75 seconds). It starts one PostgreSQL and one NATS for the whole run and gives each spec its own database. It retries a failing spec once (`DISTRIBUTED_TEST_FLAKES`, default 1, deliberately lower than the repo-wide `TEST_FLAKES=5`), because this suite exists to catch nondeterministic cluster behaviour and retrying hides exactly that. + +`make test-e2e-cluster` runs `local-ai` as real child processes, one per frontend replica and one per worker, so a spec can kill a replica and assert what the survivors do. Budget about 8m40s: three of its six specs wait out real staleness and health-check windows. It needs a built binary and the mock backend: + +```bash +make build build-mock-backend +make test-e2e-cluster +``` + +Two environment variables steer it: + +| Variable | Purpose | +|---|---| +| `LOCALAI_E2E_BINARY` | path to the `local-ai` binary (default: `local-ai` in the repository root) | +| `LOCALAI_E2E_LOG_DIR` | directory for the per-process logs (default: a Ginkgo temp dir) | + +Set `LOCALAI_E2E_LOG_DIR` when debugging. A cluster failure is unreadable without the individual frontend and worker logs, and Ginkgo only tells you which assertion failed. + +A missing binary skips the cluster specs locally but fails them whenever `CI` is set, so a build problem cannot turn the CI job green without ever starting a cluster. `LOCALAI_E2E_REQUIRE_BINARIES=1` forces that failing behaviour anywhere; `LOCALAI_E2E_REQUIRE_BINARIES=0` forces the skip back on even under CI. + +Both suites run on pull requests via `.github/workflows/tests-e2e-distributed.yml`. + ### React UI tests and coverage The React UI (`core/http/react-ui/`) is covered by Playwright e2e specs, gated by a **monotonic line-coverage ratchet** (`make test-ui-coverage-check`, run in CI). The metric is non-deterministic — a fast local box reads higher than a slow CI runner for the same code — so a small tolerance is unavoidable. From 2c314d66d64a19a5688b3ac9d928af784f1ff47a Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 20:21:36 +0000 Subject: [PATCH 21/23] docs: correct the flake-attempts semantics and fill the review gaps --flake-attempts is total attempts, not retries: ginkgo v2.29.0 sets maxAttempts = FlakeAttempts and loops attempt < maxAttempts, and the flag's usage string reads "0 - failed tests are not retried". At 1 there is no retry at all, so "retries a failing spec once" was false in CONTRIBUTING.md and implied in .agents/building-and-testing.md. Both now say each spec runs once, and cite the source so the next reader need not re-derive it. Also restores the React-UI stub rationale, which is load-bearing because a spec asserting on a UI asset passes locally against a real dist/ and is served the stub in CI; explains why 213 and ~240 differ; records that the workflow also triggers on master pushes, where paths-ignore does not apply; and completes the LOCALAI_E2E_REQUIRE_BINARIES value table, including that any unparseable value reads as ON. In .agents/ci-caching.md the stale "13 of those 20" figure now carries its qualifier inline rather than in the following sentence. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .agents/building-and-testing.md | 9 +++++---- .agents/ci-caching.md | 2 +- CONTRIBUTING.md | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/.agents/building-and-testing.md b/.agents/building-and-testing.md index f82445f7e5fa..874e49f8b3b7 100644 --- a/.agents/building-and-testing.md +++ b/.agents/building-and-testing.md @@ -53,14 +53,15 @@ Two suites cover distributed mode (frontend replicas, worker nodes, PostgreSQL, - `make test-e2e-distributed` runs `Distributed && !VLLMMultinode && !Cluster` over `./tests/e2e/distributed` recursively. Services are wired directly into the test binary. ~240 specs, ~75s. - `make test-e2e-cluster` runs `Cluster` and spawns real `local-ai` child processes through the `tests/e2e/distributed/cluster` helper package. 6 specs, 8m39s measured over three consecutive runs (509.1s / 509.8s / 512.3s). -Both jobs live in `.github/workflows/tests-e2e-distributed.yml`, PR-triggered with a `paths-ignore` filter (see [.agents/ci-caching.md](ci-caching.md)) and `timeout-minutes: 45` each. They are advisory only because `master` carries no branch protection, which is a repository setting and not a YAML key: `continue-on-error: true` would flip the run's *conclusion* to success and hide the failure, so it is not used. +Both jobs live in `.github/workflows/tests-e2e-distributed.yml`, with `timeout-minutes: 45` each. They trigger on pull requests *and* on every push to `master`; the `paths-ignore` filter (see [.agents/ci-caching.md](ci-caching.md)) sits on the pull-request trigger only, so a master push always runs both. They are advisory only because `master` carries no branch protection, which is a repository setting and not a YAML key: `continue-on-error: true` would flip the run's *conclusion* to success and hide the failure, so it is not used. -- **Containers are suite-scoped, not spec-scoped.** `SetupInfra` used to start a PostgreSQL (~10s) and a NATS (~3.5s) per spec. Across the 213 specs behind it that was roughly **48 minutes of pure container startup per run**, which is why this suite was never in CI. Containers now start once in `BeforeSuite` and each spec gets its own database via `CREATE DATABASE` (~67ms), which is what the `dbName` argument was always describing. Adding a spec needs no change: call `SetupInfra("some-name")` as before, the name is a prefix and a counter keeps it unique. +- **Containers are suite-scoped, not spec-scoped.** `SetupInfra` used to start a PostgreSQL (~10s) and a NATS (~3.5s) per spec. Across the 213 specs behind it that was roughly **48 minutes of pure container startup per run**, which is why this suite was never in CI. (213 rather than the ~240 above: the larger number is everything the label filter selects, the smaller one is just the specs that call `SetupInfra`.) Containers now start once in `BeforeSuite` and each spec gets its own database via `CREATE DATABASE` (~67ms), which is what the `dbName` argument was always describing. Adding a spec needs no change: call `SetupInfra("some-name")` as before, the name is a prefix and a counter keeps it unique. - **Consequence for new specs:** the NATS bus is now *shared* within a Ginkgo process, so a wildcard subscriber can observe another spec's traffic. Filter assertions on an identifier your spec owns (a node ID, a job ID) instead of counting everything on `jobs.*.progress`, and verify the spec with `--randomize-all`. - **`BeforeSuite`, not `SynchronizedBeforeSuite`.** Under `ginkgo -p` each process then gets its own container pair, keeping NATS subjects isolated per process. A single shared NATS across parallel processes would let specs on different processes see each other's messages on the same subject. - **The label split.** The 8 argument-validation specs under `tests/e2e/distributed/cluster/` carry `Label("Distributed")` only, on purpose: they need no binary, no PostgreSQL and no NATS, so they belong in the fast job. That is why `test-e2e-distributed` keeps `-r` (it must reach the subpackage) and `test-e2e-cluster` deliberately does **not** (the subpackage is out of its scope). - **`--fail-on-empty` is load-bearing on both targets.** Ginkgo exits 0 when a label filter selects nothing, so without it a refactor that renames or drops `Label("Cluster")` leaves the target reporting "Test Suite Passed" having started no cluster at all. `LOCALAI_E2E_REQUIRE_BINARIES` does not cover this case: it only fires inside a spec that is actually running. -- **The binary gate.** `localAIBinary()` and `mockBackendBinary()` **fail** rather than skip when `CI` is set, or when `LOCALAI_E2E_REQUIRE_BINARIES` is truthy; `LOCALAI_E2E_REQUIRE_BINARIES=0` (also `off`/`no`/`disabled`) forces skipping even under CI. The polarity is deliberate: in CI a skipped cluster spec is indistinguishable from a passing one, because Ginkgo exits 0 on skips. Locally a missing binary still just skips, since `CI` is unset in an ordinary shell. -- **Flake budget:** `DISTRIBUTED_TEST_FLAKES` defaults to **1**, not the repo-wide `TEST_FLAKES=5`, and `test-e2e-cluster` pins `--flake-attempts 1` outright. These suites exist to surface nondeterminism, so a retry converts exactly that signal into a green run. Raise it locally when bisecting something unrelated, not in the Makefile. +- **The binary gate.** `localAIBinary()` and `mockBackendBinary()` **fail** rather than skip when `CI` is set, or when `LOCALAI_E2E_REQUIRE_BINARIES` is truthy; `LOCALAI_E2E_REQUIRE_BINARIES=0` (also `off`, `no`, `n`, `disabled`, and anything `strconv.ParseBool` reads as false) forces skipping even under CI. **Any value that parses as neither reads as ON**, not off: setting the variable to something meaningless means someone meant to turn the gate on, and reading it as false would quietly restore the silent skip the flag exists to remove. The whole polarity is deliberate, because in CI a skipped cluster spec is indistinguishable from a passing one: Ginkgo exits 0 on skips. Locally a missing binary still just skips, since `CI` is unset in an ordinary shell. +- **Flake budget: no retries at all.** `--flake-attempts` is *total attempts*, not retries (ginkgo v2.29.0 `internal/group.go` sets `maxAttempts = FlakeAttempts` and loops `attempt < maxAttempts`; the flag's own usage string reads "0 - failed tests are not retried"). `DISTRIBUTED_TEST_FLAKES` defaults to **1**, so each spec runs once and a failure is a failure, and `test-e2e-cluster` pins `--flake-attempts 1` outright rather than reading the variable. The repo-wide `TEST_FLAKES=5` means up to five attempts, so up to four retries. These suites exist to surface nondeterminism, and a retry converts exactly that signal into a green run. Raise it locally when bisecting something unrelated, not in the Makefile. - **Coverage:** `tests/e2e/distributed` is excluded from the coverage roots (`COVERAGE_E2E_ROOTS = ./tests/e2e`, run non-recursively), and so is the `cluster` helper package beneath it. Neither suite moves the baseline, so production code that these suites are the only cover for reads as **uncovered**. Unit tests for such code belong under `./core/...` with `testutil.SetupTestDB()`. +- **The cluster job builds against a stubbed React UI.** `core/http/react-ui/dist` is gitignored and built by Node, so the workflow writes a one-line `index.html` there to satisfy the `//go:embed react-ui/dist/*` in `core/http/app.go` and skips a full Node and Vite install. That holds only while the suite drives the HTTP API and never the UI, which has its own e2e suite. A spec that ever asserts on a UI asset would pass locally, where a real `dist/` exists, and be served the stub in CI: if you write one, the stub step has to go and the real build come back. - **Do not shorten the cluster suite's waits.** Three of its six specs sit at ~167s each because they wait out a 60s staleness threshold plus a 15s health-check tick. That wait is what stops the assertions from passing before the system could have reacted, which was a real false green earlier on. If the job has to get faster, the levers are CI concurrency or making the thresholds configurable, not shorter waits. diff --git a/.agents/ci-caching.md b/.agents/ci-caching.md index 244bb4549fcb..8dc243747384 100644 --- a/.agents/ci-caching.md +++ b/.agents/ci-caching.md @@ -153,7 +153,7 @@ This is worth more than it looks. Measured over the week to 2026-07-30, **97% of The volume is real: 13 gallery-only PRs merged that week with 10 open at once, and 78 of the 137 PRs opened were bot-generated. -`paths-ignore` on the PR trigger of `image-pr.yml` (7 jobs), `build-test.yaml` (3), `lint.yml` (2) and `tests-e2e.yml` (1) drops 13 of those 20. `tests-e2e-distributed.yml` (2 jobs) landed after that measurement and carries the same exclusion set for the same reason: its dependency graph is 99 packages, so an allowlist of paths would silently stop guarding the moment code moved, while a diff confined to the paths below provably cannot reach it. The excluded set: +`paths-ignore` on the PR trigger of `image-pr.yml` (7 jobs), `build-test.yaml` (3), `lint.yml` (2) and `tests-e2e.yml` (1) drops 13 of those 20, measured before `tests-e2e-distributed.yml` (2 jobs) landed. That workflow carries the same exclusion set for the same reason: its dependency graph is 99 packages, so an allowlist of paths would silently stop guarding the moment code moved, while a diff confined to the paths below provably cannot reach it. The excluded set: | Path | Why no image or Go build can see it | |---|---| diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c1b176af009..ef389911eb36 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -274,7 +274,7 @@ make test-e2e-distributed # in-process: services wired directly into the test make test-e2e-cluster # process-level: real local-ai child processes ``` -`make test-e2e-distributed` is the fast one (around 240 specs in roughly 75 seconds). It starts one PostgreSQL and one NATS for the whole run and gives each spec its own database. It retries a failing spec once (`DISTRIBUTED_TEST_FLAKES`, default 1, deliberately lower than the repo-wide `TEST_FLAKES=5`), because this suite exists to catch nondeterministic cluster behaviour and retrying hides exactly that. +`make test-e2e-distributed` is the fast one (around 240 specs in roughly 75 seconds). It starts one PostgreSQL and one NATS for the whole run and gives each spec its own database. It runs each spec exactly once, with no retry: `DISTRIBUTED_TEST_FLAKES` defaults to 1 and feeds ginkgo's `--flake-attempts`, which counts *total attempts*, not retries. That is deliberately below the repo-wide `TEST_FLAKES=5`, because this suite exists to catch nondeterministic cluster behaviour and a retry hides exactly the failure it is meant to catch. Raise it locally when bisecting something unrelated. `make test-e2e-cluster` runs `local-ai` as real child processes, one per frontend replica and one per worker, so a spec can kill a replica and assert what the survivors do. Budget about 8m40s: three of its six specs wait out real staleness and health-check windows. It needs a built binary and the mock backend: @@ -294,7 +294,7 @@ Set `LOCALAI_E2E_LOG_DIR` when debugging. A cluster failure is unreadable withou A missing binary skips the cluster specs locally but fails them whenever `CI` is set, so a build problem cannot turn the CI job green without ever starting a cluster. `LOCALAI_E2E_REQUIRE_BINARIES=1` forces that failing behaviour anywhere; `LOCALAI_E2E_REQUIRE_BINARIES=0` forces the skip back on even under CI. -Both suites run on pull requests via `.github/workflows/tests-e2e-distributed.yml`. +Both suites run in `.github/workflows/tests-e2e-distributed.yml`, on pull requests and on every push to `master`. The `paths-ignore` filter is on the pull-request trigger only, so a master push always runs them. ### React UI tests and coverage From c5796d407f38217c297aaf5f71285bdbfa219ae7 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 20:47:33 +0000 Subject: [PATCH 22/23] test(distributed): correct the claims the e2e comments make Review of the whole branch found five comments that would send a reader to the wrong place, plus three smaller inaccuracies. Nothing here changes behaviour. The KNOWN RACE note on both backend-log WebSocket handlers said the fix needs an atomic snapshot-plus-subscribe "under the store lock". It does not: BackendLogStore.mu guards only the buffers map, and AppendLine enqueues and fans out under the per-buffer buf.mu. Whoever took the store lock would ship and the race would survive, so both notes now name buf.mu and say what s.mu does and does not exclude. Two comments in the cluster harness quoted Eventually(c.FrontendAlive) .Should(BeFalse()). FrontendAlive takes an index, so Gomega rejects that with "requested 1 arguments but received 0". Both now quote the closure form the specs actually use, and say why the closure is needed. proveHealthCheckingIsAlive claimed to prove the health monitor ran for the whole preceding window. It proves the monitor was alive at the end of it, and inferring backwards needs any wedge to be sticky. In the peer-replica-death spec that inverts: health checks are single-flighted by a session-scoped pg_try_advisory_lock, the spec SIGKILLs the replica that may hold it, and until Postgres reaps the session the survivor acquires nothing and checks nothing silently. Consistently(healthy) can then pass because nothing was checking, with the positive control still succeeding once the lock frees. The doc now states what is proven, names that gap, and says the assertion is a floor rather than a proof. The Makefile still called DISTRIBUTED_TEST_FLAKES a retry count, which is what seeded that error into the two docs just corrected against it, and the workflow called the 15s window a reconcile tick when the mechanism is HealthCheckInterval in the node health monitor. Also: the cluster suite measured 509.1s / 509.8s / 512.3s, so about 8m30s and not the 8m39s/8m40s three files claimed; the dead-worker spec title implied two independent detectors when both probes read one advisory-lock-serialised verdict out of the same row; and the sanitizeDBName length assertion used <= 50, which an empty string also satisfies, where the invariant for an over-long input is exactly 50. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .agents/building-and-testing.md | 2 +- .github/workflows/tests-e2e-distributed.yml | 9 ++-- CONTRIBUTING.md | 2 +- Makefile | 9 ++-- core/http/endpoints/localai/backend_logs.go | 8 +++- core/services/nodes/file_transfer_server.go | 8 +++- tests/e2e/distributed/cluster/failure.go | 18 ++++++-- .../e2e/distributed/cluster_failover_test.go | 46 ++++++++++++++++--- tests/e2e/distributed/dbname_test.go | 3 +- 9 files changed, 80 insertions(+), 25 deletions(-) diff --git a/.agents/building-and-testing.md b/.agents/building-and-testing.md index 874e49f8b3b7..eee5123ac82c 100644 --- a/.agents/building-and-testing.md +++ b/.agents/building-and-testing.md @@ -51,7 +51,7 @@ Rules (both gates): Two suites cover distributed mode (frontend replicas, worker nodes, PostgreSQL, NATS), split by a Ginkgo label: - `make test-e2e-distributed` runs `Distributed && !VLLMMultinode && !Cluster` over `./tests/e2e/distributed` recursively. Services are wired directly into the test binary. ~240 specs, ~75s. -- `make test-e2e-cluster` runs `Cluster` and spawns real `local-ai` child processes through the `tests/e2e/distributed/cluster` helper package. 6 specs, 8m39s measured over three consecutive runs (509.1s / 509.8s / 512.3s). +- `make test-e2e-cluster` runs `Cluster` and spawns real `local-ai` child processes through the `tests/e2e/distributed/cluster` helper package. 6 specs, about 8m30s measured over three consecutive runs (509.1s / 509.8s / 512.3s, so 8m29s to 8m32s). Both jobs live in `.github/workflows/tests-e2e-distributed.yml`, with `timeout-minutes: 45` each. They trigger on pull requests *and* on every push to `master`; the `paths-ignore` filter (see [.agents/ci-caching.md](ci-caching.md)) sits on the pull-request trigger only, so a master push always runs both. They are advisory only because `master` carries no branch protection, which is a repository setting and not a YAML key: `continue-on-error: true` would flip the run's *conclusion* to success and hide the failure, so it is not used. diff --git a/.github/workflows/tests-e2e-distributed.yml b/.github/workflows/tests-e2e-distributed.yml index c69ae7924553..d0cf24875c14 100644 --- a/.github/workflows/tests-e2e-distributed.yml +++ b/.github/workflows/tests-e2e-distributed.yml @@ -108,10 +108,11 @@ jobs: # which spec hung, to the runner, which prints nothing: a red job with no # evidence, which is how a suite gets disabled rather than fixed. # - # The suite itself is 8m39s over three consecutive runs (509.1s / 509.8s / - # 512.3s) on a developer box, and will be slower here. Three specs sit at - # ~167s each because they wait out a 60s staleness threshold plus a 15s - # reconcile tick. Do not shorten those windows to make this job faster: the + # The suite itself is about 8m30s over three consecutive runs (509.1s / + # 509.8s / 512.3s, so 8m29s to 8m32s) on a developer box, and will be slower + # here. Three specs sit at ~167s each because they wait out a 60s staleness + # threshold plus a 15s health-check tick (HealthCheckInterval, in + # core/services/nodes/health.go, not one of the reconcilers). Do not shorten those windows to make this job faster: the # wait is what stops the assertions from passing before the system could # have reacted, which was a real false green earlier on. timeout-minutes: 45 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ef389911eb36..f48c7c4a337c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -276,7 +276,7 @@ make test-e2e-cluster # process-level: real local-ai child processes `make test-e2e-distributed` is the fast one (around 240 specs in roughly 75 seconds). It starts one PostgreSQL and one NATS for the whole run and gives each spec its own database. It runs each spec exactly once, with no retry: `DISTRIBUTED_TEST_FLAKES` defaults to 1 and feeds ginkgo's `--flake-attempts`, which counts *total attempts*, not retries. That is deliberately below the repo-wide `TEST_FLAKES=5`, because this suite exists to catch nondeterministic cluster behaviour and a retry hides exactly the failure it is meant to catch. Raise it locally when bisecting something unrelated. -`make test-e2e-cluster` runs `local-ai` as real child processes, one per frontend replica and one per worker, so a spec can kill a replica and assert what the survivors do. Budget about 8m40s: three of its six specs wait out real staleness and health-check windows. It needs a built binary and the mock backend: +`make test-e2e-cluster` runs `local-ai` as real child processes, one per frontend replica and one per worker, so a spec can kill a replica and assert what the survivors do. Budget about 8m30s (measured 509.1s / 509.8s / 512.3s over three consecutive runs): three of its six specs wait out real staleness and health-check windows. It needs a built binary and the mock backend: ```bash make build build-mock-backend diff --git a/Makefile b/Makefile index b828390752ee..2cdca27fc44c 100644 --- a/Makefile +++ b/Makefile @@ -340,10 +340,11 @@ run-e2e-aio: protogen-go @echo 'Running e2e AIO tests' $(GOCMD) run github.com/onsi/ginkgo/v2/ginkgo --flake-attempts $(TEST_FLAKES) -v -r ./tests/e2e-aio -# Flake retries for the distributed suite. Defaults to 1, unlike TEST_FLAKES: -# this suite exists to catch nondeterministic cluster behaviour, and retrying -# hides exactly the failures it is meant to surface. Raise it locally if you are -# bisecting something unrelated. +# Total ginkgo attempts per spec for the distributed suite: --flake-attempts counts +# attempts, not retries. Defaults to 1, so each spec runs once and is never retried, +# unlike TEST_FLAKES=5. This suite exists to catch nondeterministic cluster behaviour, +# and a retry hides exactly the failures it is meant to surface. Raise it locally if +# you are bisecting something unrelated. DISTRIBUTED_TEST_FLAKES?=1 # Distributed architecture e2e (PostgreSQL + NATS via testcontainers). diff --git a/core/http/endpoints/localai/backend_logs.go b/core/http/endpoints/localai/backend_logs.go index 8b5f99d66f78..f2112502d253 100644 --- a/core/http/endpoints/localai/backend_logs.go +++ b/core/http/endpoints/localai/backend_logs.go @@ -125,8 +125,12 @@ func BackendLogsWebSocketEndpoint(ml *model.ModelLoader) echo.HandlerFunc { // a line appended in that window is never streamed. A viewer attaching while // a model loads (when a backend is at its noisiest) can silently miss lines; // they stay in the buffer, so a reload shows them. Fixing it needs an atomic - // snapshot-plus-subscribe under the store lock, not a reorder of these two - // calls, which would duplicate instead of drop. + // snapshot-plus-subscribe held under the buffer's own lock (buf.mu in + // pkg/model/backend_log_store.go), because that is the lock AppendLine takes + // while it enqueues and fans out to subscribers. The store-level s.mu guards + // only the buffers map and excludes nothing an appender does, so taking it + // leaves this race exactly where it is. Reordering these two calls is not a + // fix either: it would duplicate instead of drop. // Send existing lines as initial batch existingLines := ml.BackendLogs().GetLines(modelID) diff --git a/core/services/nodes/file_transfer_server.go b/core/services/nodes/file_transfer_server.go index 2d4bc03baaeb..9a3f0ca2f5bf 100644 --- a/core/services/nodes/file_transfer_server.go +++ b/core/services/nodes/file_transfer_server.go @@ -843,8 +843,12 @@ func handleBackendLogsWS(w http.ResponseWriter, r *http.Request, logStore *model // a line appended in that window is never streamed. A viewer attaching while // a model loads (when a backend is at its noisiest) can silently miss lines; // they stay in the buffer, so a reload shows them. Fixing it needs an atomic - // snapshot-plus-subscribe under the store lock, not a reorder of these two - // calls, which would duplicate instead of drop. + // snapshot-plus-subscribe held under the buffer's own lock (buf.mu in + // pkg/model/backend_log_store.go), because that is the lock AppendLine takes + // while it enqueues and fans out to subscribers. The store-level s.mu guards + // only the buffers map and excludes nothing an appender does, so taking it + // leaves this race exactly where it is. Reordering these two calls is not a + // fix either: it would duplicate instead of drop. // Send existing lines as initial batch existingLines := logStore.GetLines(modelID) diff --git a/tests/e2e/distributed/cluster/failure.go b/tests/e2e/distributed/cluster/failure.go index 919376f4a550..9a461eed702a 100644 --- a/tests/e2e/distributed/cluster/failure.go +++ b/tests/e2e/distributed/cluster/failure.go @@ -71,8 +71,15 @@ func (c *Cluster) KillWorker(i int) error { // dressed up as a failover one. No spec does that today; this note is here so // the first one that tries does not spend a day on it. // -// After StopFrontendGracefully, wait for the process to actually go -// (Eventually(c.FrontendAlive).Should(BeFalse())) before restarting. Restart +// After StopFrontendGracefully, wait for the process to actually go before +// restarting: +// +// Eventually(func() bool { return c.FrontendAlive(i) }, "20s", "500ms"). +// Should(BeFalse()) +// +// FrontendAlive takes an index, so it has to be wrapped in a closure; handing +// Gomega the method value directly fails with "requested 1 arguments but +// received 0". Restart // terminates whatever is still running with SIGKILL, so restarting straight // after a SIGTERM cuts the drain short and quietly turns the rolling-update // case into the crash case, which is the opposite of what pairing those two @@ -126,8 +133,11 @@ func (c *Cluster) FrontendAlive(i int) bool { // than the one it precedes. The window that stays open is the other one, // between the child exiting and waitid collecting it: there the child is a // zombie, signal 0 to a zombie succeeds, and alive reports true for a process -// that is already dead. There is no local fix; the caller's is to poll, -// Eventually(c.FrontendAlive).Should(BeFalse()), rather than assert once. +// that is already dead. There is no local fix; the caller's is to poll rather +// than assert once, wrapping the index-taking FrontendAlive in a closure: +// +// Eventually(func() bool { return c.FrontendAlive(i) }, "20s", "500ms"). +// Should(BeFalse()) func (p *Process) alive() bool { if p == nil || p.Cmd == nil || p.Cmd.Process == nil { return false diff --git a/tests/e2e/distributed/cluster_failover_test.go b/tests/e2e/distributed/cluster_failover_test.go index 3e980cb3de75..2256a16ac187 100644 --- a/tests/e2e/distributed/cluster_failover_test.go +++ b/tests/e2e/distributed/cluster_failover_test.go @@ -134,11 +134,36 @@ func (p *rosterProbe) explainStuckOffline(worker, format string, args ...any) fu // It is the terminating positive control for the two specs that assert a // healthy worker STAYS healthy. On their own those are pure negative // assertions: a cluster whose health checking had wedged entirely, say by -// leaking the Postgres advisory lock the monitor takes (health.go:110), would -// freeze the roster and satisfy them while observing a corpse. Killing a worker -// afterwards and requiring the roster to react proves the monitor was running -// for the whole window, which is the only thing that makes the preceding -// Consistently a statement about behaviour rather than about a stopped clock. +// leaking the Postgres advisory lock the monitor takes +// (core/services/nodes/health.go:112), would freeze the roster and satisfy them +// while observing a corpse. +// +// WHAT IT ACTUALLY PROVES, which is less than it looks like. Killing a worker +// afterwards and requiring the roster to react proves the monitor was alive at +// the END of the preceding window. It does not observe the window itself. The +// inference back across it holds only if a wedge would have been sticky, i.e. +// still present when this helper ran. +// +// THE RESIDUAL GAP, and it is not hypothetical in the peer-replica-death spec. +// Health checks are single-flighted across replicas by a session-scoped +// pg_try_advisory_lock (advisorylock.TryWithLockCtx, non-blocking: a replica +// that does not get the lock returns immediately and checks nothing, silently, +// because checkAll discards the acquired flag). That spec SIGKILLs frontend 1, +// which may have been holding the lock at the moment it died. Postgres releases +// a session-level advisory lock only when it reaps the dead backend, so until +// then frontend 0's ticks acquire nothing and no check runs. The roster freezes, +// Consistently(healthy) passes BECAUSE NOTHING WAS CHECKING, and this helper +// still succeeds afterwards once the session is reaped and the lock comes free. +// That wedge is transient rather than permanent, which is exactly the shape the +// backwards inference cannot see. Low probability, real, and unbounded only by +// how fast Postgres notices a dead connection. +// +// So treat this as a floor and not a proof: it rules out a health monitor that +// is permanently dead, which is the failure that would otherwise make the +// preceding Consistently a statement about a stopped clock, and it does not rule +// out a monitor that was idle for part of the window. Closing the gap needs a +// positive observation from inside the window (a log or metric assertion that a +// check ran), not a stronger assertion here. // // It costs a full detection cycle, which is why it is a shared helper: the // wall-clock price should be paid once per spec and explained once. @@ -258,7 +283,7 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun proveHealthCheckingIsAlive(c, restarted, 0) }) - It("settles a dead worker to offline on every replica", func() { + It("settles a dead worker to offline and both replicas report it offline", func() { c := startCluster(2, 1) worker := c.WorkerName(0) @@ -297,6 +322,15 @@ var _ = Describe("Cluster failover", Label("Distributed"), Label("Cluster"), fun // prevents it, so explainStuckOffline says so in the failure message // when it sees a node stuck at unhealthy. Fixing it is LocalAI work, // not test work. + // Reading the same verdict at both replicas proves shared-verdict + // propagation, NOT two independent detectors. Health checks are + // single-flighted by the advisory lock (see proveHealthCheckingIsAlive), + // so exactly one replica ran the check that wrote the offline status, and + // both probes then read that one Postgres row back. What this rules out + // is a replica that keeps a private roster, or one that reads the shared + // row and reports something else. A spec claiming both replicas can + // detect death on their own would have to isolate them from each other, + // which the shared database makes impossible by design. for _, probe := range []*rosterProbe{at0, at1} { Eventually(probe.statusOf, workerDeathTimeout, rosterPollInterval). WithArguments(worker). diff --git a/tests/e2e/distributed/dbname_test.go b/tests/e2e/distributed/dbname_test.go index 76f1030747fa..33305533d3da 100644 --- a/tests/e2e/distributed/dbname_test.go +++ b/tests/e2e/distributed/dbname_test.go @@ -16,7 +16,8 @@ var _ = Describe("Test database naming", Label("Distributed"), func() { for i := 0; i < 100; i++ { long += "a" } - Expect(len(sanitizeDBName(long))).To(BeNumerically("<=", 50)) + Expect(len(sanitizeDBName(long))).To(Equal(50), + "an over-long name must be truncated to exactly the 50-byte budget; <= 50 would also accept an empty name") }) It("never produces an empty name", func() { From b13ebeaa1bac6404093095140ecb45419cb6e66e Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 31 Aug 2026 20:54:25 +0000 Subject: [PATCH 23/23] docs(e2e): correct three claims in the distributed e2e comments The closure note in cluster/failure.go quoted a Gomega error that Gomega does not emit. Describe the argument-count failure and the Eventually().WithArguments() hint instead, so nobody greps for a string that never appears. The advisory-lock note in cluster_failover_test.go called the wedge window unbounded. A SIGKILLed local child closes its socket at once, the Postgres backend reads EOF and is reaped in milliseconds, so the mechanism bounds the window tightly. Say bounded, and keep the low probability but real framing, which was right. The workflow comment attributed HealthCheckInterval to core/services/nodes/health.go. It is declared in core/config/distributed_config.go:64; health.go only carries the ticker on the unexported checkInterval. Point a debugger at the right file. Comments only, no behaviour change. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto --- .github/workflows/tests-e2e-distributed.yml | 8 +++++--- tests/e2e/distributed/cluster/failure.go | 5 +++-- tests/e2e/distributed/cluster_failover_test.go | 5 +++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/tests-e2e-distributed.yml b/.github/workflows/tests-e2e-distributed.yml index d0cf24875c14..8f17e72f169a 100644 --- a/.github/workflows/tests-e2e-distributed.yml +++ b/.github/workflows/tests-e2e-distributed.yml @@ -112,9 +112,11 @@ jobs: # 509.8s / 512.3s, so 8m29s to 8m32s) on a developer box, and will be slower # here. Three specs sit at ~167s each because they wait out a 60s staleness # threshold plus a 15s health-check tick (HealthCheckInterval, in - # core/services/nodes/health.go, not one of the reconcilers). Do not shorten those windows to make this job faster: the - # wait is what stops the assertions from passing before the system could - # have reacted, which was a real false green earlier on. + # core/config/distributed_config.go; core/services/nodes/health.go runs the + # ticker on the unexported checkInterval, not one of the reconcilers). Do + # not shorten those windows to make this job faster: the wait is what stops + # the assertions from passing before the system could have reacted, which + # was a real false green earlier on. timeout-minutes: 45 steps: - name: Clone diff --git a/tests/e2e/distributed/cluster/failure.go b/tests/e2e/distributed/cluster/failure.go index 9a461eed702a..fe808401cd56 100644 --- a/tests/e2e/distributed/cluster/failure.go +++ b/tests/e2e/distributed/cluster/failure.go @@ -78,8 +78,9 @@ func (c *Cluster) KillWorker(i int) error { // Should(BeFalse()) // // FrontendAlive takes an index, so it has to be wrapped in a closure; handing -// Gomega the method value directly fails with "requested 1 arguments but -// received 0". Restart +// Gomega the method value directly fails immediately: Eventually reports that +// the function it was given takes one argument and none were provided, and +// points at Eventually().WithArguments(). Restart // terminates whatever is still running with SIGKILL, so restarting straight // after a SIGTERM cuts the drain short and quietly turns the rolling-update // case into the crash case, which is the opposite of what pairing those two diff --git a/tests/e2e/distributed/cluster_failover_test.go b/tests/e2e/distributed/cluster_failover_test.go index 2256a16ac187..d007a8ae6ac8 100644 --- a/tests/e2e/distributed/cluster_failover_test.go +++ b/tests/e2e/distributed/cluster_failover_test.go @@ -155,8 +155,9 @@ func (p *rosterProbe) explainStuckOffline(worker, format string, args ...any) fu // Consistently(healthy) passes BECAUSE NOTHING WAS CHECKING, and this helper // still succeeds afterwards once the session is reaped and the lock comes free. // That wedge is transient rather than permanent, which is exactly the shape the -// backwards inference cannot see. Low probability, real, and unbounded only by -// how fast Postgres notices a dead connection. +// backwards inference cannot see. Low probability, real, and bounded by how +// fast Postgres reaps the dead backend, usually immediate on a local socket +// close. // // So treat this as a floor and not a proof: it rules out a health monitor that // is permanently dead, which is the failure that would otherwise make the