fix(distributed): survive a slow control-plane database - #11837
Merged
Conversation
scheduleNewModel asked the registry for a free replica slot and treated every error as "this node is full", so a control-plane database slow enough to time out the lookup evicted a healthy loaded model. The evicted process died, a peer frontend still holding its address dialled the dead port and retried, and the model thrashed between nodes. The comment on the branch already said it meant a full node; the code never tested for it. Evict only on ErrNoFreeSlot. Any other error now returns and names the lookup that failed, so a slow database degrades into a diagnosable load failure instead of into lost work. An audit of the rest of the router found one branch of the same shape: node selection discarded the error from its last-resort finder, so a database timeout there also produced a nil node and evicted for it. That path now returns unless the finder said gorm.ErrRecordNotFound, which is the only answer that means the cluster had no node to give. No other destructive branch in router.go fires on a generic error. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
…y beat Every heartbeat UPDATEd backend_nodes. Six nodes at a ten second beat is roughly 52,000 writes a day against a six-row table, and that churn is what turned a blocked autovacuum into a 460 MB table whose six-row scan cost 867 ms and timed out the queries that place models. A beat carrying only a fresher timestamp now waits for the checkpoint interval. Each reported field is compared against the value last persisted rather than tested for presence, because a worker sends its disk figures on every beat and presence alone would suppress nothing. A node's first beat, a changed total VRAM, total disk or GPU vendor, and a free VRAM, RAM or disk reading that has moved more than 256 MiB from the persisted value all still write at once. A node that is not active is never suppressed, because it recovers only when the health monitor sees a fresh timestamp. The persisted column is up to one interval stale by design, so the stale-node threshold moves from 60s to 5m to cover it. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
…hable The readiness gate tracked only the NATS link, so a worker whose backend processes had died still answered /readyz with 200 and kept receiving loads. One node did exactly that during an incident: it reported healthy while its backend port refused connections, and every load routed to it failed. Readiness is now the NATS link and, for each backend process the worker believes it is running, a short dial of its recorded address. A worker holding no backends stays ready, because idle is a healthy state. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
A backend process is inserted into the supervisor map with its gRPC address already recorded, but the address refuses connections until the gRPC server binds, which the startup poll allows up to 30 seconds for and which takes 10 to 15 seconds on a slow node. The new data-path readiness probe dialled that address straight away, so a worker answered /readyz with 503 for the whole of every cold backend start. The container HEALTHCHECK absorbs that, but a Kubernetes readinessProbe at 10s does not, and the worker would leave rotation each time it loaded a model. The skip for a stopping process had no counterpart at the other end of the lifecycle. Backend processes now carry a serving flag, set where the startup health-check gate succeeds, and the probe dials only processes that are serving and not yet stopping. backendStartStillValid becomes markBackendServing: the check and the mark must share one lock hold, so the flag can only ever land on the entry the key currently owns. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Four transactions wedged on a corrupt index held the vacuum horizon open for 42 days. Nothing measured it, so the first symptom anyone saw was models failing to load six weeks later, by which time a six-row table had grown to 460 MB. Export the oldest xmin age, the longest open transaction, and the dead tuple ratio on the registry tables. The first is the number that would have caught it: it sits near zero in health and was 21,002,291. Sampling is scrape-driven behind a cache, and a failed sample reports the last good values rather than failing the scrape, because these gauges matter most when the database is already struggling. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
The cache advanced its clock only on a successful sample, so once the database started failing every scrape retried the query immediately. That turned the cache off in the one regime it exists for: a retry storm at scrape cadence aimed at a database already in trouble. A catalog read that consistently exceeds the 5 second timeout also paid that cost on every scrape, with all scrapes serialised behind the sampler mutex. Time every attempt rather than every success, so failures and timeouts cost the same interval as good samples. Whether a good sample exists moves to its own field, keeping the gauges absent until the first success and holding the last good values through later failures. Also note in the runbook that pg_stat_activity cannot see prepared transactions or replication slot xmins, so a healthy-looking xmin age does not by itself rule out a blocked horizon. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Exercises the real distributed stack against a control-plane database that refuses the router's slot lookup, and asserts the scheduler reports the lookup it could not answer instead of falling through to eviction. The failure is injected with privileges rather than a statement timeout. A timeout set with ALTER DATABASE also breaks AutoMigrate, and it leaks into every later spec in the suite unless it is reset, so the spec would end up testing the migration rather than the scheduler. Instead the spec creates a dedicated login role, points a second gorm handle at it, and revokes that role's SELECT on node_models.replica_index. This has to be a separate role: the test container's owner is a PostgreSQL superuser, and superusers bypass every privilege check, so revoking from CURRENT_USER is recorded and then ignored. The revoke is scoped to one column on purpose. Revoking the whole table would also blind node selection, which runs first and has a guard of its own, so the scheduler would never reach the slot lookup this spec is about. Leaving every other column readable lets selection succeed and lands the refusal exactly on NextFreeReplicaIndex, which plucks replica_index. The grant is restored from BeforeEach via DeferCleanup, so a failing assertion or a panic cannot hand the next spec a role that cannot read. Reverting the eviction guard fails this spec, which is the point of it: the router then reports "no replica slot on keeper and eviction failed" for an error that was never evidence the node was full. The surviving-row assertions are secondary under this injection, because the eviction path reads whole node_models rows and the same revoke blinds it too; a comment in the spec says so, so nobody mistakes them for the load-bearing ones. Also documents why the vector store and the control plane must not share a database: the removable-tuple cutoff is per database, not per table, so one transaction left open anywhere stops autovacuum reclaiming the node registry, and a six-row table bloats into hundreds of megabytes. The note names LOCALAI_AUTH_DATABASE_URL and LOCALAI_AGENT_POOL_DATABASE_URL as the two knobs that must differ, and the localai_control_plane_oldest_xmin_age gauge as the way to see it coming. grep for StaleNodeThreshold and HealthCheckInterval in core/config/runtime_settings_registry.go returns no matches: the distributed duration knobs are not exposed as runtime settings, so the new heartbeat checkpoint interval follows them and needs no registry entry. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
The stale-node threshold moved from 60 seconds to 5 minutes in this branch because checkpointing makes last_heartbeat up to one checkpoint interval behind by design. Two things were left inconsistent with that. NewHealthMonitor still fell back to a hardcoded 60 seconds when handed a zero threshold, so any future caller that stopped passing the configured value would mark every healthy, beating node offline on every cycle. And the threshold itself had a flag-name constant but no AppOption, no CLI field and no env binding, so an operator who widened --node-heartbeat-checkpoint had no way to widen the threshold to match. The fallback now tracks config.DefaultStaleNodeThreshold, and --stale-node-threshold / LOCALAI_STALE_NODE_THRESHOLD is wired the same way its sibling is. Heartbeat suppression compared the RAW reported free VRAM against the snapshot, but the column persists capAvailable(raw, ceiling). On any node with a VRAM budget set, whose actual free VRAM oscillates above that ceiling, every beat looked material while the persisted value never moved: suppression was defeated on exactly the nodes an operator had configured, and the write amplification this branch exists to remove came straight back there. The comparison and the snapshot now both hold the capped figure, so they measure the same quantity as the column. Fixing that needs the ceiling, and reading it cost a SELECT on every beat, including suppressed ones. The skip decision therefore moved ahead of the updates map and now reuses the ceiling cached on the last durable write, while the write path still re-reads it before capping anything. A ceiling that changed inside the checkpoint window can cost one extra or one late write; it cannot persist a wrong figure. A suppressed beat now costs no query at all. Also: the operations section now says to grant pg_read_all_stats to the LocalAI role, because PostgreSQL blanks backend_xmin and xact_start for sessions owned by other roles, and the transaction that wedged the horizon in the incident was a co-located vector store connecting as a different role, so without the grant the new gauge sees only our own sessions. The compose healthcheck comment now describes readiness covering the backend data path, and the control-plane gauge registration records the otel.SetMeterProvider ordering it depends on. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
The dead-tuple gauge queried pg_stat_user_tables against a hardcoded list of three table names. Those three do not agree on where their name comes from: BackendNode and NodeModel take gorm's default pluralisation, while GalleryOperationRecord overrides TableName, and gallery_operations already had a constant of its own that the list duplicated. A literal list keeps compiling after any of that moves, and the query then matches nothing. The failure is silent and it points the wrong way: a dead-tuple ratio that matched no rows reports the same numbers as a cluster with no bloat, so the gauge would look healthiest exactly when it had stopped working. Ask gorm what each model is stored as instead, which follows a TableName override and the default pluralisation alike. A spec pins that the override really is consulted: naive pluralisation of the type would give gallery_operation_records, so the resolution cannot quietly stop asking the model. Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happened
A development cluster stopped loading models and its model replicas thrashed between nodes. The cause was not in the distributed code.
Four inserts had wedged on a corrupt bm25 index in a LocalRecall vector store that shared a PostgreSQL database with the control plane, and they held an open transaction for 42 days. PostgreSQL computes the removable-tuple cutoff per database, so autovacuum could reclaim nothing:
backend_nodesreached 1,348,037 dead tuples for 6 live rows and grew to 460 MB, and a six-row scan cost 58,537 buffers and 867 ms.Nothing in the control plane was broken. It had a slow database. Per hour, across two frontends: 89
context canceled, 32 lock timeouts, 33 connection-refused, 41 in-flight accounting warnings, 8evicting LRU.Separating the two databases removes that trigger, but does not make the control plane tolerant of a slow database in general. This PR does.
What changed
1. The router evicted a healthy model on any lookup error.
scheduleNewModelasked the registry for a free replica slot and treated every error as "this node is full", so a database timeout evicted a loaded model belonging to another request. The evicted process died, a peer frontend still holding its address dialled the dead port, both retried, and the model thrashed. The comment on the branch already said it meant a full node; the code never tested for it. Now onlyErrNoFreeSlotjustifies eviction. An audit of the same shape found a second instance in node selection, fixed the same way.2. Heartbeats no longer write on every beat. Six nodes at a ten second beat is roughly 52,000 UPDATEs a day against a six-row table, and that churn is what turned a blocked autovacuum into an outage. A beat that carries only a fresher timestamp now waits for a checkpoint interval; a first beat, a changed total VRAM, total disk or GPU vendor, and a free VRAM/RAM/disk reading that moves more than 256 MiB still write at once, and a node that is not active is never suppressed. Because the persisted column is now up to one interval stale by design,
stale-node-thresholdmoves from 60s to 5m, and it is now settable (it previously had no binding at all).3. Worker readiness covers the data path.
/readyztracked only the NATS link, so a worker whose backend processes had died still reported ready and kept receiving loads. One node did exactly that: it answered 200 while its backend port refused connections. Readiness is now the NATS link plus a short dial of each backend the worker believes it is running. An idle worker holding no backends stays ready, and a backend that is still starting is excluded so a cold start does not read as a fault.4. The database health gauges that would have caught this.
localai_control_plane_oldest_xmin_agesits near zero when healthy and was 21,002,291 during the incident. Plus the longest open transaction and the dead-tuple ratio on the registry tables. Sampling is scrape-driven behind a cache, a failed sample returns the last good values rather than failing the scrape, and the gauges are absent until the first successful sample so a down database cannot publish the healthy value.5. Docs record why the vector store and the control plane must not share a database, so the co-location is not reintroduced.
Behaviour changes for operators
--stale-node-thresholdis now settable, and must exceed--node-heartbeat-checkpoint. This is documented and pinned by a config spec, but is not validated at startup. See the follow-ups./readyzsemantics changed: a worker holding an unreachable backend now reports 503./metrics. Note thatpg_stat_activityhidesbackend_xminfrom other roles' backends, so the LocalAI role needspg_read_all_statsor the gauge sees only its own sessions. Documented.Reading the commits
The commits tell one story in order, with two deliberate fix-up pairs found by review:
64d54008is corrected byda1cac09(a starting backend must not read as unreachable), and47edb5b6by2d6b7ff9(rate-limit failed samples). Squash-merging is recommended, since a bisect landing between a pair sees a known-transient state.How to verify
The load-bearing regression is
core/services/nodes/router_slot_uncertainty_test.go: revert theerrors.Is(slotErr, ErrNoFreeSlot)guard and it fails, which is the point of it. The end-to-end spectests/e2e/distributed/db_latency_resilience_test.gowas verified the same way, and states in its own comments which of its assertions carry the regression and which are secondary under its injection.Known follow-ups, deliberately not in this PR
stale-node-threshold > node-heartbeat-checkpointat startup. A hard failure would be wrong (the coupling is a spectrum, and booting to a fatal error over a tunable is worse than the flap it prevents); a warning beside the existing registration-token warn is the minimal fix.ResetVRAMBudgetdoes not re-capavailable_vram, so that one case waits a full checkpoint interval to self-heal rather than immediately.Assisted-by: Claude:claude-opus-5 golangci-lint ginkgo
🤖 Generated with Claude Code
https://claude.ai/code/session_01DWTXixqX2PRE76YQDHy7rs