Preserve running marker for no-timeout jobs

The new zombie-marker logic can clear runningAtMs after only 2 minutes when resolveCronJobTimeoutMs(job) returns undefined (for supported timeoutSeconds <= 0 no-timeout agent turns). During a long manual cron run, state.running is false, so onTimer() still executes; once this marker is cleared, collectRunnableJobs() can pick the same past-due job and start a second concurrent execution, causing duplicate sends/agent turns and inconsistent job state. This is a regression from the prior 2-hour stuck-marker window used by maintenance normalization.

Useful? React with 👍 / 👎.

Fixed in 5eb7e17. Jobs with resolveCronJobTimeoutMs returning undefined (timeoutSeconds <= 0, explicitly unlimited) are now skipped entirely in the zombie detection loop. Only jobs with a concrete timeout value will ever have their markers cleared.

Test does not exercise the try/catch fix

The job uses sessionTarget: "main" with payload.kind: "systemEvent", which routes through executeMainSessionCronJob. That path calls enqueueSystemEvent/requestHeartbeatNow and returns { status: "ok" } — it never calls runIsolatedAgentJob. The throwing mock has no effect, runMissedJobs completes normally, and the try/catch guard added by this PR is never triggered. The test would pass identically with or without the fix.

To actually exercise the guard, the job would need to cause an infrastructure-level failure in runMissedJobs (e.g., mock persist or ensureLoaded to throw), since individual job-execution errors are already caught inside runStartupCatchupCandidate and returned as an error result rather than re-thrown.

This is a comment left during a code review.
Path: src/cron/service.start-error-resilience.test.ts
Line: 15-70

Comment:
**Test does not exercise the try/catch fix**

The job uses `sessionTarget: "main"` with `payload.kind: "systemEvent"`, which routes through `executeMainSessionCronJob`. That path calls `enqueueSystemEvent`/`requestHeartbeatNow` and returns `{ status: "ok" }` — it never calls `runIsolatedAgentJob`. The throwing mock has no effect, `runMissedJobs` completes normally, and the try/catch guard added by this PR is never triggered. The test would pass identically with or without the fix.

To actually exercise the guard, the job would need to cause an infrastructure-level failure in `runMissedJobs` (e.g., mock `persist` or `ensureLoaded` to throw), since individual job-execution errors are already caught inside `runStartupCatchupCandidate` and returned as an error result rather than re-thrown.

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 5eb7e17. The test now:

1. Uses an isolated agentTurn job (so the missed-job code path is exercised)
2. Mocks fs.writeFile to fail on the second write attempt — this causes persist() inside applyStartupCatchupOutcomes to throw, which propagates through runMissedJobs() and triggers the try/catch guard in start()
3. Verifies state.timer is not null (timer was armed despite the failure)

This provides real coverage of the fix.

2-minute hard cap fires for explicitly-disabled-timeout jobs

resolveCronJobTimeoutMs returns undefined when a job is configured with timeoutSeconds <= 0 (explicitly unlimited). The zombie threshold in that branch falls back to MAX_TIMER_DELAY_MS * 2 = 120 s. A job intentionally running for hours without a timeout will have its runningAtMs marker cleared after 2 minutes, allowing collectRunnableJobs to pick it up again on the very next tick and start a second concurrent instance.

The fix should skip zombie detection (or use a much larger sentinel) when jobTimeoutMs is undefined, consistent with the semantics of executeJobCoreWithTimeout which already skips the timeout entirely for undefined:

const threshold =
  typeof jobTimeoutMs === "number" && jobTimeoutMs > 0
    ? jobTimeoutMs * 2
    : zombieHardCapMs;

→ change to skip the zombie check entirely when jobTimeoutMs is undefined:

if (typeof jobTimeoutMs !== "number" || jobTimeoutMs <= 0) {
  continue; // no timeout configured → never treat as zombie
}
const threshold = jobTimeoutMs * 2;

This is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 712-723

Comment:
**2-minute hard cap fires for explicitly-disabled-timeout jobs**

`resolveCronJobTimeoutMs` returns `undefined` when a job is configured with `timeoutSeconds <= 0` (explicitly unlimited). The zombie threshold in that branch falls back to `MAX_TIMER_DELAY_MS * 2 = 120 s`. A job intentionally running for hours without a timeout will have its `runningAtMs` marker cleared after 2 minutes, allowing `collectRunnableJobs` to pick it up again on the very next tick and start a second concurrent instance.

The fix should skip zombie detection (or use a much larger sentinel) when `jobTimeoutMs` is `undefined`, consistent with the semantics of `executeJobCoreWithTimeout` which already skips the timeout entirely for `undefined`:

```ts
const threshold =
  typeof jobTimeoutMs === "number" && jobTimeoutMs > 0
    ? jobTimeoutMs * 2
    : zombieHardCapMs;
```
→ change to skip the zombie check entirely when `jobTimeoutMs` is `undefined`:
```ts
if (typeof jobTimeoutMs !== "number" || jobTimeoutMs <= 0) {
  continue; // no timeout configured → never treat as zombie
}
const threshold = jobTimeoutMs * 2;
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 5eb7e17. Applied the exact suggestion — jobs with undefined/no timeout are now continued past, so they're never treated as zombies. The zombieHardCapMs fallback has been removed entirely.

Also added a test (does NOT clear runningAtMs for unlimited-timeout jobs) that verifies a job with timeoutSeconds: 0 running for 2+ hours keeps its marker intact.

Unused import

CronService is imported but not referenced anywhere in the test file.

(remove the import)

This is a comment left during a code review.
Path: src/cron/service.start-error-resilience.test.ts
Line: 4

Comment:
**Unused import**

`CronService` is imported but not referenced anywhere in the test file.

```suggestion
```
(remove the import)

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 5eb7e17. Removed the unused CronService import.

Second test covers pre-existing startup cleanup, not the new onTimer() zombie detection

The start() function already clears stale runningAtMs markers in its initial locked() block before runMissedJobs runs. This test exercises that existing path, not the new zombie-detection logic added to onTimer(). The new onTimer() code (the for (const job of state.store?.jobs ?? []) loop that clears markers at runtime) has no test coverage in this PR.

This is a comment left during a code review.
Path: src/cron/service.start-error-resilience.test.ts
Line: 72-121

Comment:
**Second test covers pre-existing startup cleanup, not the new `onTimer()` zombie detection**

The `start()` function already clears stale `runningAtMs` markers in its initial `locked()` block before `runMissedJobs` runs. This test exercises that existing path, not the new zombie-detection logic added to `onTimer()`. The new `onTimer()` code (the `for (const job of state.store?.jobs ?? [])` loop that clears markers at runtime) has no test coverage in this PR.

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 5eb7e17. Added two new tests in a separate describe block CronService onTimer() zombie detection:

1. clears zombie runningAtMs markers for jobs that exceeded their timeout — exercises the new onTimer() zombie-detection code by setting a stale runningAtMs on a job that's past due, then calling onTimer() to verify the marker is cleared.

2. does NOT clear runningAtMs for unlimited-timeout jobs — verifies the zombie detection skips jobs with timeoutSeconds: 0.

The existing startup cleanup test is kept as-is since it still provides value (regression guard for the startup path).

Recover startup catch-up state before swallowing errors

start() now always continues after runMissedJobs() throws, but runMissedJobs() performs non-atomic writes: planStartupCatchup() persists runningAtMs markers before execution, and a later failure (for example in applyStartupCatchupOutcomes persistence) can leave those markers on disk. Continuing startup from this catch block arms the scheduler against that partially-updated state, so onTimer() force-reloads jobs that still look running and skips them, which can stall missed jobs for a long time (or replay them later once a stale marker is eventually cleared). The catch path should repair/reload state (or rethrow) before proceeding.

Useful? React with 👍 / 👎.

Fixed in 820a84ef. The catch block now repairs partial state left by the failed catch-up:

1. Clears stale runningAtMs markers — planStartupCatchup() persists runningAtMs before execution. If a later step throws, the catch block reloads from disk and clears those markers so jobs can run on the very next tick instead of being stuck until the runtime zombie detector fires.

2. Clears nextRunAtMs for interrupted one-shot jobs — interruptedOneShotIds was computed in the first locked() block but never persisted if runMissedJobs() throws. The catch block clears nextRunAtMs for those one-shot jobs to prevent double-execution on the first tick.

Added a dedicated test (repairs one-shot job state when runMissedJobs throws) that verifies both runningAtMs and nextRunAtMs are cleared for one-shot jobs after a catch-up failure.

Repair startup catch-up state before swallowing errors

This catch path logs and continues even when runMissedJobs fails after planStartupCatchup() has already persisted runningAtMs markers but before applyStartupCatchupOutcomes() can clear/finalize them. In that partial-write state, subsequent ticks reload the persisted markers and treat those jobs as already running, so missed runs are deferred until stale-marker cleanup (and unlimited-timeout jobs are exempt from the new timeout-based zombie clear), which can leave startup catch-up work stuck for a long window after a transient persistence failure. The startup error path should reload/repair marker state (or rethrow) before arming the timer.

Useful? React with 👍 / 👎.

Fixed in 820a84ef. The catch block now repairs partial state left by the failed catch-up:

1. Clears stale runningAtMs markers — planStartupCatchup() persists runningAtMs before execution. If a later step throws, the catch block reloads from disk and clears those markers so jobs can run on the very next tick instead of being stuck until the runtime zombie detector fires.

2. Clears nextRunAtMs for interrupted one-shot jobs — interruptedOneShotIds was computed in the first locked() block but never persisted if runMissedJobs() throws. The catch block clears nextRunAtMs for those one-shot jobs to prevent double-execution on the first tick.

Added a dedicated test (repairs one-shot job state when runMissedJobs throws) that verifies both runningAtMs and nextRunAtMs are cleared for one-shot jobs after a catch-up failure.

Preserve interrupted one-shot skip list after catch-up failure

If runMissedJobs() throws (for example after planStartupCatchup() has already cleared startup runningAtMs markers), this catch path logs and proceeds to arm the scheduler without preserving interruptedOneShotIds. Those one-shot jobs now have no runningAtMs and still have past-due nextRunAtMs, so the first normal timer tick can execute them via collectRunnableJobs()/isRunnableJob(), violating the intended "one-shot jobs are not retried after interruption" behavior and potentially duplicating one-shot side effects.

Useful? React with 👍 / 👎.

Fixed in 820a84ef. The catch block now repairs partial state left by the failed catch-up:

1. Clears stale runningAtMs markers — planStartupCatchup() persists runningAtMs before execution. If a later step throws, the catch block reloads from disk and clears those markers so jobs can run on the very next tick instead of being stuck until the runtime zombie detector fires.

2. Clears nextRunAtMs for interrupted one-shot jobs — interruptedOneShotIds was computed in the first locked() block but never persisted if runMissedJobs() throws. The catch block clears nextRunAtMs for those one-shot jobs to prevent double-execution on the first tick.

Added a dedicated test (repairs one-shot job state when runMissedJobs throws) that verifies both runningAtMs and nextRunAtMs are cleared for one-shot jobs after a catch-up failure.

Force-reload store before repairing catch-up failure

When runMissedJobs() fails after applyStartupCatchupOutcomes() has already updated state.store in memory but before that state is written, this block can incorrectly conclude nothing is dirty and skip repair persistence. Because ensureLoaded(state, { skipRecompute: true }) does not reload when state.store is already set, the catch path never sees stale on-disk runningAtMs markers from planStartupCatchup(), and the next onTimer() force-reload treats those jobs as still running and skips them for a long window.

Useful? React with 👍 / 👎.

Fixed in 7958f679. Added forceReload: true to the ensureLoaded call in the catch block so it reloads from disk and sees the stale runningAtMs markers that planStartupCatchup() persisted, rather than relying on the in-memory state which may have been updated by applyStartupCatchupOutcomes() before the write failed.

Keep timer arming resilient to repair persist errors

This recovery block can still prevent scheduler startup: if runMissedJobs() threw due a write failure, persist(state) here can throw the same error and bubble out of start() before the later armTimer(state) call. In that scenario the scheduler still dies until restart, so this cleanup persist needs to be best-effort (or separately caught) to preserve the intended "arm timer anyway" behavior.

Useful? React with 👍 / 👎.

Fixed in 7958f679. The repair locked() block is now wrapped in a nested try/catch. If the repair persist also fails (e.g. transient I/O), the error is logged and the scheduler proceeds to arm the timer. Added a dedicated test (arms timer even when repair persist also fails) that verifies both error paths are logged and the timer is still armed.

Guard catch-path persist so startup still arms timer

The startup recovery path still performs await persist(state) without a nested safeguard. If runMissedJobs() failed because writes are currently failing (for example transient I/O or permission errors), this second persist can throw and abort start() before control reaches the later block that calls armTimer(state), so the scheduler can still remain dead under the same class of startup failure this change is meant to tolerate.

Useful? React with 👍 / 👎.

Fixed in 7958f679. Same fix as above - nested try/catch around the repair persist ensures startup always reaches armTimer. See the test arms timer even when repair persist also fails for coverage.

Make the resilience test actually fail runMissedJobs

This test's failure injection does not match the fixture behavior: the job setup here does not trigger a startup stale-marker persist, so there are only two cron-store writes before start() completes. With writeCount > 2, no write is failed, meaning the runMissedJobs() catch path is never exercised and the test can pass even when startup error-recovery logic is broken.

Useful? React with 👍 / 👎.

Fixed in 7958f679. The job fixture has no initial runningAtMs, so the startup stale-marker cleanup does not persist. Writes are: (1) planStartupCatchup persist, (2) applyStartupCatchupOutcomes persist. Changed the threshold to fail on write #2 (writeCount === 2) so the catch path is actually exercised. Also updated the comment to document the actual write sequence.

Preserve applied catch-up outcomes during repair

When runMissedJobs() fails after applyStartupCatchupOutcomes() has already updated state.store in memory but before that write is persisted, this forced reload discards those applied outcomes and reverts to the pre-outcome disk snapshot. The subsequent repair only clears runningAtMs, so jobs that already executed can become runnable again (recurring jobs remain past-due; one-shot jobs can remain enabled) and re-run on the next tick, duplicating side effects such as sends/agent turns. The repair path should keep or merge the in-memory outcomes instead of replacing them with a stale reload.

Useful? React with 👍 / 👎.

Fixed in acd38664. Removed forceReload from the catch block ensureLoaded. If applyStartupCatchupOutcomes already updated in-memory state (cleared runningAtMs for completed jobs, advanced nextRunAtMs) before its persist threw, reloading would discard those outcomes and make completed jobs runnable again. Now we work with the in-memory state and just clear remaining runningAtMs markers from incomplete jobs. Also added one-shot protection: when clearing runningAtMs on at-kind jobs, nextRunAtMs is also cleared to prevent re-execution.

Exclude one-shot jobs from zombie-marker clearing

This zombie cleanup clears runningAtMs for any timed job, including one-shot (schedule.kind === "at") jobs. If a one-shot execution completed but its outcome persist failed, the store can retain a stale runningAtMs plus past-due nextRunAtMs; once this marker is cleared, normal timer selection treats that one-shot as runnable and executes it again, violating the intended no-retry-after-interruption behavior and risking duplicate side effects. The zombie path should skip one-shots or clear their runnable state together with the marker.

Useful? React with 👍 / 👎.

Fixed in acd38664. clearZombieRunningMarkers now clears nextRunAtMs alongside runningAtMs for one-shot (kind=at) jobs, preventing the zombie-clear path from making a one-shot runnable again after its outcome persist failed.

Preserve one-shot skip state after catch-up failure

When runMissedJobs() throws, this block clears nextRunAtMs for interrupted one-shot jobs, but start() then unconditionally calls recomputeNextRuns(state) in the next locked section, which repopulates missing nextRunAtMs for schedule.kind === "at" (unless lastStatus === "ok"). In the common interrupted-run case (lastStatus unset/error), those one-shots become runnable again on the first timer tick and can execute twice, which defeats the intended "do not retry interrupted one-shot" behavior in this recovery path.

Useful? React with 👍 / 👎.

Fixed in 45329efe. recomputeNextRuns in the final locked() block now checks interruptedOneShotIds and re-clears nextRunAtMs for those jobs after recompute runs. This prevents computeJobNextRunAtMs from repopulating nextRunAtMs for one-shots where lastStatus !== ok (which always returns atMs for kind=at jobs).

Preserve one-shot skip state after zombie marker cleanup

Clearing nextRunAtMs here is not sufficient to prevent one-shot re-execution: in the same timer tick, onTimer() can call recomputeNextRunsForMaintenance(...), and for enabled schedule.kind === "at" jobs with lastStatus !== "ok", computeJobNextRunAtMs repopulates nextRunAtMs with the original at time. That makes the zombie-cleared one-shot runnable again on the next tick, so interrupted one-shots can execute twice (duplicate sends/side effects) after a stale-marker recovery.

Useful? React with 👍 / 👎.

Fixed in fb1420fe. After recomputeNextRunsForMaintenance runs in the empty-due branch, I re-check one-shot jobs that had zombie markers cleared. For kind=at jobs with no runningAtMs, no lastRunAtMs, and lastStatus !== ok, nextRunAtMs is cleared again to prevent the interrupted one-shot from being re-enabled. This guard only runs when zombies were actually cleared (clearedZombies flag), so normal maintenance recomputation is unaffected.

Arm timer even when post-recompute persist fails

This block can still leave cron dead on startup: the new interrupted-one-shot cleanup sets changed = true, which forces persist(state) before armTimer(state). If that write throws (for example, a transient I/O failure after runMissedJobs was already caught), start() exits before arming the timer, so scheduling remains down until restart. Making this persist best-effort or guaranteeing armTimer in a finally path would preserve the intended resilience.

Useful? React with 👍 / 👎.

Fixed in 8eec88a3. Wrapped the final locked block's persist in try/catch with error logging, so armTimer always executes regardless of persist failure.

Fixed in 8eec88a3. Wrapped the final locked block persist in try/catch with error logging, so armTimer always executes regardless of persist failure.

Preserve interrupted one-shot retries after zombie cleanup

Fresh evidence beyond the earlier thread: this new guard only re-clears nextRunAtMs when lastRunAtMs is missing, so interrupted one-shot retry runs (which already have lastRunAtMs from a prior failed attempt) are excluded. In that case, recomputeNextRunsForMaintenance repopulates nextRunAtMs from the original at schedule, making the job runnable again on the next tick and allowing duplicate side effects after an interrupted run.

Useful? React with 👍 / 👎.

Fixed in 4976ce7a and a4d57d27. Removed the lastRunAtms check from the one-shot zombie guard so interrupted retries with lastRunAtms from prior failed attempts are also covered. clearZombieRunningMarkers now returns Set of cleared job IDs, scoped to only those jobs.

Restrict one-shot cleanup to zombie-cleared jobs

This clearedZombies branch iterates over all one-shot jobs and clears nextRunAtMs whenever lastStatus !== "ok", even if that job’s runningAtMs was never a zombie in this tick. In a tick where one unrelated zombie marker is cleared and due.length === 0, this can wipe scheduled one-shot retry backoff (lastStatus: "error" with a future nextRunAtMs) for other jobs, so those retries are later recomputed from the original at time and can run much earlier than intended. The post-maintenance re-clear should be scoped to the specific jobs whose zombie marker was actually cleared.

Useful? React with 👍 / 👎.