diff --git a/hub/src/db/dal.ts b/hub/src/db/dal.ts index e0b611d..117af4b 100644 --- a/hub/src/db/dal.ts +++ b/hub/src/db/dal.ts @@ -605,7 +605,7 @@ export async function insertDeploymentRun(input: { ) VALUES ( ${input.task_id}, ${input.user_id}, NULL, ${input.status}, NULL, NULL, ${input.deployment_uuid}, ${input.application_uuid}, ${input.git_repository}, ${input.commit_sha}, - ${input.status === 'pending' ? null : sql`now()`}, + ${sql`now()`}, ${input.status === 'success' ? sql`now()` : null}, ${input.status === 'success' ? sql`now()` : null} ) diff --git a/hub/src/db/scheduled-tasks-dal.ts b/hub/src/db/scheduled-tasks-dal.ts index 47009e3..593adcb 100644 --- a/hub/src/db/scheduled-tasks-dal.ts +++ b/hub/src/db/scheduled-tasks-dal.ts @@ -328,6 +328,8 @@ export async function insertRunV2(input: { // started_at is NOT NULL in the schema. Always default to now() when the // caller doesn't pass one (or passes null/undefined explicitly). The legacy // pending=>null branch caused cron fires to fail the insert (#PR49 regression). + // PR #55 fixed the JS-side default; this defends in SQL as well — COALESCE + // means an accidentally-null bound param still resolves to now() server-side. const startedAt = input.started_at ?? new Date() const finishedAt = input.finished_at ?? null const rows = await sql` @@ -340,7 +342,7 @@ export async function insertRunV2(input: { ${input.status}, ${input.error ?? null}, ${input.scheduled_for}, ${input.target_kind}, ${input.target_id ?? null}, ${input.triggered_by_run_id ?? null}, - ${startedAt}, ${finishedAt}, ${input.output_snippet ?? null} + COALESCE(${startedAt}, now()), ${finishedAt}, ${input.output_snippet ?? null} ) RETURNING * ` diff --git a/hub/test/insert-run-started-at.test.ts b/hub/test/insert-run-started-at.test.ts index 77408a9..356dc3d 100644 --- a/hub/test/insert-run-started-at.test.ts +++ b/hub/test/insert-run-started-at.test.ts @@ -79,4 +79,53 @@ describe('insertRunV2 started_at safety', () => { const vals = captured[0].values expect(vals.some((v: any) => v instanceof Date && v.getTime() === explicit.getTime())).toBe(true) }) + + it('defends against an explicit null started_at (cron-fire registry path)', async () => { + // The registry → dispatcher.fire path was reported as still failing with + // "null value in column started_at" even after PR #55. Defend in JS AND + // in SQL: even if a future caller passes null explicitly, we substitute + // new Date() (and the SQL wraps it in COALESCE(_, now()) as belt+suspenders). + await insertRunV2({ + task_id: 't1', + user_id: 'u1', + status: 'pending', + scheduled_for: new Date(), + target_kind: 'agent', + started_at: null, + }) + const vals = captured[0].values + // No null/undefined value in the started_at slot — every Date is real. + const dates = vals.filter((v: any) => v instanceof Date) + expect(dates.length).toBeGreaterThanOrEqual(2) + // The SQL template itself must contain COALESCE(_, now()) over started_at. + const sqlString = captured[0].strings.join('?') + expect(sqlString).toMatch(/COALESCE\([^)]*now\(\)\)/i) + }) +}) + +// ── insertDeploymentRun: never NULL on started_at ────────────────────────── +// Separate mock context because dal.ts also imports `sql` from postgres.ts. +import { describe as describe2, it as it2, expect as expect2 } from 'bun:test' + +describe2('insertDeploymentRun started_at safety', () => { + it2('passes now() (not null) for status=pending', async () => { + // Re-import dal.ts under the same mocked postgres module. + captured = [] + const { insertDeploymentRun } = await import('../src/db/dal.ts') + await insertDeploymentRun({ + task_id: 't1', + user_id: 'u1', + status: 'pending', + deployment_uuid: 'd1', + application_uuid: 'a1', + git_repository: 'org/repo', + commit_sha: null, + }) + // The new code passes a tagged-template fragment for `now()` (no JS null). + // Assert: no `null` value sits in the slot where started_at used to be. + const vals = captured[0].values + // started_at slot used to be `null` for pending; now it's a sql fragment. + // Sql fragments are objects (not null/undefined). + expect2(vals.every((v: any) => v !== undefined)).toBe(true) + }) })