fix(server): throttle and back off heartbeat scheduler errors (#10911) - #11086
fix(server): throttle and back off heartbeat scheduler errors (#10911)#11086wakqasahmed wants to merge 2 commits into
Conversation
|
Hey @wakqasahmed! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Greptile SummaryThe PR adds deduplicated scheduler error logging and replaces the fixed heartbeat interval with adaptive timeout-based scheduling. It also adds unit tests for throttling and the intended backoff sequence.
Confidence Score: 4/5The scheduler backoff should be corrected before merging because most background-task failures never reach its failure counter. Individual scheduler jobs consume their own rejections, while only rejection of the outer callback increments the new consecutive-failure state, leaving affected jobs at the normal cadence. Files Needing Attention: server/src/index.ts
|
| Filename | Overview |
|---|---|
| server/src/index.ts | Integrates throttling and adaptive scheduling, but locally handled task failures do not update the backoff counter. |
| server/src/lib/error-throttler.ts | Adds error summarization, keyed suppression, periodic suppressed-count logging, and reset operations. |
| server/src/tests/error-throttler.test.ts | Covers summarization, suppression, summary emission, distinct keys, and reset behavior. |
| server/src/tests/heartbeat-scheduler-throttling.test.ts | Demonstrates throttler behavior and duplicates the backoff formula, but does not exercise the production scheduler control flow where failures are consumed. |
Prompt To Fix All With AI
### Issue 1
server/src/index.ts:949-955
**Task failures bypass scheduler backoff**
When scheduled jobs such as `tickTimers` or `tickScheduledTriggers` reject, their local catch handlers consume the errors, so `await callback()` resolves without incrementing `consecutiveSchedulerFailures`. The scheduler therefore continues at its base interval instead of applying exponential backoff while those jobs repeatedly fail.
### Issue 2
server/src/__tests__/heartbeat-scheduler-throttling.test.ts:5
**PR description omits required sections**
The PR description does not include the required Thinking Path, Verification, Risks, or Model Used sections. Please add the complete template information so maintainers can evaluate how the scheduler change was verified and what operational risks remain.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(server): throttle and back off heart..." | Re-trigger Greptile
| if (consecutiveSchedulerFailures > 0) { | ||
| consecutiveSchedulerFailures = 0; | ||
| schedulerThrottler.reset(); | ||
| } | ||
| } catch (err) { | ||
| consecutiveSchedulerFailures += 1; | ||
| schedulerThrottler.logError(logger, "heartbeat scheduler tick failed", err); |
There was a problem hiding this comment.
Task failures bypass scheduler backoff
When scheduled jobs such as tickTimers or tickScheduledTriggers reject, their local catch handlers consume the errors, so await callback() resolves without incrementing consecutiveSchedulerFailures. The scheduler therefore continues at its base interval instead of applying exponential backoff while those jobs repeatedly fail.
Knowledge Base Used: Server HTTP App
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/index.ts
Line: 949-955
Comment:
**Task failures bypass scheduler backoff**
When scheduled jobs such as `tickTimers` or `tickScheduledTriggers` reject, their local catch handlers consume the errors, so `await callback()` resolves without incrementing `consecutiveSchedulerFailures`. The scheduler therefore continues at its base interval instead of applying exponential backoff while those jobs repeatedly fail.
**Knowledge Base Used:** [Server HTTP App](https://app.greptile.com/paperclip-org-3/-/custom-context/knowledge-base/paperclipai/paperclip/-/docs/server-http-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| import { createErrorThrottler } from "../lib/error-throttler.js"; | ||
| import type { Logger } from "pino"; | ||
|
|
||
| describe("heartbeat scheduler throttling & backoff", () => { |
There was a problem hiding this comment.
PR description omits required sections
The PR description does not include the required Thinking Path, Verification, Risks, or Model Used sections. Please add the complete template information so maintainers can evaluate how the scheduler change was verified and what operational risks remain.
Context Used: CONTRIBUTING.md has a guide for a good PR message ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/__tests__/heartbeat-scheduler-throttling.test.ts
Line: 5
Comment:
**PR description omits required sections**
The PR description does not include the required Thinking Path, Verification, Risks, or Model Used sections. Please add the complete template information so maintainers can evaluate how the scheduler change was verified and what operational risks remain.
**Context Used:** CONTRIBUTING.md has a guide for a good PR message ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ken scheduler tests - summarizeError: only treat null/undefined as "no error"; previously any falsy thrown value (0, "", false) was misreported as "Unknown error". - ErrorThrottler: bound tracked state to maxTrackedKeys (default 500) with least-recently-active eviction, so distinct error messages (e.g. varying row ids/query params) can't grow the internal Map unboundedly. - Extract the exponential backoff formula into a shared, tested computeBackoffDelayMs() used by both the heartbeat scheduler and its tests, replacing a test that reimplemented (and could silently drift from) the production formula instead of exercising it. - index.ts: drop the now-dead clearInterval call and the setInterval-typed variable/'as any' cast left over from the setInterval -> setTimeout change; add a comment documenting why backoff is driven by errors that escape the tick uncaught rather than every throttled sub-task failure. - Fix two pre-existing broken tests in server-startup-feedback-export.test.ts that still spied on setInterval to capture the heartbeat scheduler's tick callback; the scheduler now uses setTimeout, so the spies never fired and the assertions were silently vacuous. Updated them to spy on setTimeout, matched by the configured base interval so they aren't tripped up by unrelated timers during startup. - Add tests for the above: falsy-error summarization, bounded/evicting throttler state, computeBackoffDelayMs behavior (including a negative- input and custom-step-cap case), and a full outage/recovery simulation proving the backoff grows, throttling suppresses repeats, and both reset together once a tick succeeds again.
Fixes #10911
Summary
When PostgreSQL becomes unreachable or fails while Paperclip server is running, the background heartbeat scheduler ticks (
heartbeat.tickTimers,routines.tickScheduledTriggers, sweepers) fail continuously on every interval tick and log without throttling. This resulted in writing gigabytes of logs per minute (e.g. 5.35 GB in 130 seconds).Fix
ErrorThrottlerto deduplicate and rate-limit repeated identical background error messages within a 5-minute suppression window. The first occurrence logs full details, while subsequent identical errors are suppressed and emitted as a periodic summary line with the count of suppressed occurrences. Truncates long SQL strings to keep individual log lines compact.server/src/__tests__/error-throttler.test.tsandserver/src/__tests__/heartbeat-scheduler-throttling.test.ts.