Skip to content

fix(webhook-agent-ingest): prevent duplicate cron alarm firings - #5523

Merged
pandemicsyn merged 2 commits into
mainfrom
session/agent_49f591d0-2c90-4239-a2c6-d0b5c1135455
Aug 26, 2026
Merged

fix(webhook-agent-ingest): prevent duplicate cron alarm firings#5523
pandemicsyn merged 2 commits into
mainfrom
session/agent_49f591d0-2c90-4239-a2c6-d0b5c1135455

Conversation

@pandemicsyn

Copy link
Copy Markdown
Contributor

Summary

Fixes a bug where a Cloud Agent scheduled webhook trigger could fire twice for the same cron occurrence — one invocation up to 30s early, followed by a duplicate ~1 minute later (as seen in the screenshot on the "Scheduled requests" table, e.g. 12:09 and 12:10).

Root cause, in services/webhook-agent-ingest/src/dos/TriggerDO.ts:

  • scheduleNextAlarm() applied jitter in the range ±30s when scheduling the Durable Object alarm for the next cron occurrence. Negative jitter meant the alarm could fire before its target time.
  • When the alarm fired early, the reschedule logic recomputed "next occurrence" relative to "now" — but "now" was still before the intended occurrence, so croner returned the same occurrence again, causing it to be scheduled (and fired) a second time.

Fix:

  • computeNextCronTime() (services/webhook-agent-ingest/src/util/cron.ts) now accepts an optional after reference Date, passed through to croner's nextRun(after).
  • scheduleNextAlarm() jitter changed from ±30s to 0–30s (delay-only, never negative), so the alarm can no longer fire ahead of its target occurrence.
  • scheduleNextAlarm() also anchors on max(now, previousTarget) when a valid future nextScheduledAt exists, so even a slightly-early alarm fire (clock skew, platform-level timing) correctly advances to the next occurrence instead of recomputing the one just handled.
  • updateConfig()'s "reactivated or cron expression changed" branch now clears the inherited nextScheduledAt before rescheduling — otherwise a pending (never-fired) occurrence carried over from the old config would get treated as "already consumed" by the new anchoring logic, silently skipping a valid, possibly sooner occurrence under the new cron expression/timezone.

No architectural changes — this is a targeted fix within the existing Durable Object alarm-based scheduling design.

Verification

  • No manual verification was performed. Triggering this bug requires waiting for a live scheduled cron occurrence against a deployed Durable Object (or advancing DO alarm time in a real environment), which wasn't practical in this session. Verification relied on unit tests exercising computeNextCronTime()'s new after parameter, including early-fire, far-future-anchor, and invalid-date cases, plus reasoning through all call sites of scheduleNextAlarm() (initial configure, normal alarm fire, DST-retry branch, and config update) via a static review pass.
  • [ ]

Visual Changes

N/A

Reviewer Notes

  • Core logic lives in TriggerDO.scheduleNextAlarm() and alarm() — worth tracing through all four call sites (initial configure(), updateConfig()'s reactivate/cron-change branch, the normal alarm() fire path, and the DST alarmRetry retry path) to confirm the anchoring behaves correctly in each.
  • nextScheduledAt is dual-purpose (UI display value and internal "occurrence already handled" marker); the updateConfig() fix clears it explicitly where it would otherwise be misread as "already fired" for a config that hasn't fired yet.
  • No test harness exists for TriggerDO's Durable Object alarm scheduling directly (only cron.ts unit tests); the DO-level behavior (config update reschedule, DST retry) was verified by code reading rather than an integration test.

pandemicsyn and others added 2 commits August 26, 2026 17:31
Fix a race condition where cron alarms could fire twice for the same
occurrence if the alarm triggered slightly early due to jitter.

The fix involves:
- Anchoring the next scheduled time calculation on the previously
  targeted occurrence (`nextScheduledAt`) if it is still in the future,
  ensuring the scheduler always advances to the subsequent occurrence.
- Changing the jitter implementation to be strictly positive (0-30s)
  to prevent the alarm from being scheduled before its target time.
- Updating `computeNextCronTime` to accept an optional reference date.
- Adding unit tests to verify that the next occurrence is strictly after
  the provided reference time.

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
…update

Address static review findings on the previous duplicate-alarm fix:
- updateConfig()'s reactivate/cron-change branch cleared nextScheduledAt
  before rescheduling, since it previously carried a pending (never
  fired) occurrence from the old config that would incorrectly get
  skipped by the new anchoring logic.
- Use an explicit Number.isNaN check instead of relying on incidental
  NaN comparison behavior when validating the stored nextScheduledAt.
- Add cron.test.ts coverage for far-future anchors across day
  boundaries and for an Invalid Date passed as the reference time.

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
@kilo-code-bot

kilo-code-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Reviewed the scheduled-cron double-fire fix in TriggerDO alarm scheduling and computeNextCronTime; delay-only jitter, future-anchor, and updateConfig reset look correct with high confidence.

Files Reviewed (3 files)
  • services/webhook-agent-ingest/src/dos/TriggerDO.ts
  • services/webhook-agent-ingest/src/util/cron.ts
  • services/webhook-agent-ingest/src/util/cron.test.ts

Reviewed by grok-4.6 · Input: 61K · Output: 12.9K · Cached: 386.7K

Review guidance: REVIEW.md from base branch main

@pandemicsyn
pandemicsyn merged commit 52b0576 into main Aug 26, 2026
13 checks passed
@pandemicsyn
pandemicsyn deleted the session/agent_49f591d0-2c90-4239-a2c6-d0b5c1135455 branch August 26, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants