Repository navigation
Add scheduler alarm diagnostics - #240
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds structured scheduler logging and error normalization across job creation, Durable Object alarm lifecycle, RPC-based alarm sync, job execution, and outcome reporting; exports JobManagerBase and expands processDueJobs/runDueJobsForUser results to include per-job outcome aggregates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagerRPC
participant JobManagerDO
participant SchedulerLog
Client->>ManagerRPC: syncJobManagerAlarm(env,userId)
Note right of ManagerRPC: logs sync_alarm_requested
ManagerRPC->>JobManagerDO: rpc.syncAlarm(env,userId)
JobManagerDO->>SchedulerLog: log sync_alarm (armed/no-runnable/reason)
JobManagerDO-->>ManagerRPC: { ok, nextRunAt }
ManagerRPC->>SchedulerLog: log sync_alarm_completed (userId,nextRunAt,reason)
ManagerRPC-->>Client: { ok, nextRunAt }
alt RPC unavailable
ManagerRPC->>SchedulerLog: log sync_alarm_skipped_missing_binding (userId,reason)
ManagerRPC-->>Client: { ok: true, nextRunAt: null }
end
alt error during RPC
ManagerRPC->>SchedulerLog: log sync_alarm_request_failed (schedulerErrorFields)
ManagerRPC-->>Client: throw error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
April 20, 2026 22:08
Contributor
|
🔎 Preview deployed: https://kody-pr-240.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/jobs/process-due-jobs.ts`:
- Around line 85-101: The current catch block treats a reschedule failure as a
success because jobOutcomes uses outcome.execution.ok; change the job outcome
logic so reschedule failures are counted as failures: compute a finalOutcome
that is 'failure' if rescheduleError is truthy (even when outcome.execution.ok
is true) and use that finalOutcome when pushing into jobOutcomes; also ensure
applyExecutionOutcome/update logic still disables the job and records
lastRunError (keep the existing applyExecutionOutcome call that sets
enabled:false and lastRunError) so the stored job reflects the reschedule
failure while the jobOutcomes summary reflects a failure.
In `@packages/worker/src/jobs/scheduler-logging.ts`:
- Around line 55-87: The logs currently serialize the full
JobSchedulerLogPayload in writeSchedulerLog (used by
logJobSchedulerEvent/logJobSchedulerError) which allows unbounded
user/job-provided strings; before JSON.stringify, centrally truncate/sanitize
input.errorMessage and each entry in input.jobOutcomes[*] (specifically fields
named error and rescheduleError) to a fixed max length (e.g.
MAX_LOG_STRING_LENGTH) and replace with a capped version (or "[TRUNCATED]") so
summarizeSchedulerJobOutcomes still controls count while writeSchedulerLog
enforces per-field size limits and avoids oversized or sensitive payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4791203-c12d-477a-8f42-e4bffbad0b15
📒 Files selected for processing (8)
packages/worker/src/jobs/manager-client.tspackages/worker/src/jobs/manager-do.node.test.tspackages/worker/src/jobs/manager-do.tspackages/worker/src/jobs/process-due-jobs.node.test.tspackages/worker/src/jobs/process-due-jobs.tspackages/worker/src/jobs/scheduler-logging.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/jobs/shared.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
Bot
force-pushed
the
cursor/scheduler-alarm-diagnostics-eb7d
branch
from
April 21, 2026 01:42
79ec26c to
8060d22
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/worker/src/jobs/scheduler-logging.node.test.ts (1)
37-130: Console override is not isolated across concurrent tests.Each test stubs
console.errordirectly (lines 41–44, 68–70, 111–113) and restores it infinally. If Vitest ever runs tests in this file concurrently (e.g.,test.concurrentor a future concurrency flip), these stubs will clobber each other and capture the wrong payload. Consider usingvi.spyOn(console, 'error').mockImplementation(...)inside abeforeEach/afterEachpair, or wrap each stub in a helper that returns a per-test capture closure — this also removes the need for theas typeof console.errorcasts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/scheduler-logging.node.test.ts` around lines 37 - 130, Tests directly reassign console.error which can race between concurrent tests; change the stubs to per-test spies using vi.spyOn(console, 'error').mockImplementation(...) and restore them in afterEach (or use beforeEach/afterEach) so each test (e.g., the tests calling logJobSchedulerError in this file) gets an isolated mock and no global reassignment occurs; ensure you capture the mock's calls (mock.calls) to get tag/json and call mockRestore() in cleanup.packages/worker/src/jobs/manager-do.ts (1)
93-98:alarm_firedemitted before the try/catch wrappingrunDueJobsForUser.If
logJobSchedulerEventitself ever throws (e.g. JSON.stringify on a cyclic payload) on line 93 or on line 85, the alarm handler propagates that error to the Durable Object runtime without resetting the alarm — you'll lose the fire entirely. Given the current payload fields are all primitives, this is very unlikely in practice, but it's worth being aware that all logging calls in this file are outside the top-level error boundary. No change required if you're confident the payloads stay serialization-safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/manager-do.ts` around lines 93 - 98, The logJobSchedulerEvent calls (e.g., the call emitting 'alarm_fired' using alarmInfo.retryCount/isRetry) are currently executed outside the top-level try/catch that wraps runDueJobsForUser, so any serialization error could propagate and lose the alarm; wrap each logJobSchedulerEvent invocation in a defensive try/catch (or use a small safeLog wrapper) so logging failures are swallowed or routed to a fallback logger and do not throw, and/or move the logJobSchedulerEvent calls inside the existing try/catch that surrounds runDueJobsForUser to ensure the Durable Object alarm is always reset even if logging fails.packages/worker/src/jobs/manager-do.node.test.ts (2)
66-100:createStatetreatsnulluserId as a valid stored value.
userId?: string | nullwithcurrentAlarmAt=nulland the guardif (userId !== undefined) persistedEntries.set('user-id', userId)(line 74) means passing{ userId: null }persists'user-id' → null, which then failsif (!userId)insidealarm()and exercises the missing-user branch — but no test uses it, and the type allowsnullwithout any callsite actually passing it. Either tighten the type touserId?: string(so tests always persist a real id) or add the missing-user test that would justify thenullbranch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/manager-do.node.test.ts` around lines 66 - 100, createState currently allows userId: string | null and then stores null as 'user-id', which incorrectly exercises the missing-user branch in alarm(); change the createState signature to userId?: string (remove | null) and ensure persistedEntries only gets set when userId is not undefined (keep the existing guard), so tests always persist a real id and the null branch is not accidentally exercised; update the createState parameter type and any callsites in the test file that pass null to instead omit the property or pass a real string.
102-240: Test coverage gap: error paths and missing-userId branch are not exercised.The suite covers only the happy paths of
syncAlarm(with and without a next runnable job) and ofalarm(). None of the new scheduler error branches introduced in this PR are covered:
sync_alarm_failed— triggered whengetNextRunnableJob/setAlarm/deleteAlarmthrows.alarm_run_due_jobs_failed— triggered whenrunDueJobsForUserrejects.alarm_resync_failed— triggered when the post-executionsyncAlarmrejects.alarm_firedwithreason: 'missing-user-id'— triggered when storage has no persisteduser-id.runNowpath (still catches sync errors viaconsole.error).Since the primary value of this PR is observability of failure modes, adding at least one test per error branch (asserting
logJobSchedulerErroris called with the righteventanderrorMessage) would lock in the contract the dashboards will rely on.Want me to draft tests for the three error events and the missing-userId branch?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/manager-do.node.test.ts` around lines 102 - 240, Add tests to cover the error branches introduced in JobManagerBase: simulate failures in getNextRunnableJob/setAlarm/deleteAlarm to assert syncAlarm emits a logJobSchedulerError with event 'sync_alarm_failed' and the errorMessage; simulate runDueJobsForUser rejecting to assert alarm emits 'alarm_run_due_jobs_failed'; simulate the post-execution syncAlarm rejecting to assert alarm emits 'alarm_resync_failed'; add a test where persistedEntries lacks 'user-id' to exercise alarm logging 'alarm_fired' with reason 'missing-user-id'; for each test ensure you mock the corresponding module function (getNextRunnableJob, setAlarm, deleteAlarm, runDueJobsForUser, and JobManagerBase.syncAlarm as needed) and assert mockModule.logJobSchedulerError was called with the correct event and errorMessage and that other success-path logs are not relied upon.packages/worker/src/jobs/service.ts (1)
1017-1025: O(n·m) lookup while persisting due-job results.
dueRows.find(...)is called inside theforloop for every saved job (and similarly on line 1008 insideexecuteJob). For a handful of due jobs this is fine, but if the batch ever grows, this becomes quadratic with no guard. Consider building aMap<jobId, dueRow>once beforeprocessDueJobsand reusing it for both the executor closure and the save/delete loops.Proposed refactor
+ const dueRowById = new Map(dueRows.map((row) => [row.record.id, row] as const)) const result = await processDueJobs({ jobs: dueRows.map((row) => row.record), now, executeJob: async (job) => { - const row = dueRows.find((candidate) => candidate.record.id === job.id) + const row = dueRowById.get(job.id) const callerContext = row?.callerContext ?? null return executeJobOnce({ env: input.env, job, callerContext }) }, }) for (const job of result.saveJobs) { - const row = dueRows.find((candidate) => candidate.record.id === job.id) + const row = dueRowById.get(job.id) await updateJobRow({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.ts` around lines 1017 - 1025, The code currently does repeated O(n·m) lookups using dueRows.find(...) for each job; create a Map keyed by job id (e.g., const dueRowById = new Map(dueRows.map(r => [r.record.id, r]))) once at the start of processDueJobs and replace all dueRows.find(...) usages (including in the executor passed to executeJob and the loop over result.saveJobs) with dueRowById.get(job.id); preserve the existing fallback for callerContextJson (row?.callerContextJson ?? 'null') when reading from the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/jobs/manager-client.ts`:
- Around line 41-75: The logs/events use mixed case conventions; standardize to
snake_case across related files so consumers see one format. Update all emitted
event and reason strings to snake_case (e.g., in manager-do.ts change
'no-runnable-job' -> 'no_runnable_job', 'alarm-armed' -> 'alarm_armed', etc.) to
match existing values emitted by manager-client.ts (which emits
'sync_alarm_skipped_missing_binding', 'no_runnable_job_found',
'alarm_state_updated'), and ensure callers/emitters such as
logJobSchedulerEvent, logJobSchedulerError, rpc.syncAlarm, schedulerErrorFields,
and any service handlers (e.g., run_due_jobs.empty in service.ts) use the same
snake_case tokens so logging is consistent across the codebase.
In `@packages/worker/src/jobs/manager-do.node.test.ts`:
- Around line 44-53: Replace the inline typeof import(...) type annotation in
the vi.mock callback with a top-level type import: add a top-level "import type"
for the module (e.g., import type * as SchedulerLoggingType from
'./scheduler-logging.ts') and then change the generic on importOriginal to use
that type (importOriginal<typeof SchedulerLoggingType>); keep the existing
mockModule forwarding for logJobSchedulerEvent and logJobSchedulerError and
ensure the vi.mock callback signature still uses importOriginal typed by the new
top-level type.
In `@packages/worker/src/jobs/manager-do.ts`:
- Around line 104-112: The log emitted by logJobSchedulerEvent in manager-do
uses inconsistent event and reason strings compared to service.ts; normalize
them to the same convention by changing the event and reason values used in the
logJobSchedulerEvent call (refer to logJobSchedulerEvent and the reason
conditional producing 'no-due-jobs' / 'processed-due-jobs') to match the
canonical names used in service.ts (e.g., use 'run_due_jobs.empty' and
'no_due_jobs_found' or whatever service.ts uses) so all scheduler logging (event
and reason) is consistent across manager-do and service.ts.
- Around line 22-76: The inner catch in syncAlarm is emitting sync_alarm_failed
and then rethrowing, causing duplicate error entries when callers (e.g.,
alarm()) also log; modify syncAlarm to accept a source discriminator (e.g.,
source: 'alarm' | 'rpc' | 'runNow') and include that source in the
sync_alarm_failed log payload instead of removing the log; update the
logJobSchedulerError call inside the catch in syncAlarm to add source, and
update all callers (alarm(), RPC entry, runNow path) to pass the appropriate
source when invoking syncAlarm so downstream logs can be de-duped by source.
In `@packages/worker/src/jobs/service.ts`:
- Around line 990-1003: The scheduler event emitted by logJobSchedulerEvent uses
dot notation and a mismatched reason; update the logJobSchedulerEvent call (the
object with keys event and reason) so both fields use the project's snake_case
convention to match other events (e.g., change event to run_due_jobs_empty and
reason to no_due_jobs) and/or reconcile the reason with the equivalent emission
in manager-do.ts (no-due-jobs) so the same underlying condition produces a
single searchable value across the codebase.
---
Nitpick comments:
In `@packages/worker/src/jobs/manager-do.node.test.ts`:
- Around line 66-100: createState currently allows userId: string | null and
then stores null as 'user-id', which incorrectly exercises the missing-user
branch in alarm(); change the createState signature to userId?: string (remove |
null) and ensure persistedEntries only gets set when userId is not undefined
(keep the existing guard), so tests always persist a real id and the null branch
is not accidentally exercised; update the createState parameter type and any
callsites in the test file that pass null to instead omit the property or pass a
real string.
- Around line 102-240: Add tests to cover the error branches introduced in
JobManagerBase: simulate failures in getNextRunnableJob/setAlarm/deleteAlarm to
assert syncAlarm emits a logJobSchedulerError with event 'sync_alarm_failed' and
the errorMessage; simulate runDueJobsForUser rejecting to assert alarm emits
'alarm_run_due_jobs_failed'; simulate the post-execution syncAlarm rejecting to
assert alarm emits 'alarm_resync_failed'; add a test where persistedEntries
lacks 'user-id' to exercise alarm logging 'alarm_fired' with reason
'missing-user-id'; for each test ensure you mock the corresponding module
function (getNextRunnableJob, setAlarm, deleteAlarm, runDueJobsForUser, and
JobManagerBase.syncAlarm as needed) and assert mockModule.logJobSchedulerError
was called with the correct event and errorMessage and that other success-path
logs are not relied upon.
In `@packages/worker/src/jobs/manager-do.ts`:
- Around line 93-98: The logJobSchedulerEvent calls (e.g., the call emitting
'alarm_fired' using alarmInfo.retryCount/isRetry) are currently executed outside
the top-level try/catch that wraps runDueJobsForUser, so any serialization error
could propagate and lose the alarm; wrap each logJobSchedulerEvent invocation in
a defensive try/catch (or use a small safeLog wrapper) so logging failures are
swallowed or routed to a fallback logger and do not throw, and/or move the
logJobSchedulerEvent calls inside the existing try/catch that surrounds
runDueJobsForUser to ensure the Durable Object alarm is always reset even if
logging fails.
In `@packages/worker/src/jobs/scheduler-logging.node.test.ts`:
- Around line 37-130: Tests directly reassign console.error which can race
between concurrent tests; change the stubs to per-test spies using
vi.spyOn(console, 'error').mockImplementation(...) and restore them in afterEach
(or use beforeEach/afterEach) so each test (e.g., the tests calling
logJobSchedulerError in this file) gets an isolated mock and no global
reassignment occurs; ensure you capture the mock's calls (mock.calls) to get
tag/json and call mockRestore() in cleanup.
In `@packages/worker/src/jobs/service.ts`:
- Around line 1017-1025: The code currently does repeated O(n·m) lookups using
dueRows.find(...) for each job; create a Map keyed by job id (e.g., const
dueRowById = new Map(dueRows.map(r => [r.record.id, r]))) once at the start of
processDueJobs and replace all dueRows.find(...) usages (including in the
executor passed to executeJob and the loop over result.saveJobs) with
dueRowById.get(job.id); preserve the existing fallback for callerContextJson
(row?.callerContextJson ?? 'null') when reading from the map.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d26bf934-8615-43ef-8b35-c9faa9e19b5e
📒 Files selected for processing (9)
packages/worker/src/jobs/manager-client.tspackages/worker/src/jobs/manager-do.node.test.tspackages/worker/src/jobs/manager-do.tspackages/worker/src/jobs/process-due-jobs.node.test.tspackages/worker/src/jobs/process-due-jobs.tspackages/worker/src/jobs/scheduler-logging.node.test.tspackages/worker/src/jobs/scheduler-logging.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/jobs/shared.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/worker/src/mcp/capabilities/jobs/shared.ts
- packages/worker/src/jobs/process-due-jobs.node.test.ts
- packages/worker/src/jobs/scheduler-logging.ts
- packages/worker/src/jobs/process-due-jobs.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc2230e. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mainbranch changes, preserving new job inspection/debug capabilities alongside the scheduler diagnosticscreateJobprocessDueJobs,JobManageralarm logging behavior/error branches, scheduler log sanitization, and the new alarm-state helper frommainTesting
npx vitest --config vitest.node.config.ts "packages/worker/src/jobs/manager-do.node.test.ts" "packages/worker/src/jobs/service.node.test.ts" "packages/worker/src/mcp/capabilities/jobs/job-schedule.node.test.ts"npx vitest --config vitest.node.config.ts "packages/worker/src/jobs/process-due-jobs.node.test.ts" "packages/worker/src/jobs/scheduler-logging.node.test.ts" "packages/worker/src/jobs/manager-do.node.test.ts" "packages/worker/src/mcp/capabilities/jobs/job-schedule.node.test.ts"npm run typecheckWalkthrough
Summary by CodeRabbit
New Features
Bug Fixes
Tests