Skip to content

fix(supervisor): hold the last backpressure verdict when a read fails - #4444

Open
nicktrn wants to merge 4 commits into
mainfrom
fix/backpressure-hold-last-verdict
Open

fix(supervisor): hold the last backpressure verdict when a read fails#4444
nicktrn wants to merge 4 commits into
mainfrom
fix/backpressure-hold-last-verdict

Conversation

@nicktrn

@nicktrn nicktrn commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

The dequeue brake released the moment its signal became unreadable. refresh() caught any error from source.read() and set the verdict to null, which computeEngaged() treats as not-engaged — so a few failed reads dropped an engaged brake, silently, with no log and no metric.

That handling was symmetric while the risk is not. A source that has stopped answering correlates with the pressure the brake exists for, so releasing on read failure gives up protection at exactly the wrong moment; holding too long only costs throughput.

Now a failed read keeps the last verdict instead of discarding it. The verdict then ages normally, so the existing maxVerdictAgeMs check becomes the grace window and still bounds how long a dead source can hold the brake — a permanently unreachable source releases it rather than pinning dequeuing forever. Because computeEngaged() only consults staleness for an engaged verdict, a released one is unaffected and stays released.

The default grace moves from 15s to 120s, comparable to how long the brake normally stays engaged.

One guard worth calling out: holding is only safe when something bounds it, so when maxVerdictAgeMs is unset the previous discard behaviour is kept. Otherwise an unbounded hold could pin the brake indefinitely.

Read failures were previously invisible — the catch block neither logged nor counted. Adds a read_failures_total counter, plus an error log on the transition into failure rather than once per tick, since the refresh loop runs every second.

The post-release ramp needs no change: it anchors off the engaged-to-released transition, so a grace-window release still ramps back up instead of snapping to full rate, which is what you want after a blind period.

Tests cover holding while reads fail, releasing past the max age, and the existing unbounded-config paths are unchanged.

@nicktrn nicktrn self-assigned this Jul 31, 2026
@changeset-bot

changeset-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 2cf09ff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 3 potential issues.

Open in Devin Review

Comment thread apps/supervisor/src/backpressure/backpressureMonitor.ts
Comment thread apps/supervisor/src/backpressure/backpressureMonitor.ts Outdated
Comment thread apps/supervisor/src/backpressure/backpressureMonitor.ts
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Backpressure monitoring now treats failed verdict reads as outages. It increments a failure counter, logs one error per outage, and retains the last verdict when maxVerdictAgeMs is configured. The monitor releases stale verdicts after the maximum age and fails open immediately when no maximum age is configured. Explicit null reads release engagement immediately. The default maximum age for Redis verdicts increased from 15 seconds to 120 seconds.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: retaining the last backpressure verdict after a read failure.
Description check ✅ Passed The description clearly explains the behavior change, configuration limits, metrics, logging, and test coverage, although it omits the template headings and checklist.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/backpressure-hold-last-verdict

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

Open in Devin Review

Comment thread .server-changes/backpressure-hold-last-verdict.md Outdated
Comment thread apps/supervisor/src/env.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 46b25d12-819d-4881-a493-b7812e34ee43

📥 Commits

Reviewing files that changed from the base of the PR and between 8c9c92f and 2cf09ff.

📒 Files selected for processing (1)
  • .server-changes/backpressure-hold-last-verdict.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: typecheck / typecheck
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-05-14T14:54:39.095Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3545
File: .server-changes/agent-view-sessions.md:10-10
Timestamp: 2026-05-14T14:54:39.095Z
Learning: In the `trigger.dev` repository, do not flag inconsistent dot vs slash notation in route/path strings inside `.server-changes/*.md` files. These markdown files are consumed verbatim into the changelog, so the mixed notation (e.g., `resources.orgs.../runs.$runParam/...`) is intentional and should be preserved as-is.

Applied to files:

  • .server-changes/backpressure-hold-last-verdict.md
📚 Learning: 2026-07-26T13:14:02.968Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 4378
File: .server-changes/realtime-run-reads-from-primary.md:0-0
Timestamp: 2026-07-26T13:14:02.968Z
Learning: For files in the .server-changes directory, the body text is published verbatim as dashboard-facing user release notes. Write entries in terms of user-visible behavior (what users can do/see), and avoid implementation-oriented details such as environment-variable names, internal mechanisms, or configuration knobs. If you need to include operational/configuration specifics, put those details in the PR description instead of the .server-changes entry.

Applied to files:

  • .server-changes/backpressure-hold-last-verdict.md

Comment thread .server-changes/backpressure-hold-last-verdict.md
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