From c203d4581163a2ef92c92cb83ec7c1c453876fe8 Mon Sep 17 00:00:00 2001 From: SYNITY Date: Wed, 15 Jul 2026 16:54:49 +0700 Subject: [PATCH] fix(cron): guard nil job and tenant-consistent readback in cron create [B24:2794] (#1438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a cron job over a connection with no tenant in context (gateway-token / CLI / master-scope UI session) panicked with a nil-pointer dereference. Root cause — tenant asymmetry between insert and readback: - AddJob INSERT uses tenantIDForInsert(ctx), which falls back to MasterTenantID when the context has no tenant (row inserted OK). - The readback (scanJob / GetJob) rejects a nil-tenant context with "tenant_id required", so GetJob returned (nil, false) and AddJob swallowed it as (nil, nil). - handleCreate then dereferenced job.ID (UpdateJob patch) → panic. Fix (defense-in-depth): - handleCreate (internal/gateway/methods/cron.go): add a nil-job guard before any job.ID use; return ErrInternal instead of panicking. Apply the post-create stateless/command patch under a tenant-consistent context (WithTenantID(ctx, job.TenantID)) so lockCronJobForMutation finds the row for nil-tenant callers. - AddJob (store/pg + store/sqlitestore cron_crud.go): read back under WithTenantID(ctx, tenantIDForInsert(ctx)) so insert and readback share the same tenant; return an explicit error if readback fails instead of swallowing it. Tests: - internal/store/sqlitestore/cron_crud_test.go: AddJob with a nil-tenant context returns a non-nil job + nil error (real SQLite DB). - internal/gateway/methods/cron_test.go: handleCreate does not panic and skips UpdateJob when the store returns (nil, nil). Co-authored-by: DangTinh311 --- internal/gateway/methods/cron.go | 15 ++++++++- internal/gateway/methods/cron_test.go | 25 +++++++++++++++ internal/store/pg/cron_crud.go | 12 ++++++- internal/store/sqlitestore/cron_crud.go | 12 ++++++- internal/store/sqlitestore/cron_crud_test.go | 33 ++++++++++++++++++++ 5 files changed, 94 insertions(+), 3 deletions(-) diff --git a/internal/gateway/methods/cron.go b/internal/gateway/methods/cron.go index 06c46407..2d4a928c 100644 --- a/internal/gateway/methods/cron.go +++ b/internal/gateway/methods/cron.go @@ -107,6 +107,13 @@ func (m *CronMethods) handleCreate(ctx context.Context, client *gateway.Client, client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, err.Error())) return } + // Anti-panic guard: a store may return (nil, nil) if the insert succeeded but + // the tenant-scoped readback failed (e.g. tenant asymmetry). Never dereference + // a nil job below. + if job == nil { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInternal, i18n.T(locale, i18n.MsgInternalError, "cron job created but could not be loaded"))) + return + } // Apply extra fields not in AddJob signature via an immediate patch. // Default stateless=true for new crons (saves tokens); override with explicit false. @@ -122,7 +129,13 @@ func (m *CronMethods) handleCreate(ctx context.Context, client *gateway.Client, if isCommand { patch.Command = params.Command } - if updated, pErr := m.service.UpdateJob(ctx, job.ID, patch); pErr == nil { + // Patch under a tenant-consistent context: the job was inserted under + // job.TenantID (which falls back to MasterTenantID for nil-tenant / + // master-scope connections). Using the raw ctx here would make + // lockCronJobForMutation miss the row for nil-tenant callers, silently + // dropping the stateless/command patch. + updateCtx := store.WithTenantID(ctx, job.TenantID) + if updated, pErr := m.service.UpdateJob(updateCtx, job.ID, patch); pErr == nil { job = updated } } diff --git a/internal/gateway/methods/cron_test.go b/internal/gateway/methods/cron_test.go index a366212a..a35e32f9 100644 --- a/internal/gateway/methods/cron_test.go +++ b/internal/gateway/methods/cron_test.go @@ -16,6 +16,7 @@ import ( type stubCronStore struct { jobs map[string]*store.CronJob addErr error + addNilJob bool // simulate insert-ok-but-readback-failed: AddJob returns (nil, nil) removeErr error updateErr error enableErr error @@ -38,6 +39,9 @@ func (s *stubCronStore) AddJob(_ context.Context, name string, schedule store.Cr if s.addErr != nil { return nil, s.addErr } + if s.addNilJob { + return nil, nil + } job := &store.CronJob{ ID: "new-job-id", Name: name, @@ -219,6 +223,27 @@ func TestCronCreate_ValidParams_CreatesJob(t *testing.T) { } } +// Regression (B24:2794): when the store's AddJob returns (nil, nil) — the +// insert succeeded but the tenant-scoped readback failed — handleCreate must +// NOT dereference the nil job (previously panicked at job.ID). It must return +// an error response and never call UpdateJob with a nil job. +func TestCronCreate_NilJobFromStore_NoPanic_ReturnsError(t *testing.T) { + svc := newStubCronStore() + svc.addNilJob = true + m := buildCronMethods(t, svc) + client := nullClient() + req := cronReqFrame(t, protocol.MethodCronCreate, map[string]any{ + "name": "nil-job", + "message": "do the thing", + "schedule": map[string]any{"kind": "every", "everyMs": 60000}, + }) + m.handleCreate(context.Background(), client, req) + // No panic = nil-guard hit before job.ID dereference. + if svc.updateCnt != 0 { + t.Fatalf("UpdateJob called %d times after nil job, want 0", svc.updateCnt) + } +} + // ---- Tests: handleDelete ---- func TestCronDelete_MissingJobID_ReturnsInvalidRequest(t *testing.T) { diff --git a/internal/store/pg/cron_crud.go b/internal/store/pg/cron_crud.go index ec8f5d44..09bc7bb8 100644 --- a/internal/store/pg/cron_crud.go +++ b/internal/store/pg/cron_crud.go @@ -81,7 +81,17 @@ func (s *PGCronStore) AddJob(ctx context.Context, name string, schedule store.Cr s.cacheLoaded = false // invalidate cache - job, _ := s.GetJob(ctx, id.String()) + // Read back with a tenant-consistent context. The INSERT above uses + // tenantIDForInsert(ctx), which falls back to MasterTenantID when the + // caller has no tenant in context (gateway-token / CLI / master-scope UI). + // scanJob, however, rejects a nil-tenant context with "tenant_id required". + // Reading back under the same tenant the row was inserted with keeps the + // insert and readback symmetric, so we never swallow the created job. + readCtx := store.WithTenantID(ctx, tenantIDForInsert(ctx)) + job, ok := s.GetJob(readCtx, id.String()) + if !ok || job == nil { + return nil, fmt.Errorf("cron job %s created but readback failed", id) + } return job, nil } diff --git a/internal/store/sqlitestore/cron_crud.go b/internal/store/sqlitestore/cron_crud.go index caf98c81..d613804d 100644 --- a/internal/store/sqlitestore/cron_crud.go +++ b/internal/store/sqlitestore/cron_crud.go @@ -83,7 +83,17 @@ func (s *SQLiteCronStore) AddJob(ctx context.Context, name string, schedule stor } s.InvalidateCache() - job, _ := s.GetJob(ctx, id.String()) + + // Read back with a tenant-consistent context. The INSERT above uses + // tenantIDForInsert(ctx), which falls back to MasterTenantID when the + // caller has no tenant in context. scanJob rejects a nil-tenant context + // with "tenant_id required", so reading back under the same tenant the row + // was inserted with keeps insert and readback symmetric (matches pg store). + readCtx := store.WithTenantID(ctx, tenantIDForInsert(ctx)) + job, ok := s.GetJob(readCtx, id.String()) + if !ok || job == nil { + return nil, fmt.Errorf("cron job %s created but readback failed", id) + } return job, nil } diff --git a/internal/store/sqlitestore/cron_crud_test.go b/internal/store/sqlitestore/cron_crud_test.go index f43489e1..a7f91126 100644 --- a/internal/store/sqlitestore/cron_crud_test.go +++ b/internal/store/sqlitestore/cron_crud_test.go @@ -300,6 +300,39 @@ func TestSQLiteCronStore_EnableJob_IgnoresMalformedPayload(t *testing.T) { } } +// TestSQLiteCronStore_AddJobNilTenantContext_ReturnsJob is a regression test for +// the cron-create nil-pointer bug (B24:2794). A connection without a tenant in +// context (gateway-token / CLI / master-scope UI session) inserts the row under +// MasterTenantID via the insert-tenant fallback, but the readback previously +// used the raw nil-tenant context and hit scanJob's "tenant_id required" guard. +// AddJob therefore returned (nil, nil), which the handler dereferenced → panic. +// The fix reads back under a tenant-consistent context, so AddJob must now +// return a non-nil job with nil error. +func TestSQLiteCronStore_AddJobNilTenantContext_ReturnsJob(t *testing.T) { + cronStore, _, _ := newTestSQLiteCronStore(t) + // Bare context: NO tenant, NO cross-tenant flag — exactly the shape a + // gateway-token / master-scope connection produces. + ctx := context.Background() + everyMS := int64(time.Minute / time.Millisecond) + + job, err := cronStore.AddJob(ctx, "job-nil-tenant", store.CronSchedule{ + Kind: "every", + EveryMS: &everyMS, + }, "hello", false, "", "", "", "user-1") + if err != nil { + t.Fatalf("AddJob error: %v", err) + } + if job == nil { + t.Fatal("AddJob returned (nil, nil) for nil-tenant context — readback swallowed the created job") + } + if job.TenantID != store.MasterTenantID { + t.Fatalf("job tenant = %v, want MasterTenantID %v", job.TenantID, store.MasterTenantID) + } + if job.Name != "job-nil-tenant" { + t.Fatalf("job name = %q, want %q", job.Name, "job-nil-tenant") + } +} + func newTestSQLiteCronStore(t *testing.T) (*SQLiteCronStore, context.Context, *sql.DB) { t.Helper()