diff --git a/internal/platform/indexqueue/indexqueue_test.go b/internal/platform/indexqueue/indexqueue_test.go index 2e6c16f4..2789c9b7 100644 --- a/internal/platform/indexqueue/indexqueue_test.go +++ b/internal/platform/indexqueue/indexqueue_test.go @@ -54,6 +54,8 @@ func (stubStore) Get(context.Context, int64) (*indexjobs.Job, error) { return nil, indexjobs.ErrNotFound } +func (stubStore) CancelPending(context.Context, indexjobs.Key) (int, error) { return 0, nil } + func (stubStore) List(context.Context, indexjobs.ListFilter) ([]indexjobs.Job, error) { return nil, nil } diff --git a/internal/platform/indexqueue/sources.go b/internal/platform/indexqueue/sources.go index 615d09ef..83233bbe 100644 --- a/internal/platform/indexqueue/sources.go +++ b/internal/platform/indexqueue/sources.go @@ -39,14 +39,18 @@ type catalogSource struct { func (*catalogSource) Kind() string { return catalogindex.SourceKind } // LoadItems decodes the source_id, fetches the spec content, and returns one -// item per operation. A missing spec surfaces as an error (the worker treats it -// as terminal: the spec was deleted). +// item per operation. A missing spec (or catalog) surfaces as an error wrapping +// indexjobs.ErrSourceGone, so the worker resolves the unit — the spec was +// deleted, not broken — instead of recording an open failure (#998). func (s *catalogSource) LoadItems(ctx context.Context, sourceID string) ([]indexjobs.Item, error) { catalogID, specName, ok := catalogindex.DecodeSourceID(sourceID) if !ok { return nil, fmt.Errorf("catalogSource: malformed source_id %q", sourceID) } spec, err := s.store.GetSpec(ctx, catalogID, specName) + if errors.Is(err, apigatewaycatalog.ErrNotFound) { + return nil, fmt.Errorf("catalogSource: %w: %w", indexjobs.ErrSourceGone, err) + } if err != nil { return nil, fmt.Errorf("catalogSource: get spec: %w", err) } diff --git a/internal/platform/indexqueue/sources_test.go b/internal/platform/indexqueue/sources_test.go index 165bb909..f56f0320 100644 --- a/internal/platform/indexqueue/sources_test.go +++ b/internal/platform/indexqueue/sources_test.go @@ -3,10 +3,12 @@ package indexqueue import ( "context" "errors" + "strings" "testing" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/txn2/mcp-data-platform/pkg/indexjobs" "github.com/txn2/mcp-data-platform/pkg/registry" apigatewaykit "github.com/txn2/mcp-data-platform/pkg/toolkits/apigateway" apigatewaycatalog "github.com/txn2/mcp-data-platform/pkg/toolkits/apigateway/catalog" @@ -98,15 +100,39 @@ func TestCatalogSource_LoadItems_MalformedSourceID(t *testing.T) { } } -// TestCatalogSource_LoadItems_MissingSpec proves a vanished spec surfaces as a -// wrapped error; the worker treats it as terminal (the spec was deleted between -// enqueue and claim). +// TestCatalogSource_LoadItems_MissingSpec proves a vanished spec surfaces as an +// error wrapping indexjobs.ErrSourceGone, so the worker resolves the unit +// (the spec was deleted between enqueue and claim) instead of recording an +// open failure nothing can ever supersede (#998). func TestCatalogSource_LoadItems_MissingSpec(t *testing.T) { t.Parallel() s := &catalogSource{store: apigatewaycatalog.NewMemoryStore()} - if _, err := s.LoadItems(context.Background(), catalogindex.EncodeSourceID("missing", "missing")); err == nil { + _, err := s.LoadItems(context.Background(), catalogindex.EncodeSourceID("missing", "missing")) + if err == nil { t.Fatal("expected error for missing spec") } + if !errors.Is(err, indexjobs.ErrSourceGone) { + t.Errorf("missing spec error = %v; must wrap indexjobs.ErrSourceGone", err) + } + if !strings.Contains(err.Error(), `"missing"`) { + t.Errorf("missing spec error = %v; must name the missing spec", err) + } +} + +// TestCatalogSource_LoadItems_StoreErrorIsNotGone pins the boundary of the +// gone signal: an unreadable store is NOT a deleted source, so the error must +// not wrap ErrSourceGone (the worker would silently resolve a unit that still +// exists). +func TestCatalogSource_LoadItems_StoreErrorIsNotGone(t *testing.T) { + t.Parallel() + s := &catalogSource{store: &errStore{err: errors.New("db down")}} + _, err := s.LoadItems(context.Background(), catalogindex.EncodeSourceID("c", "s")) + if err == nil { + t.Fatal("expected store error to surface") + } + if errors.Is(err, indexjobs.ErrSourceGone) { + t.Errorf("store error = %v; must NOT wrap ErrSourceGone", err) + } } // TestCatalogSource_LoadItems_ParseError proves malformed spec content surfaces diff --git a/internal/testdb/testdb.go b/internal/testdb/testdb.go index 021c150b..93354fa6 100644 --- a/internal/testdb/testdb.go +++ b/internal/testdb/testdb.go @@ -49,15 +49,22 @@ func NewWithDSN(t *testing.T) (*sql.DB, string) { } ctx := context.Background() + // The wait strategy requires BOTH the second "ready" log line (postgres + // restarts once during init) AND the mapped port to be externally + // reachable. The log line alone raced under parallel container load: + // ConnectionString could run before Docker exposed the port mapping and + // fail with `port "5432/tcp" not found`. container, err := tcpostgres.Run(ctx, "pgvector/pgvector:pg16", tcpostgres.WithDatabase("testdb"), tcpostgres.WithUsername("test"), tcpostgres.WithPassword("test"), testcontainers.WithWaitStrategy( - wait.ForLog("database system is ready to accept connections"). - WithOccurrence(2). - WithStartupTimeout(5*time.Minute), + wait.ForAll( + wait.ForLog("database system is ready to accept connections"). + WithOccurrence(2), + wait.ForListeningPort("5432/tcp"), + ).WithStartupTimeoutDefault(5*time.Minute), ), ) if err != nil { diff --git a/pkg/admin/catalog_delete_realdb_integration_test.go b/pkg/admin/catalog_delete_realdb_integration_test.go new file mode 100644 index 00000000..1a78aa11 --- /dev/null +++ b/pkg/admin/catalog_delete_realdb_integration_test.go @@ -0,0 +1,154 @@ +//go:build integration + +package admin + +import ( + "context" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/txn2/mcp-data-platform/internal/testdb" + "github.com/txn2/mcp-data-platform/pkg/indexjobs" + apicatalog "github.com/txn2/mcp-data-platform/pkg/toolkits/apigateway/catalog" + "github.com/txn2/mcp-data-platform/pkg/toolkits/apigateway/catalogindex" +) + +// specBody is a one-operation OpenAPI document the admin spec write +// path accepts, so each PUT enqueues exactly one index job through the +// real producer hook. +const specBody = `openapi: 3.0.0 +info: {title: t, version: "1"} +paths: + /a: + get: + operationId: a + responses: + "200": + description: ok` + +// requireNoResidue asserts the acceptance criteria of #998 for one +// unit: zero pending/running jobs for the key, zero open failures, and +// an api_catalog kind that reports no unresolved failures (the signal +// the portal's Degraded verdict keys on). +func requireNoResidue(t *testing.T, jobs *indexjobs.PostgresStore, catalogID, specName string) { + t.Helper() + ctx := context.Background() + sourceID := catalogindex.EncodeSourceID(catalogID, specName) + + rows, err := jobs.List(ctx, indexjobs.ListFilter{SourceKind: catalogindex.SourceKind, SourceID: sourceID}) + require.NoError(t, err) + for _, j := range rows { + require.NotEqual(t, indexjobs.StatusPending, j.Status, "pending job survived the delete: %+v", j) + require.NotEqual(t, indexjobs.StatusRunning, j.Status, "running job survived the delete: %+v", j) + } + + failures, err := jobs.ActiveFailures(ctx, catalogindex.SourceKind, 50) + require.NoError(t, err) + for _, f := range failures { + require.NotEqual(t, sourceID, f.SourceID, "open failure survived the delete: %+v", f) + } + + counts, err := jobs.Counts(ctx, catalogindex.SourceKind) + require.NoError(t, err) + require.Zero(t, counts.UnresolvedFailures, "api_catalog kind must report healthy after the delete") +} + +// TestCatalogSpecDelete_ClearsIndexResidue_RealDB drives the real +// admin delete handlers against the real Postgres-backed indexjobs +// store (#998): deleting a spec (or its whole catalog) must leave zero +// pending jobs and zero open failures for the affected keys, with no +// operator dismiss. +func TestCatalogSpecDelete_ClearsIndexResidue_RealDB(t *testing.T) { + ctx := context.Background() + db := testdb.New(t) + + catalogStore := apicatalog.NewPostgresStore(db) + jobs := indexjobs.NewPostgresStore(db) + h := NewHandler(Deps{ + APICatalogStore: catalogStore, + EmbedJobs: catalogindex.NewAdminStore(jobs, db), + ConfigStore: &mockConfigStore{mode: "database"}, + DatabaseAvailable: true, + }, nil) + + require.NoError(t, catalogStore.CreateCatalog(ctx, apicatalog.Catalog{ + ID: "cat1", Name: "cat1", DisplayName: "Catalog One", + })) + + putSpec := func(catalogID, specName string) { + t.Helper() + res := doJSON(t, h, http.MethodPut, "/api/v1/admin/api-catalogs/"+catalogID+"/specs/"+specName, map[string]any{ + "source_kind": "inline", + "content": specBody, + }) + require.Equal(t, http.StatusOK, res.Code, "upsert %s/%s: %s", catalogID, specName, res.Body.String()) + } + + t.Run("pending job is canceled by spec delete", func(t *testing.T) { + putSpec("cat1", "alpha") + + pending, err := jobs.List(ctx, indexjobs.ListFilter{ + SourceKind: catalogindex.SourceKind, + SourceID: catalogindex.EncodeSourceID("cat1", "alpha"), + Status: indexjobs.StatusPending, + }) + require.NoError(t, err) + require.Len(t, pending, 1, "spec write must enqueue a pending job") + + res := doJSON(t, h, http.MethodDelete, "/api/v1/admin/api-catalogs/cat1/specs/alpha", nil) + require.Equal(t, http.StatusOK, res.Code, res.Body.String()) + + requireNoResidue(t, jobs, "cat1", "alpha") + }) + + t.Run("open failure is resolved by spec delete", func(t *testing.T) { + putSpec("cat1", "beta") + + job, err := jobs.Claim(ctx, "it-worker") + require.NoError(t, err) + require.Equal(t, catalogindex.EncodeSourceID("cat1", "beta"), job.SourceID) + require.NoError(t, jobs.Fail(ctx, job.ID, "it-worker", "catalogSource: get spec: boom")) + + failures, err := jobs.ActiveFailures(ctx, catalogindex.SourceKind, 50) + require.NoError(t, err) + require.Len(t, failures, 1, "the failed job must surface as an open failure before the delete") + + res := doJSON(t, h, http.MethodDelete, "/api/v1/admin/api-catalogs/cat1/specs/beta", nil) + require.Equal(t, http.StatusOK, res.Code, res.Body.String()) + + requireNoResidue(t, jobs, "cat1", "beta") + }) + + t.Run("catalog delete clears residue for every spec", func(t *testing.T) { + require.NoError(t, catalogStore.CreateCatalog(ctx, apicatalog.Catalog{ + ID: "cat2", Name: "cat2", DisplayName: "Catalog Two", + })) + putSpec("cat2", "gamma") + putSpec("cat2", "delta") + + // Leave gamma's job pending and fail delta's, covering both + // residue shapes in one catalog delete. + var deltaJob *indexjobs.Job + for { + job, err := jobs.Claim(ctx, "it-worker") + if err != nil { + break + } + if job.SourceID == catalogindex.EncodeSourceID("cat2", "delta") { + deltaJob = job + } else { + require.NoError(t, jobs.Retry(ctx, job.ID, "it-worker", "put back")) + } + } + require.NotNil(t, deltaJob, "delta job must be claimable") + require.NoError(t, jobs.Fail(ctx, deltaJob.ID, "it-worker", "catalogSource: get spec: boom")) + + res := doJSON(t, h, http.MethodDelete, "/api/v1/admin/api-catalogs/cat2", nil) + require.Equal(t, http.StatusOK, res.Code, res.Body.String()) + + requireNoResidue(t, jobs, "cat2", "gamma") + requireNoResidue(t, jobs, "cat2", "delta") + }) +} diff --git a/pkg/admin/catalog_handler.go b/pkg/admin/catalog_handler.go index ea57e575..8262be0f 100644 --- a/pkg/admin/catalog_handler.go +++ b/pkg/admin/catalog_handler.go @@ -342,6 +342,10 @@ func (h *Handler) deleteCatalog(w http.ResponseWriter, r *http.Request) { case err != nil: writeError(w, http.StatusInternalServerError, "failed to delete catalog") default: + // WithoutCancel: the delete has committed, so the queue cleanup + // must not die with the request (a client disconnect here would + // otherwise leave an open failure nothing else can resolve). + catalogindex.CancelCatalogBestEffort(context.WithoutCancel(r.Context()), h.deps.EmbedJobs, id) writeJSON(w, http.StatusOK, statusResponse{Status: "deleted"}) } } @@ -466,7 +470,7 @@ func (h *Handler) copyCatalogSpecs(w http.ResponseWriter, r *http.Request, srcID // asynchronously. Without this the cloned spec // would sit at "not indexed" until the periodic // reconciler picked it up. - h.enqueueEmbedJob(r.Context(), dstID, s.SpecName) + catalogindex.EnqueueBestEffort(r.Context(), h.deps.EmbedJobs, dstID, s.SpecName) } } return true @@ -667,7 +671,7 @@ func (h *Handler) upsertCatalogSpec(w http.ResponseWriter, r *http.Request) { writeError(w, h.specErrorStatus(err), "failed to save spec: "+err.Error()) return } - h.enqueueEmbedJob(r.Context(), id, entry.SpecName) + catalogindex.EnqueueBestEffort(r.Context(), h.deps.EmbedJobs, id, entry.SpecName) h.reloadConnectionsForCatalog(id) saved, _ := h.deps.APICatalogStore.GetSpec(r.Context(), id, specName) if saved != nil { @@ -879,7 +883,7 @@ func (h *Handler) uploadCatalogSpec(w http.ResponseWriter, r *http.Request) { writeError(w, h.specErrorStatus(err), "failed to save spec: "+err.Error()) return } - h.enqueueEmbedJob(r.Context(), id, entry.SpecName) + catalogindex.EnqueueBestEffort(r.Context(), h.deps.EmbedJobs, id, entry.SpecName) h.reloadConnectionsForCatalog(id) saved, _ := h.deps.APICatalogStore.GetSpec(r.Context(), id, specName) if saved != nil { @@ -943,7 +947,7 @@ func (h *Handler) refreshCatalogSpec(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusInternalServerError, "failed to save refreshed spec: "+err.Error()) return } - h.enqueueEmbedJob(r.Context(), id, entry.SpecName) + catalogindex.EnqueueBestEffort(r.Context(), h.deps.EmbedJobs, id, entry.SpecName) h.reloadConnectionsForCatalog(id) writeJSON(w, http.StatusOK, h.specToResponseWithEmbedding(r.Context(), id, entry, false)) } @@ -972,6 +976,9 @@ func (h *Handler) deleteCatalogSpec(w http.ResponseWriter, r *http.Request) { case err != nil: writeError(w, http.StatusInternalServerError, "failed to delete spec") default: + // WithoutCancel: same rationale as deleteCatalog — the spec row + // is gone, so the cleanup outlives the request. + catalogindex.CancelBestEffort(context.WithoutCancel(r.Context()), h.deps.EmbedJobs, id, specName) h.reloadConnectionsForCatalog(id) writeJSON(w, http.StatusOK, statusResponse{Status: "deleted"}) } @@ -1054,29 +1061,6 @@ func (h *Handler) specToResponseWithEmbedding(ctx context.Context, catalogID str return resp } -// enqueueEmbedJob is the producer-side hook every spec write -// path calls after the spec row commits. It records the job -// row alongside (or just after) the spec write and lets the -// worker / reconciler / reaper drive the actual embedding pass -// off the request path. Failures are logged but do not block -// the spec write: the reconciler will pick up any spec whose -// embedding-row count is below operation_count on its next -// sweep, so a missed enqueue still converges. -func (h *Handler) enqueueEmbedJob(ctx context.Context, catalogID, specName string) { - if h.deps.EmbedJobs == nil { - // No queue (file mode / no DB). The data path falls - // back to lexical and the operator gets no embeddings; - // this is the documented degraded mode. - return - } - if _, err := h.deps.EmbedJobs.Enqueue(ctx, catalogindex.SpecKey{ - CatalogID: catalogID, SpecName: specName, - }, catalogindex.KindSpecWrite); err != nil { - slog.Warn("apigateway: enqueue embedding job failed", - logKeyCatalogID, logsan.SanitizeForLog(catalogID), logKeySpecName, logsan.SanitizeForLog(specName), logKeyError, err) - } -} - // logKeyCatalogID is the structured-log key for catalog ids in the // admin handler. Kept local to this file so other admin handlers // don't accidentally drift the spelling. diff --git a/pkg/admin/catalog_handler_embedjobs_test.go b/pkg/admin/catalog_handler_embedjobs_test.go index 3840b772..7c5f3ec1 100644 --- a/pkg/admin/catalog_handler_embedjobs_test.go +++ b/pkg/admin/catalog_handler_embedjobs_test.go @@ -13,7 +13,7 @@ import ( "github.com/txn2/mcp-data-platform/pkg/toolkits/apigateway/catalogindex" ) -// fakeEmbedJobs implements admin.EmbedJobsStore in-memory. +// fakeEmbedJobs implements catalogindex.Store in-memory. // Exists only for admin-handler tests; the production store is // Postgres-backed and tested separately. type fakeEmbedJobs struct { @@ -22,11 +22,18 @@ type fakeEmbedJobs struct { nextID int64 enqueueErr error + cancelErr error listErr error getErr error statusesErr error healthErr error + // canceled / canceledCatalogs record every Cancel / CancelCatalog + // call so delete-path tests can assert the handler unwound the + // queue residue. + canceled []catalogindex.SpecKey + canceledCatalogs []string + statuses []catalogindex.SpecStatusRow health *catalogindex.CatalogHealth } @@ -55,6 +62,46 @@ func (f *fakeEmbedJobs) Enqueue(_ context.Context, key catalogindex.SpecKey, kin return true, nil } +// Cancel mirrors the AdminStore semantics: pending jobs for the key +// are dropped (failed rows stay; the fake's Job has no resolved +// marker, so resolution is asserted via the canceled record). +func (f *fakeEmbedJobs) Cancel(_ context.Context, key catalogindex.SpecKey) error { + if f.cancelErr != nil { + return f.cancelErr + } + f.mu.Lock() + defer f.mu.Unlock() + f.canceled = append(f.canceled, key) + kept := f.jobs[:0] + for _, j := range f.jobs { + if j.CatalogID == key.CatalogID && j.SpecName == key.SpecName && j.Status == catalogindex.StatusPending { + continue + } + kept = append(kept, j) + } + f.jobs = kept + return nil +} + +// CancelCatalog drops every pending job under the catalog. +func (f *fakeEmbedJobs) CancelCatalog(_ context.Context, catalogID string) error { + if f.cancelErr != nil { + return f.cancelErr + } + f.mu.Lock() + defer f.mu.Unlock() + f.canceledCatalogs = append(f.canceledCatalogs, catalogID) + kept := f.jobs[:0] + for _, j := range f.jobs { + if j.CatalogID == catalogID && j.Status == catalogindex.StatusPending { + continue + } + kept = append(kept, j) + } + f.jobs = kept + return nil +} + func (f *fakeEmbedJobs) List(_ context.Context, filter catalogindex.ListFilter) ([]catalogindex.Job, error) { if f.listErr != nil { return nil, f.listErr @@ -181,8 +228,8 @@ func TestSpecUpsert_EnqueuesJob(t *testing.T) { } // TestSpecUpsert_EnqueueErrorIsBestEffort drives the producer hook's -// best-effort warn branch (catalog_handler.go:1078): the spec write -// commits but the queue Enqueue fails. The HTTP write must still +// best-effort warn branch (catalogindex.EnqueueBestEffort): the spec +// write commits but the queue Enqueue fails. The HTTP write must still // return 200 because a missed enqueue is non-fatal (the reconciler // re-detects the gap on its next sweep), and no job row is recorded // because Enqueue errored before appending. @@ -475,3 +522,76 @@ func TestSpecToResponseWithEmbedding_PopulatesJobFields(t *testing.T) { t.Errorf("missing job state on spec response: %+v", body) } } + +// TestSpecDelete_CancelsJobs covers the delete-side counterpart to +// the producer hook (#998): removing a spec must unwind its queued +// index work, or the orphaned job fails terminally and pins the +// api_catalog index kind to Degraded until an operator dismisses it. +func TestSpecDelete_CancelsJobs(t *testing.T) { + t.Parallel() + h, _, jobs := newCatalogTestHandlerWithJobs(t) + res := doJSON(t, h, http.MethodPut, "/api/v1/admin/api-catalogs/petstore/specs/default", map[string]any{ + "source_kind": "inline", + "content": minimalSpec, + }) + if res.Code != http.StatusOK { + t.Fatalf("upsert: %d %s", res.Code, res.Body.String()) + } + if len(jobs.jobs) != 1 { + t.Fatalf("expected 1 pending job before delete, got %d", len(jobs.jobs)) + } + + res = doJSON(t, h, http.MethodDelete, "/api/v1/admin/api-catalogs/petstore/specs/default", nil) + if res.Code != http.StatusOK { + t.Fatalf("delete: %d %s", res.Code, res.Body.String()) + } + want := catalogindex.SpecKey{CatalogID: "petstore", SpecName: "default"} + if len(jobs.canceled) != 1 || jobs.canceled[0] != want { + t.Errorf("Cancel calls = %+v; want exactly %+v", jobs.canceled, want) + } + if len(jobs.jobs) != 0 { + t.Errorf("pending jobs after delete = %d; want 0", len(jobs.jobs)) + } +} + +// TestSpecDelete_CancelErrorIsBestEffort: the spec row is already +// gone, so a queue hiccup must not fail the delete. The worker's +// source-gone path settles anything the missed cancel left behind. +func TestSpecDelete_CancelErrorIsBestEffort(t *testing.T) { + t.Parallel() + h, store, jobs := newCatalogTestHandlerWithJobs(t) + _ = store.UpsertSpec(context.Background(), "petstore", apicatalog.SpecEntry{ + SpecName: "default", Content: minimalSpec, SourceKind: apicatalog.SourceInline, + }) + jobs.cancelErr = errors.New("queue down") + res := doJSON(t, h, http.MethodDelete, "/api/v1/admin/api-catalogs/petstore/specs/default", nil) + if res.Code != http.StatusOK { + t.Errorf("delete must succeed despite cancel failure: %d %s", res.Code, res.Body.String()) + } +} + +// TestCatalogDelete_CancelsJobsForEverySpec covers the whole-catalog +// variant of the same hole: the cascade removes every spec row, so +// every spec's queue residue must be unwound. +func TestCatalogDelete_CancelsJobsForEverySpec(t *testing.T) { + t.Parallel() + h, store, jobs := newCatalogTestHandlerWithJobs(t) + for _, name := range []string{"alpha", "beta"} { + if err := store.UpsertSpec(context.Background(), "petstore", apicatalog.SpecEntry{ + SpecName: name, Content: minimalSpec, SourceKind: apicatalog.SourceInline, + }); err != nil { + t.Fatalf("UpsertSpec(%s): %v", name, err) + } + } + + res := doJSON(t, h, http.MethodDelete, "/api/v1/admin/api-catalogs/petstore", nil) + if res.Code != http.StatusOK { + t.Fatalf("delete catalog: %d %s", res.Code, res.Body.String()) + } + if len(jobs.canceledCatalogs) != 1 || jobs.canceledCatalogs[0] != "petstore" { + t.Errorf("CancelCatalog calls = %+v; want exactly [petstore]", jobs.canceledCatalogs) + } + if len(jobs.jobs) != 0 { + t.Errorf("pending jobs after catalog delete = %d; want 0", len(jobs.jobs)) + } +} diff --git a/pkg/admin/handler.go b/pkg/admin/handler.go index 70f7f403..3a92ef90 100644 --- a/pkg/admin/handler.go +++ b/pkg/admin/handler.go @@ -194,11 +194,12 @@ type Deps struct { // EmbedJobs is the Postgres-backed job queue for api-catalog // embedding work. The admin handler enqueues jobs on every - // spec write and lets the worker / reconciler / reaper run - // the actual embedding pass off the request path. nil when - // the platform was built without a database; spec writes - // still succeed in that mode but no embeddings are persisted. - EmbedJobs EmbedJobsStore + // spec write, cancels them on spec/catalog delete (#998), and + // lets the worker / reconciler / reaper run the actual + // embedding pass off the request path. nil when the platform + // was built without a database; spec writes still succeed in + // that mode but no embeddings are persisted. + EmbedJobs catalogindex.Store // IndexJobs is the cross-kind read + command surface for the admin // Indexing dashboard (per-kind job-state counts, coverage, the job @@ -250,19 +251,6 @@ type IndexJobsService interface { Resolve(ctx context.Context, kind, sourceID string) (int, error) } -// EmbedJobsStore is the subset of catalogindex.Store the admin -// handler uses (enqueue + read-side queries for the UI). Declared -// here so admin can mock the queue without depending on its -// concrete implementation. catalogindex backs it with the generic -// index_jobs queue joined to the api_catalog tables. -type EmbedJobsStore interface { - Enqueue(ctx context.Context, key catalogindex.SpecKey, kind catalogindex.Kind) (bool, error) - List(ctx context.Context, filter catalogindex.ListFilter) ([]catalogindex.Job, error) - Get(ctx context.Context, id int64) (*catalogindex.Job, error) - SpecStatuses(ctx context.Context, catalogID string) ([]catalogindex.SpecStatusRow, error) - Health(ctx context.Context, catalogID string) (*catalogindex.CatalogHealth, error) -} - // APICatalogStore is the subset of apigateway/catalog.Store that the // admin handler needs. Declared here to avoid pulling the // apigateway package into pkg/admin's import surface (the apigateway diff --git a/pkg/indexjobs/registry_test.go b/pkg/indexjobs/registry_test.go index 8ea687a3..33e52b2c 100644 --- a/pkg/indexjobs/registry_test.go +++ b/pkg/indexjobs/registry_test.go @@ -26,13 +26,14 @@ func (s *stubSource) OnSucceeded(id string) { } type stubSink struct { - kind string - existing map[string]Vector - listErr error - upserted []Vector - upErr error - stamped int - gaps []string + kind string + existing map[string]Vector + listErr error + upserted []Vector + upsertCalls int + upErr error + stamped int + gaps []string } func (s *stubSink) Kind() string { return s.kind } @@ -42,6 +43,7 @@ func (s *stubSink) ListExisting(_ context.Context, _ Key) (map[string]Vector, er func (s *stubSink) Upsert(_ context.Context, _ Key, rows []Vector) error { s.upserted = rows + s.upsertCalls++ return s.upErr } func (*stubSink) UpsertBatch(_ context.Context, _ Key, _ []Vector) error { return nil } diff --git a/pkg/indexjobs/store_postgres.go b/pkg/indexjobs/store_postgres.go index 56f2a752..a6ab89a8 100644 --- a/pkg/indexjobs/store_postgres.go +++ b/pkg/indexjobs/store_postgres.go @@ -420,6 +420,27 @@ func (s *PostgresStore) PurgeTerminal(ctx context.Context, retentionDays int) (i } } +// CancelPending deletes the unit's pending job rows. Running rows are +// deliberately not touched (a worker holds their lease; the +// ErrSourceGone path resolves them when LoadItems notices the source +// is gone). +func (s *PostgresStore) CancelPending(ctx context.Context, key Key) (int, error) { + const q = ` + DELETE FROM index_jobs + WHERE source_kind = $1 AND source_id = $2 + AND status = 'pending' + ` + res, err := s.db.ExecContext(ctx, q, key.SourceKind, key.SourceID) + if err != nil { + return 0, fmt.Errorf("indexjobs: cancel pending: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return 0, fmt.Errorf("indexjobs: cancel pending rows-affected: %w", err) + } + return int(n), nil +} + // Get returns one job by id. func (s *PostgresStore) Get(ctx context.Context, id int64) (*Job, error) { q := `SELECT ` + jobColumns + ` FROM index_jobs WHERE id = $1` diff --git a/pkg/indexjobs/store_postgres_test.go b/pkg/indexjobs/store_postgres_test.go index 5fbee73f..079d409f 100644 --- a/pkg/indexjobs/store_postgres_test.go +++ b/pkg/indexjobs/store_postgres_test.go @@ -343,6 +343,32 @@ func TestStore_ResolveFailuresError(t *testing.T) { } } +func TestStore_CancelPending(t *testing.T) { + t.Parallel() + s, mock, done := newMockStore(t) + defer done() + mock.ExpectExec("DELETE FROM index_jobs"). + WithArgs("api_catalog", "cat1\x1fspec1"). + WillReturnResult(sqlmock.NewResult(0, 2)) + n, err := s.CancelPending(context.Background(), Key{SourceKind: "api_catalog", SourceID: "cat1\x1fspec1"}) + if err != nil { + t.Fatalf("CancelPending: %v", err) + } + if n != 2 { + t.Errorf("canceled = %d; want 2", n) + } +} + +func TestStore_CancelPendingError(t *testing.T) { + t.Parallel() + s, mock, done := newMockStore(t) + defer done() + mock.ExpectExec("DELETE FROM index_jobs").WillReturnError(errors.New("db down")) + if _, err := s.CancelPending(context.Background(), Key{SourceKind: "tools", SourceID: "p"}); err == nil { + t.Fatal("expected cancel-pending error") + } +} + func TestStore_ActiveFailures(t *testing.T) { t.Parallel() s, mock, done := newMockStore(t) diff --git a/pkg/indexjobs/stubs_test.go b/pkg/indexjobs/stubs_test.go index f812f22a..9fc9ee47 100644 --- a/pkg/indexjobs/stubs_test.go +++ b/pkg/indexjobs/stubs_test.go @@ -27,3 +27,4 @@ func (*noopStore) ActiveFailures(context.Context, string, int) ([]FailedUnit, er } func (*noopStore) ResolveFailures(context.Context, Key) (int, error) { return 0, nil } func (*noopStore) PurgeTerminal(context.Context, int) (int, error) { return 0, nil } +func (*noopStore) CancelPending(context.Context, Key) (int, error) { return 0, nil } diff --git a/pkg/indexjobs/types.go b/pkg/indexjobs/types.go index 8bd0dcf9..2610dd08 100644 --- a/pkg/indexjobs/types.go +++ b/pkg/indexjobs/types.go @@ -326,6 +326,17 @@ var ErrNoJob = errors.New("indexjobs: no pending job available") // rotated lease gracefully. var ErrNotFound = errors.New("indexjobs: job not found") +// ErrSourceGone is the Source's "this unit no longer exists" signal. +// A LoadItems error wrapping this sentinel tells the worker the +// source row was deleted on purpose (not that loading failed), so +// the worker clears the unit's vectors, completes the job, and +// resolves any open failures for the key instead of recording a +// terminal failure. Without this distinction a job orphaned by a +// source delete lands in an open failure that no later success can +// ever supersede — permanent Degraded residue only an operator +// dismiss could clear (#998). +var ErrSourceGone = errors.New("indexjobs: source gone") + // Store is the persistence interface for the job queue. The // concrete implementation is Postgres-backed (see // store_postgres.go); the interface is declared here so @@ -416,6 +427,15 @@ type Store interface { // the number of rows deleted. A non-positive retentionDays is a // no-op (retention disabled). Backs the retainer's periodic sweep. PurgeTerminal(ctx context.Context, retentionDays int) (deleted int, err error) + + // CancelPending deletes the unit's pending job rows, the producer's + // counterpart to Enqueue for a source delete: work queued for a unit + // that no longer exists is dropped before a worker wastes a claim on + // it. Running rows are left alone (a worker holds the lease); their + // LoadItems surfaces ErrSourceGone and the worker resolves them. + // Returns the number of rows deleted (zero when nothing was queued, + // which a delete path treats as success, not an error). + CancelPending(ctx context.Context, key Key) (canceled int, err error) } // Source is a consumer's "what to index" contract. One Source per @@ -430,12 +450,14 @@ type Source interface { Kind() string // LoadItems returns every embeddable item for the unit. An - // empty slice (with a nil error) means "nothing to index" - // (e.g. the source row was deleted between enqueue and claim); - // the worker treats that as a clean completion that writes zero - // vectors. An error is treated as a unit-resolve failure and - // terminates the job (the source is gone or unreadable; retry - // will not help). + // empty slice (with a nil error) means "the unit exists but has + // nothing to index"; the worker treats that as a clean completion + // that writes zero vectors. An error wrapping ErrSourceGone means + // the source row no longer exists (deleted between enqueue and + // claim); the worker clears the unit's vectors, completes the + // job, and resolves any open failures for the key. Any other + // error is a unit-resolve failure and terminates the job into an + // open failure (the source is unreadable; retry will not help). LoadItems(ctx context.Context, sourceID string) ([]Item, error) // OnSucceeded is an optional post-embed hook called after a diff --git a/pkg/indexjobs/worker.go b/pkg/indexjobs/worker.go index 963ccb09..83810ff1 100644 --- a/pkg/indexjobs/worker.go +++ b/pkg/indexjobs/worker.go @@ -221,9 +221,17 @@ func (w *Worker) process(ctx context.Context, job *Job) { } items, err := source.LoadItems(ctx, job.SourceID) + if errors.Is(err, ErrSourceGone) { + // The source row was deleted on purpose. Resolve the unit + // instead of recording an open failure no later success could + // ever supersede (#998). + w.resolveGone(ctx, job, sink, err) + return + } if err != nil { - // The source row is gone or unreadable. Not retryable: it - // may have been deleted on purpose. Move to terminal failed. + // The source is unreadable. Not retryable: LoadItems reads + // the consumer's own store, and the next reconciler sweep + // re-enqueues the unit if a gap remains. w.terminate(ctx, job, fmt.Sprintf("load items failed: %v", err)) return } @@ -239,7 +247,7 @@ func (w *Worker) process(ctx context.Context, job *Job) { existing, err = sink.ListExisting(ctx, key) if err != nil { // A read failure from our own DB is retryable. - w.retryOrFail(ctx, job, fmt.Sprintf("list existing failed: %v", err)) + w.retryOrFail(ctx, job, source, sink, fmt.Sprintf("list existing failed: %v", err)) return } } @@ -257,12 +265,12 @@ func (w *Worker) process(ctx context.Context, job *Job) { persistBatch: w.persistBatchFn(ctx, key, sink), }) if err != nil { - w.retryOrFail(ctx, job, fmt.Sprintf("embed failed: %v", err)) + w.retryOrFail(ctx, job, source, sink, fmt.Sprintf("embed failed: %v", err)) return } if err := sink.Upsert(ctx, key, rows); err != nil { - w.retryOrFail(ctx, job, fmt.Sprintf("persist failed: %v", err)) + w.retryOrFail(ctx, job, source, sink, fmt.Sprintf("persist failed: %v", err)) return } @@ -352,12 +360,23 @@ func (w *Worker) heartbeat(ctx context.Context, job *Job) { // retryOrFail routes a job to Retry (with backoff) or Fail // (terminal) based on the attempts counter, which Claim already -// incremented. -func (w *Worker) retryOrFail(ctx context.Context, job *Job, errMsg string) { +// incremented. Before pinning a terminal failure it re-probes the +// source: if the unit was deleted mid-attempt, the failure being +// recorded now could never be superseded (the delete-side cancel ran +// while this job was running, so it saw no pending row to drop and no +// failed row to resolve), which is the #998 residue in a race shape. +// A gone probe routes to resolveGone instead; earlier attempts need +// no probe because their retry re-enters LoadItems, which surfaces +// ErrSourceGone itself. +func (w *Worker) retryOrFail(ctx context.Context, job *Job, source Source, sink Sink, errMsg string) { slog.Warn("indexjobs: job error", logKeyJobID, job.ID, logKeySourceKind, job.SourceKind, logKeySourceID, job.SourceID, "attempts", job.Attempts, logKeyError, errMsg) if job.Attempts >= MaxAttempts { + if _, probeErr := source.LoadItems(ctx, job.SourceID); errors.Is(probeErr, ErrSourceGone) { + w.resolveGone(ctx, job, sink, probeErr) + return + } w.terminate(ctx, job, errMsg) return } @@ -366,6 +385,29 @@ func (w *Worker) retryOrFail(ctx context.Context, job *Job, errMsg string) { } } +// resolveGone settles a job whose source unit no longer exists: the +// unit's vectors are cleared (best-effort; kinds with FK cascade have +// already dropped them) and the job is completed, which also resolves +// any open failures for the key — including a failure recorded by an +// earlier attempt that ran before the source was deleted. No +// StampExpected (the expected-count row belongs to the deleted +// source) and no OnSucceeded (there is nothing left to reload). +func (w *Worker) resolveGone(ctx context.Context, job *Job, sink Sink, cause error) { + slog.Info("indexjobs: source gone; resolving unit", + logKeyJobID, job.ID, logKeySourceKind, job.SourceKind, + logKeySourceID, job.SourceID, logKeyError, cause) + key := Key{SourceKind: job.SourceKind, SourceID: job.SourceID} + if err := sink.Upsert(ctx, key, nil); err != nil { + slog.Warn("indexjobs: clear vectors for gone source failed", + logKeyJobID, job.ID, logKeySourceKind, job.SourceKind, + logKeySourceID, job.SourceID, logKeyError, err) + } + if err := w.cfg.Store.Complete(ctx, job.ID, w.cfg.WorkerID); err != nil && !errors.Is(err, ErrNotFound) { + slog.Error("indexjobs: complete for gone source failed", + logKeyJobID, job.ID, logKeyError, err) + } +} + // terminate marks a job permanently failed. func (w *Worker) terminate(ctx context.Context, job *Job, errMsg string) { slog.Warn("indexjobs: job failed terminally", diff --git a/pkg/indexjobs/worker_test.go b/pkg/indexjobs/worker_test.go index 9509ae46..b635b6d3 100644 --- a/pkg/indexjobs/worker_test.go +++ b/pkg/indexjobs/worker_test.go @@ -3,6 +3,7 @@ package indexjobs import ( "context" "errors" + "fmt" "sync/atomic" "testing" "time" @@ -114,6 +115,57 @@ func TestWorkerProcess_LoadItemsErrorTerminates(t *testing.T) { } } +func TestWorkerProcess_SourceGoneResolvesInsteadOfFailing(t *testing.T) { + t.Parallel() + var reloaded bool + src := &stubSource{ + kind: "k", + err: fmt.Errorf("spec %q deleted: %w", "u1", ErrSourceGone), + onOK: func(string) { reloaded = true }, + } + snk := &stubSink{kind: "k"} + store := &recordingStore{} + w := newTestWorker(store, registryWith(src, snk)) + + w.process(context.Background(), writeJob("k")) + + if !store.completed { + t.Error("source-gone job should Complete (resolving open failures), not fail") + } + if store.failed || store.retried { + t.Error("source-gone must not record a failure or retry: nothing can ever supersede it") + } + if snk.upsertCalls != 1 || len(snk.upserted) != 0 { + t.Errorf("gone unit's vectors should be cleared with one empty Upsert; calls=%d rows=%d", + snk.upsertCalls, len(snk.upserted)) + } + if snk.stamped != 0 { + t.Errorf("StampExpected should not run for a gone unit; stamped=%d", snk.stamped) + } + if reloaded { + t.Error("OnSucceeded should not fire for a gone unit") + } +} + +// TestWorkerProcess_SourceGoneCompletesDespiteUpsertError: clearing a +// gone unit's vectors is best-effort (kinds with FK cascade have +// already dropped them); a sink error must not turn the resolution +// back into residue. +func TestWorkerProcess_SourceGoneCompletesDespiteUpsertError(t *testing.T) { + t.Parallel() + src := &stubSource{kind: "k", err: fmt.Errorf("gone: %w", ErrSourceGone)} + snk := &stubSink{kind: "k", upErr: errors.New("vector table down")} + store := &recordingStore{} + w := newTestWorker(store, registryWith(src, snk)) + + w.process(context.Background(), writeJob("k")) + + if !store.completed || store.failed || store.retried { + t.Errorf("source-gone must Complete despite Upsert error; completed=%v failed=%v retried=%v", + store.completed, store.failed, store.retried) + } +} + func TestWorkerProcess_ListExistingErrorRetries(t *testing.T) { t.Parallel() src := &stubSource{kind: "k", items: twoItems()} @@ -161,6 +213,49 @@ func TestWorkerProcess_ExhaustedAttemptsFails(t *testing.T) { } } +// vanishingSource returns items on the first LoadItems call and +// ErrSourceGone afterwards, modeling a source deleted mid-attempt. +type vanishingSource struct { + stubSource + calls int +} + +func (s *vanishingSource) LoadItems(_ context.Context, _ string) ([]Item, error) { + s.calls++ + if s.calls == 1 { + return twoItems(), nil + } + return nil, fmt.Errorf("deleted mid-attempt: %w", ErrSourceGone) +} + +// TestWorkerProcess_ExhaustedAttemptsProbesForGoneSource pins the +// race half of #998: the source is deleted while the final attempt is +// mid-flight (LoadItems already succeeded), so the delete-side cancel +// saw a running job and nothing to resolve. A terminal Fail recorded +// now could never be superseded; the exhausted-attempts path must +// re-probe the source and resolve instead. +func TestWorkerProcess_ExhaustedAttemptsProbesForGoneSource(t *testing.T) { + t.Parallel() + src := &vanishingSource{stubSource: stubSource{kind: "k"}} + snk := &stubSink{kind: "k", upErr: errors.New("fk violation: spec row cascaded away")} + store := &recordingStore{} + w := newTestWorker(store, registryWith(src, snk)) + + job := writeJob("k") + job.Attempts = MaxAttempts + w.process(context.Background(), job) + + if !store.completed { + t.Error("final-attempt failure with a gone source should resolve (Complete), not Fail") + } + if store.failed || store.retried { + t.Errorf("no terminal failure or retry expected; failed=%v retried=%v", store.failed, store.retried) + } + if src.calls != 2 { + t.Errorf("LoadItems calls = %d; want 2 (initial load + terminal probe)", src.calls) + } +} + func TestWorkerProcess_ManualRetrySkipsDedup(t *testing.T) { t.Parallel() // ListExisting would error, but manual_retry must skip it, so the diff --git a/pkg/platform/integration_indexjobs_sourcegone_test.go b/pkg/platform/integration_indexjobs_sourcegone_test.go new file mode 100644 index 00000000..21eba172 --- /dev/null +++ b/pkg/platform/integration_indexjobs_sourcegone_test.go @@ -0,0 +1,125 @@ +//go:build integration + +package platform_test + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/txn2/mcp-data-platform/internal/platform/indexqueue" + "github.com/txn2/mcp-data-platform/internal/testdb" + "github.com/txn2/mcp-data-platform/pkg/indexjobs" + apicatalog "github.com/txn2/mcp-data-platform/pkg/toolkits/apigateway/catalog" + "github.com/txn2/mcp-data-platform/pkg/toolkits/apigateway/catalogindex" +) + +// zeroEmbedder satisfies embedding.Provider with fixed zero vectors. +// The source-gone path resolves before any embed call, so it is never +// actually invoked; it exists to satisfy the worker's config. +type zeroEmbedder struct{ dim int } + +func (e *zeroEmbedder) Embed(_ context.Context, _ string) ([]float32, error) { + return make([]float32, e.dim), nil +} + +func (e *zeroEmbedder) EmbedBatch(_ context.Context, texts []string) ([][]float32, error) { + out := make([][]float32, len(texts)) + for i := range out { + out[i] = make([]float32, e.dim) + } + return out, nil +} + +func (e *zeroEmbedder) Dimension() int { return e.dim } +func (*zeroEmbedder) Kind() string { return "zero" } + +// sourceGoneSpec is a one-operation OpenAPI document so the seeded +// spec is valid until the test deletes it out from under the queue. +const sourceGoneSpec = `openapi: 3.0.0 +info: {title: t, version: "1"} +paths: + /a: + get: + operationId: a + responses: + "200": + description: ok` + +// TestIndexJobs_SourceGoneSelfHeals_RealDB proves the self-healing +// half of #998 through the real assembled queue (indexqueue.New with +// the real catalogSource, sink, worker, and Postgres store): a job +// orphaned by a spec delete that the handler-side cancel did not +// cover — the racing-write case — completes as source-gone instead of +// failing terminally, and its completion resolves the unit's earlier +// open failure. No operator dismiss, no permanent Degraded. +func TestIndexJobs_SourceGoneSelfHeals_RealDB(t *testing.T) { + ctx := context.Background() + db := testdb.New(t) + + catalogStore := apicatalog.NewPostgresStore(db) + jobs := indexjobs.NewPostgresStore(db) + key := indexjobs.Key{ + SourceKind: catalogindex.SourceKind, + SourceID: catalogindex.EncodeSourceID("cat1", "spec1"), + } + + require.NoError(t, catalogStore.CreateCatalog(ctx, apicatalog.Catalog{ID: "cat1", Name: "cat1"})) + require.NoError(t, catalogStore.UpsertSpec(ctx, "cat1", apicatalog.SpecEntry{ + SpecName: "spec1", Content: sourceGoneSpec, SourceKind: apicatalog.SourceInline, OperationCount: 1, + })) + + // An earlier attempt failed while the spec still existed, leaving + // an open failure for the unit. + _, err := jobs.Enqueue(ctx, key, indexjobs.TriggerWrite) + require.NoError(t, err) + staleJob, err := jobs.Claim(ctx, "stale-worker") + require.NoError(t, err) + require.NoError(t, jobs.Fail(ctx, staleJob.ID, "stale-worker", "embed failed: provider down")) + failures, err := jobs.ActiveFailures(ctx, catalogindex.SourceKind, 10) + require.NoError(t, err) + require.Len(t, failures, 1, "seeded open failure must be visible before the run") + + // A retry job is queued, and then the spec is deleted without the + // handler-side cancel (the racing case the cancel cannot cover). + _, err = jobs.Enqueue(ctx, key, indexjobs.TriggerWrite) + require.NoError(t, err) + require.NoError(t, catalogStore.DeleteSpec(ctx, "cat1", "spec1")) + + h := indexqueue.New(indexqueue.Config{ + DB: db, + Embedder: &zeroEmbedder{dim: 4}, + ModelName: "zero", + LeaseDuration: time.Minute, + CatalogStore: catalogStore, + }) + require.NotNil(t, h) + require.NoError(t, h.Start(ctx)) + defer func() { _ = h.Stop(ctx) }() + + deadline := time.Now().Add(10 * time.Second) + for { + rows, listErr := jobs.List(ctx, indexjobs.ListFilter{ + SourceKind: catalogindex.SourceKind, + SourceID: key.SourceID, + Status: indexjobs.StatusSucceeded, + }) + require.NoError(t, listErr) + if len(rows) == 1 { + break + } + require.True(t, time.Now().Before(deadline), + "orphaned job did not resolve as succeeded within the deadline") + time.Sleep(50 * time.Millisecond) + } + + failures, err = jobs.ActiveFailures(ctx, catalogindex.SourceKind, 10) + require.NoError(t, err) + require.Empty(t, failures, "the source-gone completion must resolve the unit's open failure") + + counts, err := jobs.Counts(ctx, catalogindex.SourceKind) + require.NoError(t, err) + require.Zero(t, counts.UnresolvedFailures, "api_catalog kind must report healthy") +} diff --git a/pkg/toolkits/apigateway/catalog/memory.go b/pkg/toolkits/apigateway/catalog/memory.go index cce5f75b..2f7ea80f 100644 --- a/pkg/toolkits/apigateway/catalog/memory.go +++ b/pkg/toolkits/apigateway/catalog/memory.go @@ -2,6 +2,7 @@ package catalog import ( "context" + "fmt" "sort" "sync" "time" @@ -194,17 +195,15 @@ func (s *MemoryStore) UpsertSpec(_ context.Context, catalogID string, spec SpecE return nil } -// GetSpec returns one spec from the catalog or ErrNotFound. +// GetSpec returns one spec from the catalog, or a spec-naming error +// wrapping ErrNotFound (matching the Postgres store, so the surfaced +// message identifies the missing spec rather than the catalog). func (s *MemoryStore) GetSpec(_ context.Context, catalogID, specName string) (*SpecEntry, error) { s.mu.Lock() defer s.mu.Unlock() - bucket, ok := s.specs[catalogID] - if !ok { - return nil, ErrNotFound - } - spec, ok := bucket[specName] + spec, ok := s.specs[catalogID][specName] if !ok { - return nil, ErrNotFound + return nil, fmt.Errorf("spec %q not found in catalog %q: %w", specName, catalogID, ErrNotFound) } return &spec, nil } diff --git a/pkg/toolkits/apigateway/catalog/store_postgres.go b/pkg/toolkits/apigateway/catalog/store_postgres.go index 1921b64f..05b045d9 100644 --- a/pkg/toolkits/apigateway/catalog/store_postgres.go +++ b/pkg/toolkits/apigateway/catalog/store_postgres.go @@ -275,7 +275,10 @@ func (s *PostgresStore) GetSpec(ctx context.Context, catalogID, specName string) &spec.ETag, &spec.BasePath, &spec.Title, &spec.Description, &fetchedAt, &spec.CreatedAt, &spec.UpdatedAt, &spec.OperationCount) if errors.Is(err, sql.ErrNoRows) { - return nil, ErrNotFound + // Name the spec: the bare sentinel reads "catalog: not found", + // which sends whoever sees the surfaced error to the wrong + // object when the catalog itself is present and healthy (#998). + return nil, fmt.Errorf("spec %q not found in catalog %q: %w", specName, catalogID, ErrNotFound) } if err != nil { return nil, fmt.Errorf("catalog: get spec: %w", err) diff --git a/pkg/toolkits/apigateway/catalogindex/adminstore.go b/pkg/toolkits/apigateway/catalogindex/adminstore.go index f037bfa4..e65e96ff 100644 --- a/pkg/toolkits/apigateway/catalogindex/adminstore.go +++ b/pkg/toolkits/apigateway/catalogindex/adminstore.go @@ -41,6 +41,50 @@ func (s *AdminStore) Enqueue(ctx context.Context, key SpecKey, kind Kind) (bool, return created, nil } +// Cancel drops the spec's pending jobs and resolves its open +// failures, the delete-side counterpart to Enqueue. Running jobs are +// left to the worker, whose ErrSourceGone path resolves them once +// LoadItems notices the spec row is gone. +func (s *AdminStore) Cancel(ctx context.Context, key SpecKey) error { + k := indexjobs.Key{SourceKind: SourceKind, SourceID: EncodeSourceID(key.CatalogID, key.SpecName)} + if _, err := s.jobs.CancelPending(ctx, k); err != nil { + return fmt.Errorf("catalogindex: cancel pending: %w", err) + } + if _, err := s.jobs.ResolveFailures(ctx, k); err != nil { + return fmt.Errorf("catalogindex: resolve failures: %w", err) + } + return nil +} + +// CancelCatalog clears every spec's queue residue after a catalog +// delete, matching jobs by the encoded source_id prefix (starts_with +// rather than LIKE, so a catalog id containing a LIKE metacharacter +// cannot over-match). Runs against index_jobs directly, like the +// package's other cross-table queries, because the framework store's +// per-key methods would require the spec names the cascade already +// deleted. +func (s *AdminStore) CancelCatalog(ctx context.Context, catalogID string) error { + const cancelQ = ` + DELETE FROM index_jobs + WHERE source_kind = $1 AND starts_with(source_id, $2) + AND status = 'pending' + ` + const resolveQ = ` + UPDATE index_jobs + SET resolved_at = NOW() + WHERE source_kind = $1 AND starts_with(source_id, $2) + AND status = 'failed' AND resolved_at IS NULL + ` + prefix := sourceIDPrefix(catalogID) + if _, err := s.db.ExecContext(ctx, cancelQ, SourceKind, prefix); err != nil { + return fmt.Errorf("catalogindex: cancel catalog pending: %w", err) + } + if _, err := s.db.ExecContext(ctx, resolveQ, SourceKind, prefix); err != nil { + return fmt.Errorf("catalogindex: resolve catalog failures: %w", err) + } + return nil +} + // List returns the matching jobs in api-catalog terms. A filter with // a spec name targets that exact unit; a filter with only a catalog // id matches every unit under it via the source_id prefix. diff --git a/pkg/toolkits/apigateway/catalogindex/adminstore_test.go b/pkg/toolkits/apigateway/catalogindex/adminstore_test.go index 11e5310c..ed0deaab 100644 --- a/pkg/toolkits/apigateway/catalogindex/adminstore_test.go +++ b/pkg/toolkits/apigateway/catalogindex/adminstore_test.go @@ -2,6 +2,7 @@ package catalogindex import ( "context" + "errors" "testing" "time" @@ -10,14 +11,19 @@ import ( "github.com/txn2/mcp-data-platform/pkg/indexjobs" ) -// fakeJobs is an indexjobs.Store stub recording the last Enqueue/List -// inputs so the AdminStore translation can be asserted. +// fakeJobs is an indexjobs.Store stub recording the last +// Enqueue/List/Cancel inputs so the AdminStore translation can be +// asserted. type fakeJobs struct { lastEnqueueKey indexjobs.Key lastEnqueueTrigger indexjobs.Trigger lastFilter indexjobs.ListFilter listResult []indexjobs.Job getResult *indexjobs.Job + lastCancelKey indexjobs.Key + lastResolveKey indexjobs.Key + cancelErr error + resolveErr error } func (f *fakeJobs) Enqueue(_ context.Context, k indexjobs.Key, tr indexjobs.Trigger) (bool, error) { @@ -51,10 +57,18 @@ func (*fakeJobs) ActiveFailures(context.Context, string, int) ([]indexjobs.Faile return nil, nil } -func (*fakeJobs) ResolveFailures(context.Context, indexjobs.Key) (int, error) { return 0, nil } +func (f *fakeJobs) ResolveFailures(_ context.Context, k indexjobs.Key) (int, error) { + f.lastResolveKey = k + return 0, f.resolveErr +} func (*fakeJobs) PurgeTerminal(context.Context, int) (int, error) { return 0, nil } +func (f *fakeJobs) CancelPending(_ context.Context, k indexjobs.Key) (int, error) { + f.lastCancelKey = k + return 0, f.cancelErr +} + func TestAdminStore_EnqueueEncodesAndMapsTrigger(t *testing.T) { t.Parallel() jobs := &fakeJobs{} @@ -73,6 +87,84 @@ func TestAdminStore_EnqueueEncodesAndMapsTrigger(t *testing.T) { } } +func TestAdminStore_CancelDropsPendingAndResolvesFailures(t *testing.T) { + t.Parallel() + jobs := &fakeJobs{} + s := NewAdminStore(jobs, nil) + if err := s.Cancel(context.Background(), SpecKey{CatalogID: "c", SpecName: "s"}); err != nil { + t.Fatalf("Cancel: %v", err) + } + want := indexjobs.Key{SourceKind: SourceKind, SourceID: EncodeSourceID("c", "s")} + if jobs.lastCancelKey != want { + t.Errorf("CancelPending key = %+v; want %+v", jobs.lastCancelKey, want) + } + if jobs.lastResolveKey != want { + t.Errorf("ResolveFailures key = %+v; want %+v", jobs.lastResolveKey, want) + } +} + +func TestAdminStore_CancelPropagatesErrors(t *testing.T) { + t.Parallel() + cancelErr := errors.New("cancel down") + resolveErr := errors.New("resolve down") + + s := NewAdminStore(&fakeJobs{cancelErr: cancelErr}, nil) + if err := s.Cancel(context.Background(), SpecKey{CatalogID: "c", SpecName: "s"}); !errors.Is(err, cancelErr) { + t.Errorf("Cancel with pending-delete error = %v; want wrap of %v", err, cancelErr) + } + + s = NewAdminStore(&fakeJobs{resolveErr: resolveErr}, nil) + if err := s.Cancel(context.Background(), SpecKey{CatalogID: "c", SpecName: "s"}); !errors.Is(err, resolveErr) { + t.Errorf("Cancel with resolve error = %v; want wrap of %v", err, resolveErr) + } +} + +func TestAdminStore_CancelCatalogDropsPendingAndResolvesByPrefix(t *testing.T) { + t.Parallel() + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() //nolint:errcheck // test cleanup + s := NewAdminStore(&fakeJobs{}, db) + + prefix := sourceIDPrefix("c") + mock.ExpectExec("DELETE FROM index_jobs"). + WithArgs(SourceKind, prefix). + WillReturnResult(sqlmock.NewResult(0, 2)) + mock.ExpectExec("UPDATE index_jobs"). + WithArgs(SourceKind, prefix). + WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := s.CancelCatalog(context.Background(), "c"); err != nil { + t.Fatalf("CancelCatalog: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("expectations: %v", err) + } +} + +func TestAdminStore_CancelCatalogPropagatesErrors(t *testing.T) { + t.Parallel() + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() //nolint:errcheck // test cleanup + s := NewAdminStore(&fakeJobs{}, db) + + mock.ExpectExec("DELETE FROM index_jobs").WillReturnError(errors.New("db down")) + if err := s.CancelCatalog(context.Background(), "c"); err == nil { + t.Error("expected pending-delete error to propagate") + } + + mock.ExpectExec("DELETE FROM index_jobs").WillReturnResult(sqlmock.NewResult(0, 0)) + mock.ExpectExec("UPDATE index_jobs").WillReturnError(errors.New("db down")) + if err := s.CancelCatalog(context.Background(), "c"); err == nil { + t.Error("expected resolve error to propagate") + } +} + func TestAdminStore_ListByCatalogUsesPrefix(t *testing.T) { t.Parallel() jobs := &fakeJobs{listResult: []indexjobs.Job{ diff --git a/pkg/toolkits/apigateway/catalogindex/besteffort.go b/pkg/toolkits/apigateway/catalogindex/besteffort.go new file mode 100644 index 00000000..02c86b02 --- /dev/null +++ b/pkg/toolkits/apigateway/catalogindex/besteffort.go @@ -0,0 +1,68 @@ +package catalogindex + +import ( + "context" + "log/slog" + + "github.com/txn2/mcp-data-platform/internal/logsan" +) + +// Structured-log keys for the best-effort hooks, matching the admin +// handler's spelling so catalog log lines stay greppable across +// packages. +const ( + logKeyCatalogID = "catalog_id" + logKeySpecName = "spec_name" + logKeyError = "error" +) + +// EnqueueBestEffort is the producer-side hook every admin spec write +// path calls after the spec row commits. It records the job row and +// lets the worker / reconciler / reaper drive the actual embedding +// pass off the request path. Failures are logged but do not block the +// spec write: the reconciler picks up any spec whose embedding-row +// count is below operation_count on its next sweep, so a missed +// enqueue still converges. A nil store (file mode / no DB) is the +// documented degraded mode: the data path falls back to lexical. +func EnqueueBestEffort(ctx context.Context, s Store, catalogID, specName string) { + if s == nil { + return + } + if _, err := s.Enqueue(ctx, SpecKey{CatalogID: catalogID, SpecName: specName}, KindSpecWrite); err != nil { + slog.Warn("apigateway: enqueue embedding job failed", + logKeyCatalogID, logsan.SanitizeForLog(catalogID), + logKeySpecName, logsan.SanitizeForLog(specName), logKeyError, err) + } +} + +// CancelBestEffort is the delete-side counterpart to +// EnqueueBestEffort: after a spec row is removed, its queued index +// jobs are dropped and its open failures resolved so the delete +// leaves no residue pinning the api_catalog index kind to Degraded +// (#998). Best-effort like the enqueue side: a missed cancel +// self-heals through the worker's source-gone path, and a lingering +// failure remains operator-dismissable, so the delete response does +// not depend on it. +func CancelBestEffort(ctx context.Context, s Store, catalogID, specName string) { + if s == nil { + return + } + if err := s.Cancel(ctx, SpecKey{CatalogID: catalogID, SpecName: specName}); err != nil { + slog.Warn("apigateway: cancel embedding jobs failed", + logKeyCatalogID, logsan.SanitizeForLog(catalogID), + logKeySpecName, logsan.SanitizeForLog(specName), logKeyError, err) + } +} + +// CancelCatalogBestEffort is CancelBestEffort for a whole-catalog +// delete: every spec the cascade removed has its residue cleared by +// source_id prefix, with the same best-effort semantics. +func CancelCatalogBestEffort(ctx context.Context, s Store, catalogID string) { + if s == nil { + return + } + if err := s.CancelCatalog(ctx, catalogID); err != nil { + slog.Warn("apigateway: cancel catalog embedding jobs failed", + logKeyCatalogID, logsan.SanitizeForLog(catalogID), logKeyError, err) + } +} diff --git a/pkg/toolkits/apigateway/catalogindex/besteffort_test.go b/pkg/toolkits/apigateway/catalogindex/besteffort_test.go new file mode 100644 index 00000000..0b7bba29 --- /dev/null +++ b/pkg/toolkits/apigateway/catalogindex/besteffort_test.go @@ -0,0 +1,80 @@ +package catalogindex + +import ( + "context" + "errors" + "testing" +) + +// stubQueue implements Store for the best-effort hook tests, recording +// the calls and returning a configurable error. +type stubQueue struct { + err error + enqueued []SpecKey + canceled []SpecKey + canceledCatalogs []string +} + +func (s *stubQueue) Enqueue(_ context.Context, key SpecKey, _ Kind) (bool, error) { + s.enqueued = append(s.enqueued, key) + return s.err == nil, s.err +} + +func (s *stubQueue) Cancel(_ context.Context, key SpecKey) error { + s.canceled = append(s.canceled, key) + return s.err +} + +func (s *stubQueue) CancelCatalog(_ context.Context, catalogID string) error { + s.canceledCatalogs = append(s.canceledCatalogs, catalogID) + return s.err +} + +func (*stubQueue) List(context.Context, ListFilter) ([]Job, error) { return nil, nil } +func (*stubQueue) Get(context.Context, int64) (*Job, error) { return nil, ErrNotFound } +func (*stubQueue) SpecStatuses(context.Context, string) ([]SpecStatusRow, error) { return nil, nil } +func (*stubQueue) Health(context.Context, string) (*CatalogHealth, error) { + return &CatalogHealth{}, nil +} + +func TestEnqueueBestEffort(t *testing.T) { + t.Parallel() + // nil store (file mode / no DB) is a silent no-op. + EnqueueBestEffort(context.Background(), nil, "c", "s") + + q := &stubQueue{} + EnqueueBestEffort(context.Background(), q, "c", "s") + if len(q.enqueued) != 1 || q.enqueued[0] != (SpecKey{CatalogID: "c", SpecName: "s"}) { + t.Errorf("enqueued = %+v; want [{c s}]", q.enqueued) + } + + // An enqueue error is logged, not surfaced: the hook has no + // return value, so the write path proceeds regardless. + EnqueueBestEffort(context.Background(), &stubQueue{err: errors.New("queue down")}, "c", "s") +} + +func TestCancelBestEffort(t *testing.T) { + t.Parallel() + CancelBestEffort(context.Background(), nil, "c", "s") + + q := &stubQueue{} + CancelBestEffort(context.Background(), q, "c", "s") + if len(q.canceled) != 1 || q.canceled[0] != (SpecKey{CatalogID: "c", SpecName: "s"}) { + t.Errorf("canceled = %+v; want [{c s}]", q.canceled) + } + + CancelBestEffort(context.Background(), &stubQueue{err: errors.New("queue down")}, "c", "s") +} + +func TestCancelCatalogBestEffort(t *testing.T) { + t.Parallel() + CancelCatalogBestEffort(context.Background(), nil, "c") + + q := &stubQueue{} + CancelCatalogBestEffort(context.Background(), q, "c") + if len(q.canceledCatalogs) != 1 || q.canceledCatalogs[0] != "c" { + t.Errorf("canceledCatalogs = %+v; want [c]", q.canceledCatalogs) + } + + CancelCatalogBestEffort(context.Background(), &stubQueue{err: errors.New("queue down")}, "c") +} diff --git a/pkg/toolkits/apigateway/catalogindex/catalogindex.go b/pkg/toolkits/apigateway/catalogindex/catalogindex.go index bd1fbb6d..8722467b 100644 --- a/pkg/toolkits/apigateway/catalogindex/catalogindex.go +++ b/pkg/toolkits/apigateway/catalogindex/catalogindex.go @@ -189,6 +189,15 @@ type CatalogHealth struct { // the api_catalog tables. type Store interface { Enqueue(ctx context.Context, key SpecKey, kind Kind) (bool, error) + // Cancel clears the queue residue a deleted spec leaves behind: + // its pending jobs are dropped and its open failures resolved, so + // a spec delete never pins the api_catalog index kind to Degraded + // (#998). Zero residue is success, not an error. + Cancel(ctx context.Context, key SpecKey) error + // CancelCatalog is the whole-catalog Cancel: it clears the residue + // for every spec the catalog delete cascaded away, matched by the + // encoded source_id prefix so no pre-delete spec listing is needed. + CancelCatalog(ctx context.Context, catalogID string) error List(ctx context.Context, filter ListFilter) ([]Job, error) Get(ctx context.Context, id int64) (*Job, error) SpecStatuses(ctx context.Context, catalogID string) ([]SpecStatusRow, error) diff --git a/testdata/allowed_internal_imports.txt b/testdata/allowed_internal_imports.txt index acb61732..84cf5cb7 100644 --- a/testdata/allowed_internal_imports.txt +++ b/testdata/allowed_internal_imports.txt @@ -364,6 +364,7 @@ pkg/toolkits/apigateway -> pkg/query pkg/toolkits/apigateway -> pkg/semantic pkg/toolkits/apigateway -> pkg/toolkit pkg/toolkits/apigateway -> pkg/toolkits/apigateway/catalog +pkg/toolkits/apigateway/catalogindex -> internal/logsan pkg/toolkits/apigateway/catalogindex -> pkg/indexjobs pkg/toolkits/apigateway/catalogindex -> pkg/toolkits/apigateway/catalog pkg/toolkits/datahub -> pkg/query