diff --git a/core/http/endpoints/localai/backend.go b/core/http/endpoints/localai/backend.go index 9381405fe..f699c7d3b 100644 --- a/core/http/endpoints/localai/backend.go +++ b/core/http/endpoints/localai/backend.go @@ -140,12 +140,12 @@ func (mgs *BackendEndpointService) ApplyBackendEndpoint(systemState *system.Syst if err != nil { return err } - mgs.backendApplier.BackendGalleryChannel <- galleryop.ManagementOp[gallery.GalleryBackend, any]{ + mgs.backendApplier.EnqueueBackendOp(galleryop.ManagementOp[gallery.GalleryBackend, any]{ ID: uuid.String(), GalleryElementName: input.ID, Galleries: mgs.galleries, Force: input.Force, - } + }) return c.JSON(200, schema.BackendResponse{ID: uuid.String(), StatusURL: fmt.Sprintf("%sbackends/jobs/%s", middleware.BaseURL(c), uuid.String())}) } @@ -221,17 +221,21 @@ func (mgs *BackendEndpointService) DeleteBackendEndpoint() echo.HandlerFunc { return func(c echo.Context) error { backendName := c.Param("name") - mgs.backendApplier.BackendGalleryChannel <- galleryop.ManagementOp[gallery.GalleryBackend, any]{ - Delete: true, - GalleryElementName: backendName, - Galleries: mgs.galleries, - } - uuid, err := uuid.NewUUID() if err != nil { return err } + // The op carries the same ID the caller is handed back: without it the + // deletion ran under an empty ID and StatusURL pointed at a job that + // could never have a status. + mgs.backendApplier.EnqueueBackendOp(galleryop.ManagementOp[gallery.GalleryBackend, any]{ + ID: uuid.String(), + Delete: true, + GalleryElementName: backendName, + Galleries: mgs.galleries, + }) + return c.JSON(200, schema.BackendResponse{ID: uuid.String(), StatusURL: fmt.Sprintf("%sbackends/jobs/%s", middleware.BaseURL(c), uuid.String())}) } } @@ -313,12 +317,12 @@ func (mgs *BackendEndpointService) UpgradeBackendEndpoint() echo.HandlerFunc { return err } - mgs.backendApplier.BackendGalleryChannel <- galleryop.ManagementOp[gallery.GalleryBackend, any]{ + mgs.backendApplier.EnqueueBackendOp(galleryop.ManagementOp[gallery.GalleryBackend, any]{ ID: uuid.String(), GalleryElementName: backendName, Galleries: mgs.galleries, Upgrade: true, - } + }) return c.JSON(200, schema.BackendResponse{ID: uuid.String(), StatusURL: fmt.Sprintf("%sbackends/jobs/%s", middleware.BaseURL(c), uuid.String())}) } diff --git a/core/http/endpoints/localai/gallery.go b/core/http/endpoints/localai/gallery.go index fb0a77bb4..61d5d0169 100644 --- a/core/http/endpoints/localai/gallery.go +++ b/core/http/endpoints/localai/gallery.go @@ -87,14 +87,14 @@ func (mgs *ModelGalleryEndpointService) ApplyModelGalleryEndpoint() echo.Handler if err != nil { return err } - mgs.galleryApplier.ModelGalleryChannel <- galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + mgs.galleryApplier.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ Req: input.GalleryModel, ID: uuid.String(), GalleryElementName: input.ID, Variant: input.Variant, Galleries: mgs.galleries, BackendGalleries: mgs.backendGalleries, - } + }) return c.JSON(200, schema.GalleryResponse{ID: uuid.String(), StatusURL: fmt.Sprintf("%smodels/jobs/%s", middleware.BaseURL(c), uuid.String())}) } @@ -110,18 +110,22 @@ func (mgs *ModelGalleryEndpointService) DeleteModelGalleryEndpoint() echo.Handle return func(c echo.Context) error { modelName := c.Param("name") - mgs.galleryApplier.ModelGalleryChannel <- galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ - Delete: true, - GalleryElementName: modelName, - } - - mgs.configLoader.RemoveModelConfig(modelName) - uuid, err := uuid.NewUUID() if err != nil { return err } + // The op carries the same ID the caller is handed back: without it the + // deletion ran under an empty ID and StatusURL pointed at a job that + // could never have a status. + mgs.galleryApplier.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + ID: uuid.String(), + Delete: true, + GalleryElementName: modelName, + }) + + mgs.configLoader.RemoveModelConfig(modelName) + return c.JSON(200, schema.GalleryResponse{ID: uuid.String(), StatusURL: fmt.Sprintf("%smodels/jobs/%s", middleware.BaseURL(c), uuid.String())}) } } diff --git a/core/http/endpoints/localai/import_model.go b/core/http/endpoints/localai/import_model.go index 21b7673da..b3cf491eb 100644 --- a/core/http/endpoints/localai/import_model.go +++ b/core/http/endpoints/localai/import_model.go @@ -108,7 +108,7 @@ func ImportModelURIEndpoint(cl *config.ModelConfigLoader, appConfig *config.Appl opcache.Set(galleryID, uuid.String()) } - galleryService.ModelGalleryChannel <- galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + galleryService.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ Req: gallery.GalleryModel{ Overrides: map[string]any{}, }, @@ -116,7 +116,7 @@ func ImportModelURIEndpoint(cl *config.ModelConfigLoader, appConfig *config.Appl GalleryElementName: galleryID, GalleryElement: &modelConfig, BackendGalleries: appConfig.BackendGalleries, - } + }) resp.ID = uuid.String() resp.StatusURL = fmt.Sprintf("%smodels/jobs/%s", httpUtils.BaseURL(c), uuid.String()) diff --git a/core/http/endpoints/localai/nodes.go b/core/http/endpoints/localai/nodes.go index f4abbf303..bb9fc0225 100644 --- a/core/http/endpoints/localai/nodes.go +++ b/core/http/endpoints/localai/nodes.go @@ -521,9 +521,7 @@ func InstallBackendOnNodeEndpoint(_ nodes.NodeCommandSender, galleryService *gal CancelFunc: cancelFunc, } galleryService.StoreCancellation(jobID, cancelFunc) - go func() { - galleryService.BackendGalleryChannel <- op - }() + galleryService.EnqueueBackendOp(op) xlog.Info("Node-scoped backend install dispatched", "node", nodeID, "backend", req.Backend, "uri", req.URI, "jobID", jobID) return c.JSON(http.StatusAccepted, map[string]string{ @@ -586,9 +584,7 @@ func UpgradeBackendOnNodeEndpoint(galleryService *galleryop.GalleryService, opca CancelFunc: cancelFunc, } galleryService.StoreCancellation(jobID, cancelFunc) - go func() { - galleryService.BackendGalleryChannel <- op - }() + galleryService.EnqueueBackendOp(op) xlog.Info("Node-scoped backend upgrade dispatched", "node", nodeID, "backend", req.Backend, "jobID", jobID) return c.JSON(http.StatusAccepted, map[string]string{ diff --git a/core/http/routes/ui_api.go b/core/http/routes/ui_api.go index 2cf958fcc..4b2bb7dde 100644 --- a/core/http/routes/ui_api.go +++ b/core/http/routes/ui_api.go @@ -986,9 +986,7 @@ func RegisterUIAPIRoutes(app *echo.Echo, cl *config.ModelConfigLoader, ml *model } // Store cancellation function immediately so queued operations can be cancelled galleryService.StoreCancellation(uid, cancelFunc) - go func() { - galleryService.ModelGalleryChannel <- op - }() + galleryService.EnqueueModelOp(op) return c.JSON(200, map[string]any{ "jobID": uid, @@ -1035,10 +1033,8 @@ func RegisterUIAPIRoutes(app *echo.Echo, cl *config.ModelConfigLoader, ml *model } // Store cancellation function immediately so queued operations can be cancelled galleryService.StoreCancellation(uid, cancelFunc) - go func() { - galleryService.ModelGalleryChannel <- op - cl.RemoveModelConfig(galleryName) - }() + galleryService.EnqueueModelOp(op) + cl.RemoveModelConfig(galleryName) return c.JSON(200, map[string]any{ "jobID": uid, @@ -1444,9 +1440,7 @@ func RegisterUIAPIRoutes(app *echo.Echo, cl *config.ModelConfigLoader, ml *model } // Store cancellation function immediately so queued operations can be cancelled galleryService.StoreCancellation(uid, cancelFunc) - go func() { - galleryService.BackendGalleryChannel <- op - }() + galleryService.EnqueueBackendOp(op) return c.JSON(200, map[string]any{ "jobID": uid, @@ -1508,9 +1502,7 @@ func RegisterUIAPIRoutes(app *echo.Echo, cl *config.ModelConfigLoader, ml *model } // Store cancellation function immediately so queued operations can be cancelled galleryService.StoreCancellation(uid, cancelFunc) - go func() { - galleryService.BackendGalleryChannel <- op - }() + galleryService.EnqueueBackendOp(op) return c.JSON(200, map[string]any{ "jobID": uid, @@ -1556,9 +1548,7 @@ func RegisterUIAPIRoutes(app *echo.Echo, cl *config.ModelConfigLoader, ml *model } // Store cancellation function immediately so queued operations can be cancelled galleryService.StoreCancellation(uid, cancelFunc) - go func() { - galleryService.BackendGalleryChannel <- op - }() + galleryService.EnqueueBackendOp(op) return c.JSON(200, map[string]any{ "jobID": uid, @@ -1677,11 +1667,7 @@ func RegisterUIAPIRoutes(app *echo.Echo, cl *config.ModelConfigLoader, ml *model } // Store cancellation function immediately so queued operations can be cancelled galleryService.StoreCancellation(uid, cancelFunc) - // Non-blocking send — BackendGalleryChannel is unbuffered and a direct - // send would hang the HTTP handler whenever the worker is busy. - go func() { - galleryService.BackendGalleryChannel <- op - }() + galleryService.EnqueueBackendOp(op) return c.JSON(200, map[string]any{ "jobID": uid, diff --git a/core/services/distributed/gallery.go b/core/services/distributed/gallery.go index de213c4f5..1c60ab3b4 100644 --- a/core/services/distributed/gallery.go +++ b/core/services/distributed/gallery.go @@ -203,6 +203,20 @@ func (s *GalleryStore) Cancel(id string) error { return s.UpdateStatus(id, "cancelled", "") } +// ListStale returns the IDs of in-progress operations that CleanStale would +// reap. Callers need the IDs, not just a count: the reaper corrects the +// database, but each replica also holds an in-memory copy of the operation +// status that the API actually serves, and that copy has to be corrected too +// or the reaped op keeps reporting "downloading" forever. +func (s *GalleryStore) ListStale(age time.Duration) ([]string, error) { + cutoff := time.Now().Add(-age) + var ids []string + err := s.db.Model(&GalleryOperationRecord{}). + Where("updated_at < ? AND status IN ?", cutoff, activeStatuses). + Pluck("id", &ids).Error + return ids, err +} + // CleanStale marks abandoned in-progress operations as failed and returns the // number of rows reaped. Called on startup AND periodically to recover from // crashed/restarted instances that left records in pending/downloading/ diff --git a/core/services/galleryop/enqueue.go b/core/services/galleryop/enqueue.go new file mode 100644 index 000000000..13adbb9be --- /dev/null +++ b/core/services/galleryop/enqueue.go @@ -0,0 +1,106 @@ +package galleryop + +import ( + "context" + "fmt" + + "github.com/mudler/LocalAI/core/gallery" + "github.com/mudler/xlog" +) + +// queuedMessage is the status message an operation carries between admission +// (the HTTP handler minting the job ID) and the moment the worker starts it. +const queuedMessage = "queued" + +// EnqueueModelOp admits a model operation: it registers a queryable "queued" +// status for op.ID and then hands the op to the gallery worker. +// +// Why the status is written here and not in modelHandler: the gallery channels +// are unbuffered and the worker processes one operation at a time, so an op +// submitted while another install is downloading sits in a blocked send for as +// long as that install takes. The admission handlers return HTTP 200 with the +// job ID immediately, so for that whole window the client held an ID that +// GET /models/jobs/ answered with "could not find any status for ID" and +// GET /models/jobs did not list at all — the endpoint reported success for work +// nothing could observe. Writing the status before the send makes the job +// queryable from the instant its ID is handed out. +// +// Delivery is asynchronous on purpose: a direct send would block the HTTP +// handler for the entire duration of the in-flight install (observed as a +// request that never gets a response while /readyz stays green). +func (g *GalleryService) EnqueueModelOp(op ManagementOp[gallery.GalleryModel, gallery.ModelConfig]) { + g.markQueued(op.ID, op.GalleryElementName, op.Delete) + go func() { + select { + case g.ModelGalleryChannel <- op: + case <-enqueueContext(op.Context).Done(): + g.abandonQueued(op.ID, op.GalleryElementName) + } + }() +} + +// EnqueueBackendOp is the BackendGalleryChannel sibling of EnqueueModelOp. +// Same rationale — see that comment. +func (g *GalleryService) EnqueueBackendOp(op ManagementOp[gallery.GalleryBackend, any]) { + g.markQueued(op.ID, op.GalleryElementName, op.Delete) + go func() { + select { + case g.BackendGalleryChannel <- op: + case <-enqueueContext(op.Context).Done(): + g.abandonQueued(op.ID, op.GalleryElementName) + } + }() +} + +// enqueueContext gives the delivery goroutine something to select on. Ops +// submitted without a context (the /models/apply path) simply wait for the +// worker; ops that carry one (every UI path) are released the moment the +// operation is cancelled, so cancelling a still-queued op no longer strands +// the goroutine on a send that will never be received. +func enqueueContext(ctx context.Context) context.Context { + if ctx == nil { + return context.Background() + } + return ctx +} + +// markQueued publishes the pre-worker status for an admitted operation. +func (g *GalleryService) markQueued(id, elementName string, deletion bool) { + if id == "" { + return + } + g.UpdateStatus(id, &OpStatus{ + Message: queuedMessage, + Phase: queuedMessage, + GalleryElementName: elementName, + Deletion: deletion, + Cancellable: !deletion, + }) +} + +// abandonQueued turns a queued operation that the worker never accepted into a +// terminal failure. Without it the op keeps claiming it is waiting to start +// while the goroutine that was supposed to deliver it is gone, which is the +// same phantom-operation shape as an op orphaned by a replica restart. +// +// A status that already reached a terminal state (a concurrent cancel is the +// common case) wins: we must not overwrite "cancelled" with a generic failure. +func (g *GalleryService) abandonQueued(id, elementName string) { + if id == "" { + return + } + g.Lock() + if st, ok := g.statuses[id]; ok && st.Processed { + g.Unlock() + return + } + g.Unlock() + + xlog.Warn("Gallery operation was never picked up by the worker", "op_id", id, "element", elementName) + g.UpdateStatus(id, &OpStatus{ + Processed: true, + Error: fmt.Errorf("operation was cancelled before the gallery worker could start it"), + Message: "error: operation was cancelled before the gallery worker could start it", + GalleryElementName: elementName, + }) +} diff --git a/core/services/galleryop/enqueue_test.go b/core/services/galleryop/enqueue_test.go new file mode 100644 index 000000000..26d32aaa7 --- /dev/null +++ b/core/services/galleryop/enqueue_test.go @@ -0,0 +1,197 @@ +package galleryop_test + +import ( + "context" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/mudler/LocalAI/core/config" + "github.com/mudler/LocalAI/core/gallery" + "github.com/mudler/LocalAI/core/services/distributed" + "github.com/mudler/LocalAI/core/services/galleryop" + "github.com/mudler/LocalAI/core/services/testutil" +) + +// These specs reproduce the "install endpoint lies about success" bug observed +// on a 2-replica distributed cluster: POST /models/apply answered HTTP 200 with +// a fresh job UUID, but GET /models/jobs/ answered HTTP 500 "could not +// find any status for ID" and GET /models/jobs never listed the job. +// +// The mechanism is that the admission handlers mint the UUID, hand the op to an +// unbuffered channel from a detached goroutine, and return 200 immediately. The +// worker is strictly serial, so while a long install is in flight the op sits in +// a blocked send and NOTHING has written a status for it — the first status +// write happens inside modelHandler, i.e. only once the worker actually starts +// the work. A job that is queued behind a running install is therefore +// indistinguishable, over the API, from a job ID that was never issued. +var _ = Describe("GalleryService operation admission", func() { + var svc *galleryop.GalleryService + + BeforeEach(func() { + svc = galleryop.NewGalleryService(&config.ApplicationConfig{}, nil) + }) + + Context("when the worker is busy and cannot accept the op yet", func() { + It("still makes the model job queryable straight away", func() { + // No consumer is running: this is exactly the state of the channel + // while the worker is mid-download on a previous op. + svc.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + ID: "job-queued-model", + GalleryElementName: "localai@longcat-video-avatar-1.5", + }) + + Eventually(func() *galleryop.OpStatus { + return svc.GetStatus("job-queued-model") + }, "2s", "10ms").ShouldNot(BeNil(), "a job ID handed to the client must have a status the client can poll") + + Expect(svc.GetAllStatus()).To(HaveKey("job-queued-model")) + st := svc.GetStatus("job-queued-model") + Expect(st.Processed).To(BeFalse()) + Expect(st.GalleryElementName).To(Equal("localai@longcat-video-avatar-1.5")) + }) + + It("still makes the backend job queryable straight away", func() { + svc.EnqueueBackendOp(galleryop.ManagementOp[gallery.GalleryBackend, any]{ + ID: "job-queued-backend", + GalleryElementName: "llama-cpp", + }) + + Eventually(func() *galleryop.OpStatus { + return svc.GetStatus("job-queued-backend") + }, "2s", "10ms").ShouldNot(BeNil()) + }) + }) + + Context("when the op is abandoned before the worker ever accepts it", func() { + It("turns the queued job into a terminal failure instead of leaking silently", func() { + ctx, cancel := context.WithCancel(context.Background()) + svc.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + ID: "job-abandoned", + GalleryElementName: "localai@some-model", + Context: ctx, + CancelFunc: cancel, + }) + cancel() + + Eventually(func() bool { + st := svc.GetStatus("job-abandoned") + return st != nil && st.Processed + }, "2s", "10ms").Should(BeTrue(), "an op that never reached the worker must not stay 'queued' forever") + }) + }) + + Context("when the worker is draining the channel", func() { + It("delivers the op to the worker", func() { + received := make(chan string, 1) + go func() { + op := <-svc.ModelGalleryChannel + received <- op.ID + }() + + svc.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + ID: "job-delivered", + GalleryElementName: "localai@some-model", + }) + + Eventually(received, "2s").Should(Receive(Equal("job-delivered"))) + }) + }) +}) + +// This spec covers the orphaned-op half of the report: a controller replaced +// mid-download left an op reporting phase=downloading / processed=false / +// error=none indefinitely while nothing was downloading. The PostgreSQL-side +// duplicate guard does time out (FindDuplicate ignores rows not updated for 30 +// minutes, and CleanStale marks them failed), but the reaper only ever touched +// the database — the in-memory statuses map that GET /models/jobs/ and +// /api/operations actually read was never corrected, so every replica kept +// serving the frozen "downloading" status forever. +var _ = Describe("GalleryService.ReapStaleOperations", func() { + It("marks the reaped operation failed in memory, not just in the store", func() { + db := testutil.SetupTestDB() + store, err := distributed.NewGalleryStore(db) + Expect(err).ToNot(HaveOccurred()) + + svc := galleryop.NewGalleryService(&config.ApplicationConfig{}, nil) + svc.SetGalleryStore(store) + + Expect(store.Create(&distributed.GalleryOperationRecord{ + ID: "orphaned-op", + GalleryElementName: "localai@longcat-video-avatar-1.5", + OpType: "model_install", + Status: "downloading", + Cancellable: true, + })).To(Succeed()) + + // The in-memory view the API serves: frozen mid-download. + svc.UpdateStatus("orphaned-op", &galleryop.OpStatus{ + Message: "downloading", + Phase: "downloading", + Progress: 13.8, + Cancellable: true, + GalleryElementName: "localai@longcat-video-avatar-1.5", + }) + + // Age the row past the reap horizon. Raw SQL so gorm's autoUpdateTime + // does not stamp updated_at back to now. + Expect(db.Exec("UPDATE gallery_operations SET updated_at = ? WHERE id = ?", + time.Now().Add(-2*time.Hour), "orphaned-op").Error).To(Succeed()) + + n, err := svc.ReapStaleOperations(30 * time.Minute) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(BeNumerically("==", 1)) + + st := svc.GetStatus("orphaned-op") + Expect(st).ToNot(BeNil()) + Expect(st.Processed).To(BeTrue(), "a reaped op must stop claiming it is still downloading") + Expect(st.Error).To(HaveOccurred()) + Expect(st.Cancellable).To(BeFalse()) + }) +}) + +// The worker is a single goroutine consuming both gallery channels serially, +// so anything that takes it down takes every queued operation with it. A panic +// inside one install handler used to propagate out of that goroutine and kill +// the whole process; contained, it must fail only the operation that caused it +// and leave the consumer able to pick up the next one. +type panickingModelManager struct{} + +func (panickingModelManager) InstallModel(_ context.Context, _ *galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig], _ galleryop.ProgressCallback) error { + panic("boom: malformed gallery entry") +} + +func (panickingModelManager) DeleteModel(string) error { return nil } + +var _ = Describe("GalleryService worker resilience", func() { + It("keeps consuming operations after a handler panics", func() { + svc := galleryop.NewGalleryService(&config.ApplicationConfig{}, nil) + svc.SetModelManager(panickingModelManager{}) + ctx, cancel := context.WithCancel(context.Background()) + DeferCleanup(cancel) + + Expect(svc.Start(ctx, nil, nil)).To(Succeed()) + + svc.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + ID: "job-panics", + GalleryElementName: "localai@exploding-entry", + }) + + Eventually(func() bool { + st := svc.GetStatus("job-panics") + return st != nil && st.Processed && st.Error != nil + }, "5s", "20ms").Should(BeTrue(), "the panicking op must be reported as failed") + + // The consumer must still be alive for the next operation. + svc.EnqueueModelOp(galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig]{ + ID: "job-after-panic", + GalleryElementName: "localai@another-entry", + }) + + Eventually(func() bool { + st := svc.GetStatus("job-after-panic") + return st != nil && st.Processed && st.Error != nil + }, "5s", "20ms").Should(BeTrue(), "a later op must still be picked up by the worker") + }) +}) diff --git a/core/services/galleryop/service.go b/core/services/galleryop/service.go index 771e22b9a..9e62e3343 100644 --- a/core/services/galleryop/service.go +++ b/core/services/galleryop/service.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "runtime/debug" "sync" "time" @@ -319,6 +320,12 @@ func (g *GalleryService) ReapStaleOperations(age time.Duration) (int64, error) { if store == nil { return 0, nil } + // Collect the IDs before the update: once CleanStale flips them to + // "failed" they no longer match the stale predicate. + staleIDs, err := store.ListStale(age) + if err != nil { + xlog.Warn("Failed to list stale gallery operations", "error", err) + } n, err := store.CleanStale(age) if err != nil { return 0, err @@ -326,9 +333,43 @@ func (g *GalleryService) ReapStaleOperations(age time.Duration) (int64, error) { if n > 0 { xlog.Info("Reaped stale gallery operations", "count", n) } + // The database row is only half the picture. GET /models/jobs/ and + // /api/operations read the in-memory statuses map, which is populated + // locally and via the NATS progress broadcast and never expires. An op + // orphaned by a replica that died mid-download therefore kept serving its + // last frozen tick (phase=downloading, processed=false, error=none) on + // every replica indefinitely, long after the reaper had already given up + // on the row. Reconcile the in-memory copy with the reap. + for _, id := range staleIDs { + g.failStaleStatus(id) + } return n, nil } +// failStaleStatus flips a locally-cached in-progress status to a terminal +// failure after its store row was reaped. Statuses that already reached a +// terminal state are left alone so a genuine completion or cancellation that +// raced the reaper is not rewritten as a failure. +func (g *GalleryService) failStaleStatus(id string) { + g.Lock() + st, ok := g.statuses[id] + if !ok || st == nil || st.Processed { + g.Unlock() + return + } + elementName := st.GalleryElementName + g.Unlock() + + xlog.Warn("Marking orphaned gallery operation as failed", "op_id", id, "element", elementName) + g.UpdateStatus(id, &OpStatus{ + Processed: true, + Error: errors.New("stale operation reaped (abandoned by a crashed or restarted instance)"), + Message: "error: stale operation reaped (abandoned by a crashed or restarted instance)", + GalleryElementName: elementName, + Cancellable: false, + }) +} + // CancelOperation cancels an in-progress operation by its ID. // // In distributed mode the UI's cancel click may land on a different replica @@ -464,6 +505,25 @@ func (g *GalleryService) removeCancellation(id string) { delete(g.cancellations, id) } +// runOpHandler runs one operation handler and converts a panic into an error. +// +// The gallery worker is a single goroutine consuming both channels serially. A +// panic anywhere in an install handler (a malformed gallery entry, a nil +// dereference in a backend-specific path) took down the entire process with it, +// and every queued operation went with it. Containing the panic to the +// operation that caused it keeps the consumer alive so subsequent operations +// are still picked up, and surfaces the failure on the op itself instead of as +// an unexplained restart. +func runOpHandler(fn func() error) (err error) { + defer func() { + if r := recover(); r != nil { + xlog.Error("Gallery operation handler panicked", "panic", r, "stack", string(debug.Stack())) + err = fmt.Errorf("gallery operation handler panicked: %v", r) + } + }() + return fn() +} + func (g *GalleryService) Start(c context.Context, cl *config.ModelConfigLoader, systemState *system.SystemState) error { // updates the status with an error var updateError func(id string, e error) @@ -492,15 +552,21 @@ func (g *GalleryService) Start(c context.Context, cl *config.ModelConfigLoader, } // Create DB record for distributed tracking if g.galleryStore != nil { - g.galleryStore.Create(&distributed.GalleryOperationRecord{ + if err := g.galleryStore.Create(&distributed.GalleryOperationRecord{ ID: op.ID, GalleryElementName: op.GalleryElementName, OpType: "backend_install", Status: "pending", Cancellable: true, - }) + }); err != nil { + // Not fatal: the install still runs and the in-memory + // status still updates. Logged because without the row + // the cross-replica dedup guard and hydration cannot + // see this operation at all. + xlog.Warn("Failed to create gallery operation record", "op_id", op.ID, "error", err) + } } - err := g.backendHandler(&op, systemState) + err := runOpHandler(func() error { return g.backendHandler(&op, systemState) }) if err != nil { updateError(op.ID, err) } else if g.OnBackendOpCompleted != nil { @@ -525,16 +591,18 @@ func (g *GalleryService) Start(c context.Context, cl *config.ModelConfigLoader, if op.Delete { opType = "model_delete" } - g.galleryStore.Create(&distributed.GalleryOperationRecord{ + if err := g.galleryStore.Create(&distributed.GalleryOperationRecord{ ID: op.ID, GalleryElementName: op.GalleryElementName, OpType: opType, Status: "pending", // A delete is not cancellable; an install is. Cancellable: !op.Delete, - }) + }); err != nil { + xlog.Warn("Failed to create gallery operation record", "op_id", op.ID, "error", err) + } } - err := g.modelHandler(&op, cl, systemState) + err := runOpHandler(func() error { return g.modelHandler(&op, cl, systemState) }) if err != nil { updateError(op.ID, err) } diff --git a/docs/content/features/model-gallery.md b/docs/content/features/model-gallery.md index 63510528c..28138f3de 100644 --- a/docs/content/features/model-gallery.md +++ b/docs/content/features/model-gallery.md @@ -666,3 +666,14 @@ Returns a json containing the error, and if the job is being processed: ```json {"error":null,"processed":true,"message":"completed"} ``` + +Installations are processed one at a time. A job submitted while another install +is still running is reported as queued until the installer picks it up: + +```json +{"error":null,"processed":false,"message":"queued","phase":"queued"} +``` + +A job ID is queryable from the moment `/models/apply` returns it, so a `404`/`500` +from this endpoint means the ID is genuinely unknown rather than merely waiting +its turn.