Skip to content

perf(server): skip periodic checks for disabled providers - #18851

Merged
juliusmarminge merged 6 commits into
pingdotgg:mainfrom
saphid:t3code/sweep/disabled-provider-health-skip
Oct 20, 2026
Merged

juliusmarminge merged 6 commits into
pingdotgg:mainfrom
saphid:t3code/sweep/disabled-provider-health-skip

Conversation

@saphid

@saphid saphid commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Every provider instance runs a periodic health check while any client wants provider status, and disabled instances were included. With one provider enabled and the rest disabled, each cycle re-probed every disabled instance and pushed a provider-status catalog that differed only in checkedAt. The original measurement on an idle web client was a 37,360-byte disabled-only providerStatuses push every 300 s.

Change

hasProviderStatusDemand in packages/provider-core/src/server/managedProvider.ts returns false for a disabled snapshot, so the interval loop skips the probe and the publication. That function is only consulted by the interval loop. Manual refresh of a disabled provider still probes, enabling a provider still checks it immediately, and periodic checks resume once the snapshot is enabled. Provider configuration changes rebuild the managed instance, so the gate always reads the current enabled state.

Tests

The original two-case parametrized scenario (generic vs. instance demand) collapsed into one test in managedProvider.test.ts, "skips periodic checks while disabled and resumes them once enabled". The two demand shapes exercised the same early return. The test drives the real publication stream with TestClock: two 5-minute intervals while disabled produce no probe and no publication, manual refresh probes, enabling through settings probes immediately, and the next interval probes again. It fails on main (3 probes instead of 1) and passes here. The file's test count went from 12 on main to 13.

Verification

Headless Linux, Node 24.

  • vp test run src/server/managedProvider.test.ts in packages/provider-core: 13 passed.
  • Targeted vp lint, vp fmt --check, and provider-core typecheck: clean.
  • Live headless Linux web/server with isolated state and a two-second health interval: eight disabled providers produced four unique provider-status frames (44,696 bytes) over eight seconds on main, versus zero on this head. WebSocket pings continued during capture. Manual refresh while disabled still produced one fresh status event. Enabling a Pi RPC fixture in Settings discovered its model and resumed periodic status timestamps; disabling it again returned to zero frames in a settled eight-second window.
  • Sanitized captures: main, this head, including manual refresh and enabling. Frame counts deduplicate identical messages. These are fixture measurements, not a production bandwidth estimate.
  • Settings enable/disable recording.
  • Not verified on macOS, Windows, iOS, Android, or Electron desktop. Both reviewers traced the web and client-runtime consumers of checkedAt and found nothing that relies on periodic updates for disabled providers.

Headless Linux Settings after disabling the Pi fixture

Independent read-only reviews by Claude Opus 5.5 and GPT-6-Sol found no blockers.

Takes over #18851 by @saphid, whose measurement and gate this keeps. Implementation: Claude Fable 5.1 through Claude Code in T3 Code.

Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 13 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 72bb4cbb-25e2-488d-885a-53de4aa49ef1
📥 Commits

Reviewing files that changed from the base of the PR and between 5d195a6 and 9b0700b.

📒 Files selected for processing: 2
  • packages/provider-core/src/server/managedProvider.test.ts
  • packages/provider-core/src/server/managedProvider.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fd695207-838d-49b4-b748-6bbe4d9d52d0
📥 Commits

Reviewing files that changed from the base of the PR and between 9077e81 and 5d195a6.

📒 Files selected for processing: 2
  • packages/provider-core/src/server/managedProvider.test.ts
  • packages/provider-core/src/server/managedProvider.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

📝 Walkthrough

When the provider snapshot is disabled, hasProviderStatusDemand returns false without checking background-work demand. A test covers periodic checks while disabled, manual refresh, and periodic checks after settings enable the provider.

Changes: Provider Check Scheduling

Layer / File(s) Summary
Disabled-provider demand and periodic checks
packages/provider-core/src/server/managedProvider.ts, packages/provider-core/src/server/managedProvider.test.ts
Disabled snapshots skip background-work demand checks. The test checks that periodic checks remain skipped while disabled, manual refresh runs a check, and enabling the provider restores periodic checks.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 5d195

No issue requiring a fix before merging is established.

Pre-merge checks: Passed 4
Check name Status Explanation
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.
Title check Passed The title clearly and concisely describes the main change: skipping periodic server checks for disabled providers. It also follows the repository's conventional commit format.
Description check Passed The description is detailed and covers the problem, implementation, tests, verification results, limitations, issue context, and review information. It does not use a separate “Scope and approval” hea…
Full details: Description check

Explanation

The description is detailed and covers the problem, implementation, tests, verification results, limitations, issue context, and review information. It does not use a separate “Scope and approval” heading, but it references the prior PR and explains the focused scope, so the required context is substantially present.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 12, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR changes production behavior by suppressing periodic provider status checks for disabled providers, while preserving manual and settings-triggered refreshes. The change is localized and tested, but it gates recurring runtime work based on provider state and merits human review.

You can add or adjust custom eligibility rules. Learn more.

AdEx-Partners-DE added a commit to AdEx-Partners-DE/t3code that referenced this pull request Oct 13, 2026
…tream pingdotgg#18851)

Own diff of pingdotgg#18851 at 4c27e20, applied on stark/main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one. I'll trim the tests to the useful coverage, rebuild the implementation against current main, run independent reviews, and verify it before pushing to this branch.

@juliusmarminge
juliusmarminge merged commit 3496ecb into pingdotgg:main Oct 20, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants