diff --git a/core/services/worker/backend_reinstall_cwd_test.go b/core/services/worker/backend_reinstall_cwd_test.go new file mode 100644 index 000000000..2b41eb52c --- /dev/null +++ b/core/services/worker/backend_reinstall_cwd_test.go @@ -0,0 +1,123 @@ +package worker + +import ( + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// Production incident (Jetson Thor worker, distributed mode): after two +// successive reinstalls of a backend, a later model load failed with +// +// rpc error: code = Internal desc = failed to load LongCat model: [Errno 2] No such file or directory +// +// raised from inside the backend's `import torch`, before any model file was +// touched — torch's custom-op registration calls inspect.getmodule -> +// os.path.abspath, and abspath calls getcwd(2) for a relative path. +// +// gallery.InstallBackend replaces a backend by renaming the live directory to +// `.install-backup`, moving the staged directory into place, then +// deleting the backup. A working directory follows the inode across a rename, +// so a backend process that outlives that swap ends up with a deleted inode as +// its CWD and every getcwd(2) in it fails with ENOENT. Scanning /proc inside +// the worker container found exactly that: +// +// pid 23467 CWD DELETED: /backends/cuda13-nvidia-l4t-arm64-longcat-video-development.install-backup (deleted) +// +// Python backends import torch lazily inside LoadModel, so such a survivor +// looks perfectly healthy — it answers HealthCheck and keeps its gRPC port — +// and only detonates when a model is actually loaded through it. Restarting +// the worker container cleared the condition. +// +// The install paths do stop running processes before replacing the directory +// (installBackend's force branch, upgradeBackend, backend.delete), but they +// resolve them by *name*. That bookkeeping reaps nothing whenever the recorded +// name no longer resolves into the install's identity set: a legacy entry with +// an empty backendName, a ListSystemBackends failure degrading backendIdentity +// to name-only matching, or an earlier reinstall having already rewritten the +// metadata.json that carries the alias. Each of those leaves a live process +// whose directory is about to be unlinked — and nothing downstream notices, +// because the reuse gate checks liveness and name, and the name is precisely +// what does NOT change across a reinstall. +// +// These specs pin the missing invariant: a process may only be reused if the +// directory it is running out of is still the installed one. +var _ = Describe("Backend reinstall must not poison later model loads", func() { + const ( + backendName = "cuda13-nvidia-l4t-arm64-longcat-video-development" + processKey = "LongCat-Video#0" + ) + + var backendDir string + + BeforeEach(func() { + backendDir = filepath.Join(GinkgoT().TempDir(), backendName) + Expect(os.MkdirAll(backendDir, 0o750)).To(Succeed()) + }) + + // newSupervisor mirrors startBackend: it records the backend directory and + // its identity while that directory is still the live one. + newSupervisor := func() *backendSupervisor { + info, err := os.Stat(backendDir) + Expect(err).ToNot(HaveOccurred()) + return &backendSupervisor{ + processes: map[string]*backendProcess{ + processKey: { + addr: "127.0.0.1:30232", + backendName: backendName, + backendDir: backendDir, + backendDirID: info, + }, + }, + } + } + + // reinstall reproduces gallery.InstallBackend's atomic swap verbatim: + // rename the live directory aside, move the staged one into place, delete + // the backup. The path is unchanged afterwards; the inode is not. + reinstall := func() { + backup := backendDir + ".install-backup" + Expect(os.Rename(backendDir, backup)).To(Succeed()) + Expect(os.MkdirAll(backendDir, 0o750)).To(Succeed()) + Expect(os.RemoveAll(backup)).To(Succeed()) + } + + Describe("processMatchesBackend", func() { + It("refuses to reuse a process whose backend directory a reinstall replaced", func() { + s := newSupervisor() + reinstall() + + Expect(s.processMatchesBackend(processKey, backendName)).To(BeFalse(), + "the process is running out of a deleted inode: its CWD no longer resolves, "+ + "so reusing it hands the next load a backend that fails inside import torch") + }) + + It("refuses to reuse a process whose backend directory was removed outright", func() { + s := newSupervisor() + Expect(os.RemoveAll(backendDir)).To(Succeed()) + + Expect(s.processMatchesBackend(processKey, backendName)).To(BeFalse(), + "a backend delete that missed this process leaves it in the same unusable state") + }) + + It("reuses a process whose backend directory is untouched", func() { + Expect(newSupervisor().processMatchesBackend(processKey, backendName)).To(BeTrue(), + "the common case must stay on the fast path") + }) + + It("reuses processes that predate directory recording", func() { + // Entries created before this field exists carry no recorded + // directory. Treating them as mismatched would restart every + // running backend once on rollout. + s := &backendSupervisor{ + processes: map[string]*backendProcess{ + processKey: {addr: "127.0.0.1:30232", backendName: backendName}, + }, + } + + Expect(s.processMatchesBackend(processKey, backendName)).To(BeTrue()) + }) + }) +}) diff --git a/core/services/worker/install.go b/core/services/worker/install.go index 67c5bc2f0..122b5d266 100644 --- a/core/services/worker/install.go +++ b/core/services/worker/install.go @@ -74,12 +74,16 @@ func (s *backendSupervisor) installBackend(req messaging.BackendInstallRequest, if addr := s.getAddr(processKey); addr != "" { switch { case !s.processMatchesBackend(processKey, req.Backend): - // The slot is held by a process started from a DIFFERENT - // backend — typically this model's previous backend was - // deleted (or superseded by a -development variant) while its - // process stayed up. Reusing that address would serve the load - // from a backend directory that may no longer exist on disk. - xlog.Warn("Process for this model replica belongs to another backend; restarting it", + // The slot is held by a process that must not serve this + // backend: either it was started from a DIFFERENT backend + // (this model's previous backend was deleted, or superseded by + // a -development variant, while its process stayed up), or a + // reinstall replaced the directory it is running out of, which + // leaves its working directory a deleted inode. Either way, + // reusing that address serves the load from a backend + // directory that no longer exists. processMatchesBackend + // reports the specific reason. + xlog.Warn("Process for this model replica must not be reused; restarting it", "backend", req.Backend, "model", req.ModelID, "replica", req.ReplicaIndex, "addr", addr) if err := s.stopBackendExact(processKey, false); err != nil { return "", fmt.Errorf("stopping mismatched backend process before reinstall: %w", err) diff --git a/core/services/worker/supervisor.go b/core/services/worker/supervisor.go index dab96e4ab..1e01cf441 100644 --- a/core/services/worker/supervisor.go +++ b/core/services/worker/supervisor.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" "os" + "path/filepath" "slices" "strings" "sync" @@ -33,6 +34,19 @@ type backendProcess struct { // keyed on the backend name resolves to nothing and the process is // orphaned while its files are removed from disk. backendName string + // backendDir is the directory the process was started out of — its + // working directory, and the directory a reinstall replaces. backendDirID + // is that directory's identity at spawn time, compared with os.SameFile so + // a rebuilt directory at the same path is recognised as a different one. + // + // A working directory follows the inode across a rename, so a process that + // outlives gallery.InstallBackend's rename-install-delete swap ends up with + // a deleted inode as its CWD and every getcwd(2) in it fails with ENOENT. + // Recording identity (not just the path) is what lets the reuse gate spot + // such a survivor; matching on backendName alone cannot, because the name + // is exactly what stays the same across a reinstall. + backendDir string + backendDirID os.FileInfo } const workerBackendFreeTimeout = 5 * time.Second @@ -402,6 +416,21 @@ func (s *backendSupervisor) releasePort(port int) { // backend it was started from, recorded so delete/stop can find this process // by backend name later. func (s *backendSupervisor) startBackend(backend, backendName, backendPath string) (string, error) { + // Mirrors the working directory pkg/model.startProcess hands the child. + backendDir := filepath.Dir(backendPath) + + // A process that outlived a reinstall of this backend is still alive and + // still recorded under this key, but its working directory is now a + // deleted inode. The reuse branch below would hand that process straight + // back to the caller; stop it first so the fresh spawn chdirs into the + // newly installed directory. Forced, because a graceful Free() into a + // process whose CWD no longer resolves buys nothing and can hang. + if s.getAddr(backend) != "" && !s.backendDirIntact(backend) { + if err := s.stopBackendExact(backend, true); err != nil { + return "", fmt.Errorf("stopping backend process running from a replaced directory %s: %w", backend, err) + } + } + s.mu.Lock() // Already running? @@ -433,11 +462,25 @@ func (s *backendSupervisor) startBackend(backend, backendName, backendPath strin return "", fmt.Errorf("starting backend process: %w", err) } + // Record the directory this process runs out of, and its identity, while + // it is still the live one. pkg/model.startProcess passes exactly this + // directory as the child's working directory, so it is what a later + // reinstall unlinks out from under this process. A stat failure is not + // fatal: leaving backendDirID nil degrades to today's name-only reuse + // gate rather than refusing to start the backend. + dirInfo, dirErr := os.Stat(backendDir) + if dirErr != nil { + xlog.Warn("Could not record backend directory identity; reuse gate degrades to name matching", + "backend", backend, "dir", backendDir, "error", dirErr) + } + s.processes[backend] = &backendProcess{ - proc: proc, - addr: clientAddr, - port: port, - backendName: backendName, + proc: proc, + addr: clientAddr, + port: port, + backendName: backendName, + backendDir: backendDir, + backendDirID: dirInfo, } xlog.Info("Backend process started", "backend", backend, "addr", clientAddr) @@ -676,6 +719,10 @@ func (s *backendSupervisor) resolveStopTargets(id string) []string { // A process with no recorded backendName predates this field and is accepted: // treating it as a mismatch would restart every running backend once on // rollout. +// +// The name check alone is not sufficient. A reinstall keeps the name and +// replaces the directory, which is the one case name matching cannot see, so +// backendDirIntact gates reuse first. func (s *backendSupervisor) processMatchesBackend(key, backend string) bool { s.mu.Lock() bp, ok := s.processes[key] @@ -686,6 +733,10 @@ func (s *backendSupervisor) processMatchesBackend(key, backend string) bool { recorded := bp.backendName s.mu.Unlock() + if !s.backendDirIntact(key) { + return false + } + if recorded == "" || recorded == backend { return true } @@ -695,6 +746,59 @@ func (s *backendSupervisor) processMatchesBackend(key, backend string) bool { return match } +// backendDirIntact reports whether the process under `key` is still running +// out of the installed backend directory. +// +// gallery.InstallBackend replaces a backend by renaming the live directory to +// `.install-backup`, moving the staged directory into place, then +// deleting the backup. A working directory follows the inode across a rename, +// so any process that outlived that swap now has a deleted inode as its CWD +// and every getcwd(2) in it fails with ENOENT. Python backends import torch +// lazily inside LoadModel, so the survivor still answers HealthCheck and only +// detonates when a model is loaded through it — surfacing as a bare +// `[Errno 2] No such file or directory` from deep inside torch's custom-op +// registration, with nothing pointing at the reinstall that caused it. +// +// The install paths already stop processes by name before replacing the +// directory. This is the backstop for when that bookkeeping misses one — a +// legacy entry with no recorded name, backendIdentity degraded to name-only +// matching because ListSystemBackends failed, or an earlier reinstall having +// rewritten the metadata.json that carries the alias. Comparing the recorded +// directory identity with what is on disk needs none of that bookkeeping to +// have been correct. +// +// A process with no recorded directory predates this field and is accepted, so +// a rollout does not restart every running backend once. +func (s *backendSupervisor) backendDirIntact(key string) bool { + s.mu.Lock() + bp, ok := s.processes[key] + if !ok { + s.mu.Unlock() + return false + } + dir, recorded := bp.backendDir, bp.backendDirID + s.mu.Unlock() + + if dir == "" || recorded == nil { + return true + } + + current, err := os.Stat(dir) + if err != nil { + xlog.Warn("Backend process is running from a directory that no longer exists; it must not be reused", + "processKey", key, "dir", dir, "error", err) + return false + } + // Same path, different inode: the directory was replaced underneath the + // process, which is exactly what a reinstall does. + if !os.SameFile(recorded, current) { + xlog.Warn("Backend directory was replaced under a running process (reinstall); it must not be reused", + "processKey", key, "dir", dir) + return false + } + return true +} + // stopBackend stops the backend process(es) matching the given identifier. // Accepts a bare modelID (stops every replica) or a full processKey // (stops just that replica).