Skip to content

improvement(cron): fire-and-forget for cron-invoked endpoints#4764

Merged
TheodoreSpeaks merged 3 commits into
stagingfrom
feat/schedule-fire-forget
May 27, 2026
Merged

improvement(cron): fire-and-forget for cron-invoked endpoints#4764
TheodoreSpeaks merged 3 commits into
stagingfrom
feat/schedule-fire-forget

Conversation

@TheodoreSpeaks
Copy link
Copy Markdown
Collaborator

@TheodoreSpeaks TheodoreSpeaks commented May 27, 2026

Summary

  • schedules/execute, the webhook poll routes, notifications/poll, and renew-subscriptions now ack the cron caller immediately with 202 and run the work detached in the background, so the caller's request timeout never trips and retries can't pile duplicate runs on the server
  • Added lib/core/utils/background.ts (runDetached); extracted runScheduleTick from the schedule route; renew-subscriptions gained a Redis lock (it had none)
  • Schedule overlap is deduped by the existing FOR UPDATE SKIP LOCKED row claiming (+ advisory locks in DB-fallback mode); no in-process guard needed. Poll/notifications keep their existing Redis lock, with release moved into the detached finally

Type of Change

  • Improvement

Testing

Tested manually. bun run lint, bun run check:api-validation:strict, and unit/route tests across the touched files all pass.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 27, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 27, 2026 11:07pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 27, 2026

PR Summary

Medium Risk
Touches schedule execution, webhook/notification polling, and OAuth subscription renewal—operational paths where duplicate or partial runs matter—but overlapping work is mitigated by existing DB claims and Redis locks on most routes.

Overview
Cron-triggered long-running routes now ack immediately with HTTP 202 (status: 'started' or 'skip') instead of blocking until work finishes, so short cron client timeouts do not abort processing or encourage duplicate retries.

A new runDetached helper runs the real work in the background and logs failures without unhandled rejections. schedules/execute pulls the drain loop into exported runScheduleTick; the GET handler only authenticates and starts the tick detached (schedule safety still relies on FOR UPDATE SKIP LOCKED claiming, not a route-level Redis lock). Webhook poll, notifications poll, and Teams renew-subscriptions follow the same pattern; poll routes move Redis lock release into the detached task’s finally. Renew-subscriptions additionally gains a Redis lock where there was none before.

The schedules API contract response drops processedCount in favor of status: 'started'. Route and unit tests were added/updated for auth, 202/skip, lock behavior, and direct tick/poll testing.

Reviewed by Cursor Bugbot for commit 82d194c. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit cb5b5f4. Configure here.

Comment thread apps/sim/app/api/schedules/execute/route.ts Outdated
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 27, 2026

Greptile Summary

This PR converts four cron-invoked endpoints (schedules/execute, webhooks/poll/[provider], notifications/poll, renew-subscriptions) to a fire-and-forget pattern: each handler now acquires any necessary Redis lock synchronously, launches work via the new runDetached utility, and immediately returns 202 Accepted so the cron caller never hits a request timeout.

  • lib/core/utils/background.ts introduces runDetached(label, work), which schedules work as a microtask continuation (Promise.resolve().then(work)) and catches both sync throws and async rejections, preventing unhandled-rejection crashes while preserving AsyncLocalStorage context for logging.
  • schedules/execute extracts the tick loop into runScheduleTick(requestId) (exported for unit testing); cross-replica deduplication continues to rely on FOR UPDATE SKIP LOCKED row claiming, not in-process guards.
  • renew-subscriptions gains a Redis lock it previously lacked, using the same acquire-before-respond / release-in-finally structure shared by the other routes.

Confidence Score: 5/5

Safe to merge — the fire-and-forget refactor is mechanically correct across all four routes, lock release is guarded by finally in every background task, and the new runDetached utility correctly handles both sync and async failures.

The core pattern (acquire lock synchronously → return 202 → release lock in detached finally) is consistently applied. Error paths are covered by tests for three of four routes, and the runDetached utility itself has its own unit tests. No correctness issues were found.

No files require special attention. The only gaps are minor: a missing error-path test in renew-subscriptions/route.test.ts and a redundant alias in the webhook poll route.

Important Files Changed

Filename Overview
apps/sim/lib/core/utils/background.ts New runDetached utility: schedules work via Promise.resolve().then(work) so sync throws are promoted to rejections and caught; correctly preserves AsyncLocalStorage context for logging.
apps/sim/app/api/schedules/execute/route.ts Extracts runScheduleTick (exported for tests) and fires it detached; FOR UPDATE SKIP LOCKED still prevents cross-replica duplicates; 202 returned immediately.
apps/sim/app/api/cron/renew-subscriptions/route.ts Added Redis lock (previously missing), extracted renewExpiringSubscriptions, fires detached; lock is released in background finally.
apps/sim/app/api/cron/renew-subscriptions/route.test.ts New test file covers auth failure, lock-acquired happy path, and lock-already-held skip; missing a test verifying lock release when renewExpiringSubscriptions throws.
apps/sim/app/api/notifications/poll/route.ts Lock acquired synchronously before detaching; release moved into the detached finally; remaining sync path (auth, lock check) still covered by the outer try-catch.
apps/sim/app/api/webhooks/poll/[provider]/route.ts Same fire-and-forget pattern applied; pollingProvider alias is an unnecessary intermediate variable — provider is already in scope for the closure.

Sequence Diagram

sequenceDiagram
    participant Cron as Cron Caller
    participant Handler as Route Handler
    participant Redis as Redis (Lock)
    participant BG as Background Task

    Cron->>Handler: GET (with auth header)
    Handler->>Handler: verifyCronAuth()
    Handler->>Redis: acquireLock(key, value, TTL)
    Redis-->>Handler: true / false

    alt Lock already held
        Handler-->>Cron: "202 { status: 'skip' }"
    else Lock acquired
        Handler->>BG: runDetached(label, work)
        Note over BG: Runs detached — caller does not await
        Handler-->>Cron: "202 { status: 'started' }"
        BG->>BG: do work (DB queries, Graph API, polling)
        BG->>Redis: releaseLock(key, value) [in finally]
    end
Loading

Reviews (2): Last reviewed commit: "improvement(cron): drop single-flight gu..." | Re-trigger Greptile

Comment thread apps/sim/lib/api/contracts/schedules.ts
@TheodoreSpeaks
Copy link
Copy Markdown
Collaborator Author

@greptile review

@TheodoreSpeaks TheodoreSpeaks merged commit 7ddd90b into staging May 27, 2026
14 checks passed
@waleedlatif1 waleedlatif1 deleted the feat/schedule-fire-forget branch May 29, 2026 01:04
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.

1 participant