mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 05:40:44 +00:00
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 <vincentkoc@ieee.org>
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -245,6 +245,7 @@ export function createMockCronStateForJobs(params: {
|
||||
storeFileMtimeMs: null,
|
||||
op: Promise.resolve(),
|
||||
warnedDisabled: false,
|
||||
warnedMissingSessionTargetJobIds: new Set<string>(),
|
||||
deps: {
|
||||
storePath: "/mock/path",
|
||||
cronEnabled: true,
|
||||
|
||||
@@ -154,6 +154,11 @@ function resolveEveryAnchorMs(params: {
|
||||
}
|
||||
|
||||
export function assertSupportedJobSpec(job: Pick<CronJob, "sessionTarget" | "payload">) {
|
||||
if (typeof job.sessionTarget !== "string") {
|
||||
throw new Error(
|
||||
'cron job is missing sessionTarget; expected "main", "isolated", "current", or "session:<id>"',
|
||||
);
|
||||
}
|
||||
const isIsolatedLike =
|
||||
job.sessionTarget === "isolated" ||
|
||||
job.sessionTarget === "current" ||
|
||||
|
||||
@@ -128,6 +128,12 @@ export type CronServiceState = {
|
||||
running: boolean;
|
||||
op: Promise<unknown>;
|
||||
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<string>;
|
||||
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<string>(),
|
||||
storeLoadedAtMs: null,
|
||||
storeFileMtimeMs: null,
|
||||
};
|
||||
|
||||
115
src/cron/service/store.load-missing-session-target.test.ts
Normal file
115
src/cron/service/store.load-missing-session-target.test.ts
Normal file
@@ -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<string, unknown>) {
|
||||
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<typeof assertSupportedJobSpec>[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();
|
||||
});
|
||||
});
|
||||
@@ -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 ?? "<unknown>";
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user