From b33eb93aac8ab6d178dec5413b81dd03f80f8a37 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Fri, 24 Apr 2026 17:57:39 -0700 Subject: [PATCH] fix(cron): default missing sessionTarget on load and guard assertSupportedJobSpec (#70367) * fix(cron): default missing sessionTarget on load and guard assertSupportedJobSpec * fix(cron): use Object.hasOwn for payload.kind check and log the backfill Address review feedback on #70367: - Switch the new payload.kind lookup from `in` to `Object.hasOwn` so prototype pollution cannot drive the defaulter (Aisle Low finding). - Log a warning when a job is auto-defaulted at load time, matching the adjacent legacyJobIdIssue pattern so operators can run `openclaw doctor --fix` to persist the canonical shape (Greptile P2). * fix(cron): dedupe sessionTarget backfill warn per jobId and sharpen crash site reference Address deep-review feedback on #70367: - The code comment referenced assertSupportedJobSpec as the tick-time crash site, but that function is only called from create/patch (jobs.ts:607, 686) and manual-run preflight (ops.ts:516). The actual on-tick TypeError surfaces in runIsolatedAgentJob (server-cron.ts). Update the comment to say so. - ensureLoaded runs with forceReload:true on every onTimer tick (~60s). Before this change, a persistent legacy job missing sessionTarget produced one warn line per tick, forever. Add a per-jobId dedupe set on CronServiceState (mirroring the existing warnedDisabled flag) so the warn fires once per job per process. - Drop the 'run openclaw doctor --fix' remediation from the warn message. Doctor's cron-store migration has no trackIssue entry for missing sessionTarget (doctor-cron-store-migration.ts CronStoreIssueKey), so doctor --fix on a store whose only defect is missing sessionTarget silently returns without writing anything. Point operators at jobs.json directly until that gap is closed. * docs(changelog): note cron session target repair --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Vincent Koc --- CHANGELOG.md | 1 + src/cron/service.test-harness.ts | 1 + src/cron/service/jobs.ts | 5 + src/cron/service/state.ts | 7 ++ .../store.load-missing-session-target.test.ts | 115 ++++++++++++++++++ src/cron/service/store.ts | 37 ++++++ 6 files changed, 166 insertions(+) create mode 100644 src/cron/service/store.load-missing-session-target.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ec6ffc82d89..3f9cd5bd473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -333,6 +333,7 @@ Docs: https://docs.openclaw.ai - Pi embedded runs: pass real built-in tools into Pi session creation and then narrow active tool names after custom tool registration, so the runner and compaction paths compile cleanly and keep OpenClaw-managed custom tool allowlists without feeding string arrays into `createAgentSession`. Thanks @vincentkoc. - Agents/OpenAI websocket: route native OpenAI websocket metadata and session-header decisions through the shared endpoint classifier so local mocks and custom `models.providers.openai.baseUrl` endpoints stay out of the native OpenAI path consistently across embedded-runner and websocket transport code. Thanks @vincentkoc. - Cron/MCP: retire bundled MCP runtimes through one shared cleanup path for isolated cron run ends, persistent cron session rollover, and direct cron `deleteAfterRun` fallback cleanup. Fixes #69145, #68623, and #68827. Thanks @steipete. +- Cron: default legacy persisted jobs that are missing `sessionTarget` during store load and report a clear validation error if a malformed job still reaches execution, preventing undefined `startsWith` crashes on scheduled ticks. Fixes #70360 and #63383. (#70367) Thanks @mvanhorn. - MCP/gateway: tear down stdio MCP process trees on transport close and dispose bundled MCP runtimes during session delete/reset, preventing orphaned wrapper/server processes from accumulating. Fixes #68809 and #69465. Thanks @steipete. - Agents/MCP: retire bundled MCP runtimes after completed one-shot subagent cleanup and nested `sessions_send` steps, while keeping persistent subagent sessions warm. Thanks @steipete. - Config: render validation warnings with real line breaks instead of a literal `\n` sequence in CLI/audit output. Fixes #70140. Thanks @steipete. diff --git a/src/cron/service.test-harness.ts b/src/cron/service.test-harness.ts index 2c288592375..5a231930bcf 100644 --- a/src/cron/service.test-harness.ts +++ b/src/cron/service.test-harness.ts @@ -245,6 +245,7 @@ export function createMockCronStateForJobs(params: { storeFileMtimeMs: null, op: Promise.resolve(), warnedDisabled: false, + warnedMissingSessionTargetJobIds: new Set(), deps: { storePath: "/mock/path", cronEnabled: true, diff --git a/src/cron/service/jobs.ts b/src/cron/service/jobs.ts index f9905d92a95..2868ce34feb 100644 --- a/src/cron/service/jobs.ts +++ b/src/cron/service/jobs.ts @@ -154,6 +154,11 @@ function resolveEveryAnchorMs(params: { } export function assertSupportedJobSpec(job: Pick) { + if (typeof job.sessionTarget !== "string") { + throw new Error( + 'cron job is missing sessionTarget; expected "main", "isolated", "current", or "session:"', + ); + } const isIsolatedLike = job.sessionTarget === "isolated" || job.sessionTarget === "current" || diff --git a/src/cron/service/state.ts b/src/cron/service/state.ts index 6cd7f6e1612..80d277eecc9 100644 --- a/src/cron/service/state.ts +++ b/src/cron/service/state.ts @@ -128,6 +128,12 @@ export type CronServiceState = { running: boolean; op: Promise; warnedDisabled: boolean; + /** + * Job ids whose missing `sessionTarget` was defaulted at load and warned + * about. Used to suppress duplicate warns across forceReload ticks so a + * single broken job does not spam the log on every scheduler cycle. + */ + warnedMissingSessionTargetJobIds: Set; storeLoadedAtMs: number | null; storeFileMtimeMs: number | null; }; @@ -140,6 +146,7 @@ export function createCronServiceState(deps: CronServiceDeps): CronServiceState running: false, op: Promise.resolve(), warnedDisabled: false, + warnedMissingSessionTargetJobIds: new Set(), storeLoadedAtMs: null, storeFileMtimeMs: null, }; diff --git a/src/cron/service/store.load-missing-session-target.test.ts b/src/cron/service/store.load-missing-session-target.test.ts new file mode 100644 index 00000000000..0553114685c --- /dev/null +++ b/src/cron/service/store.load-missing-session-target.test.ts @@ -0,0 +1,115 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { describe, expect, it, vi } from "vitest"; +import { setupCronServiceSuite } from "../service.test-harness.js"; +import { assertSupportedJobSpec, findJobOrThrow } from "./jobs.js"; +import { createCronServiceState } from "./state.js"; +import { ensureLoaded } from "./store.js"; + +const { logger, makeStorePath } = setupCronServiceSuite({ + prefix: "cron-service-store-missing-session-target-", +}); + +const STORE_TEST_NOW = Date.parse("2026-03-23T12:00:00.000Z"); + +async function writeSingleJobStore(storePath: string, job: Record) { + await fs.mkdir(path.dirname(storePath), { recursive: true }); + await fs.writeFile(storePath, JSON.stringify({ version: 1, jobs: [job] }, null, 2), "utf8"); +} + +function createStoreTestState(storePath: string) { + return createCronServiceState({ + storePath, + cronEnabled: true, + log: logger, + nowMs: () => STORE_TEST_NOW, + enqueueSystemEvent: vi.fn(), + requestHeartbeatNow: vi.fn(), + runIsolatedAgentJob: vi.fn(async () => ({ status: "ok" as const })), + }); +} + +describe("cron service store load: missing sessionTarget", () => { + it('defaults missing sessionTarget to "main" for systemEvent payloads', async () => { + const { storePath } = await makeStorePath(); + + await writeSingleJobStore(storePath, { + id: "missing-session-target-system-event", + name: "missing session target system event", + enabled: true, + createdAtMs: STORE_TEST_NOW - 60_000, + updatedAtMs: STORE_TEST_NOW - 60_000, + schedule: { kind: "every", everyMs: 60_000 }, + wakeMode: "now", + payload: { kind: "systemEvent", text: "tick" }, + state: {}, + }); + + const state = createStoreTestState(storePath); + await ensureLoaded(state); + + const job = findJobOrThrow(state, "missing-session-target-system-event"); + expect(job.sessionTarget).toBe("main"); + expect(() => assertSupportedJobSpec(job)).not.toThrow(); + }); + + it('defaults missing sessionTarget to "isolated" for agentTurn payloads', async () => { + const { storePath } = await makeStorePath(); + + await writeSingleJobStore(storePath, { + id: "missing-session-target-agent-turn", + name: "missing session target agent turn", + enabled: true, + createdAtMs: STORE_TEST_NOW - 60_000, + updatedAtMs: STORE_TEST_NOW - 60_000, + schedule: { kind: "every", everyMs: 60_000 }, + wakeMode: "now", + payload: { kind: "agentTurn", message: "ping" }, + state: {}, + }); + + const state = createStoreTestState(storePath); + await ensureLoaded(state); + + const job = findJobOrThrow(state, "missing-session-target-agent-turn"); + expect(job.sessionTarget).toBe("isolated"); + expect(() => assertSupportedJobSpec(job)).not.toThrow(); + }); + + it("assertSupportedJobSpec throws a clear error when sessionTarget is missing", () => { + const bogus = { + payload: { kind: "agentTurn" as const, message: "ping" }, + } as unknown as Parameters[0]; + expect(() => assertSupportedJobSpec(bogus)).toThrow(/missing sessionTarget/); + }); + + it("warns once per jobId across repeated forceReload cycles", async () => { + const { storePath } = await makeStorePath(); + + await writeSingleJobStore(storePath, { + id: "log-dedupe-target", + name: "log dedupe target", + enabled: true, + createdAtMs: STORE_TEST_NOW - 60_000, + updatedAtMs: STORE_TEST_NOW - 60_000, + schedule: { kind: "every", everyMs: 60_000 }, + wakeMode: "now", + payload: { kind: "agentTurn", message: "ping" }, + state: {}, + }); + + const warnSpy = vi.spyOn(logger, "warn"); + const state = createStoreTestState(storePath); + + await ensureLoaded(state); + await ensureLoaded(state, { forceReload: true }); + await ensureLoaded(state, { forceReload: true }); + + const missingSessionTargetWarns = warnSpy.mock.calls.filter((call) => { + const msg = typeof call[1] === "string" ? call[1] : ""; + return msg.includes("missing sessionTarget"); + }); + expect(missingSessionTargetWarns).toHaveLength(1); + warnSpy.mockRestore(); + }); +}); diff --git a/src/cron/service/store.ts b/src/cron/service/store.ts index 2e8b19af454..8d2eb3eabad 100644 --- a/src/cron/service/store.ts +++ b/src/cron/service/store.ts @@ -67,6 +67,43 @@ export async function ensureLoaded( if (typeof hydrated.enabled !== "boolean") { hydrated.enabled = true; } + // Same shape: persisted jobs missing `sessionTarget` crash downstream + // on any code path that dereferences `.startsWith` (e.g. + // `runIsolatedAgentJob` in `src/gateway/server-cron.ts`). Mirror the + // defaulter applied at create time: systemEvent payloads -> "main", + // agentTurn -> "isolated". Use `Object.hasOwn` rather than `in` so a + // poisoned prototype cannot feed a crafted `kind` into the defaulter. + if (typeof hydrated.sessionTarget !== "string") { + const payload = hydrated.payload as unknown; + const payloadKind = + payload && + typeof payload === "object" && + !Array.isArray(payload) && + Object.hasOwn(payload, "kind") + ? (payload as { kind?: unknown }).kind + : undefined; + let defaulted: "main" | "isolated" | undefined; + if (payloadKind === "systemEvent") { + defaulted = "main"; + } else if (payloadKind === "agentTurn") { + defaulted = "isolated"; + } + if (defaulted) { + hydrated.sessionTarget = defaulted; + // `ensureLoaded` is called with `forceReload: true` on every tick; + // warn once per jobId per process to avoid log spam on repeated + // loads of the same still-broken store file. + const jobId = typeof hydrated.id === "string" ? hydrated.id : undefined; + const dedupeKey = jobId ?? ""; + if (!state.warnedMissingSessionTargetJobIds.has(dedupeKey)) { + state.warnedMissingSessionTargetJobIds.add(dedupeKey); + state.deps.log.warn( + { storePath: state.deps.storePath, jobId, defaulted }, + "cron: job missing sessionTarget; defaulted in memory (edit jobs.json to persist canonical shape)", + ); + } + } + } } state.store = { version: 1,