fix(selfhost): improve orb-relay-drain robustness against broker degradation - #3929
fix(selfhost): improve orb-relay-drain robustness against broker degradation#3929sentry[bot] wants to merge 1 commit into
Conversation
|
| method: "POST", | ||
| headers: { authorization: `Bearer ${env.ORB_ENROLLMENT_SECRET}`, "content-type": "application/json" }, | ||
| signal: AbortSignal.timeout(30_000), | ||
| body: JSON.stringify({ ack }), |
There was a problem hiding this comment.
Authorization header removed from broker API request, leaving orb-relay-drain unauthenticated
Removed authorization header from broker API fetch call, leaving request unauthenticated.
Restore the headers with the Bearer token and update only the timeout value.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/orb/broker-client.ts">
<violation number="1" location="src/orb/broker-client.ts:252">
<priority>critical</priority>
<title>Authorization header removed from broker API request, leaving orb-relay-drain unauthenticated</title>
<evidence>The `headers` property containing `authorization: Bearer ${env.ORB_ENROLLMENT_SECRET}` was replaced with `signal: AbortSignal.timeout(30_000)`. The fetch call now sends no authentication token to the broker API, and the duplicate `signal` property means the second timeout (15_000) takes precedence.</evidence>
<recommendation>Restore the original `headers` line with the authorization bearer token and content-type. If a signal change is still needed, update only the existing `signal: AbortSignal.timeout(15_000)` value rather than replacing the headers.</recommendation>
</violation>
</file>
| () => | ||
| void drainRelay().catch((error) => | ||
| captureError(error, { kind: "orb_relay_drain" }), | ||
| 30_000, |
There was a problem hiding this comment.
Error monitoring silently disabled in orb-relay-drain interval handler
Replaced captureError with a no-op literal, silently swallowing drain loop errors.
Restore the captureError call to preserve monitoring of drain failures.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/server.ts">
<violation number="2" location="src/server.ts:1033">
<priority>high</priority>
<title>Error monitoring silently disabled in orb-relay-drain interval handler</title>
<evidence>The `captureError(error, { kind: "orb_relay_drain" })` call in the setInterval error handler was replaced with a no-op literal `30_000`. Errors from drain operations will now be silently swallowed instead of being reported to the monitoring system.</evidence>
<recommendation>Restore the `captureError(error, { kind: "orb_relay_drain" })` call to ensure drain failures are visible in monitoring.</recommendation>
</violation>
</file>
| }, | ||
| env, | ||
| drain: drainOrbRelay, | ||
| if (drainInFlight) return; |
There was a problem hiding this comment.
orb-relay-drain loop function definition removed, breaking scheduled drain execution
Removed drainRelay function wrapper, leaving drain logic orphaned and the loop broken.
Restore the drainRelay async function wrapper around the drain logic.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/server.ts">
<violation number="3" location="src/server.ts:1010">
<priority>high</priority>
<title>orb-relay-drain loop function definition removed, breaking scheduled drain execution</title>
<evidence>The `const drainRelay = async (): Promise<void> => {` function definition was removed, but its body content (`if (drainInFlight)`, `try/finally`, `await drainOrbRelayWithMonitor`) was left in place without a function wrapper. The subsequent `void drainRelay()` call references an undefined function, and the setInterval callback also references it. The drain loop is completely broken.</evidence>
<recommendation>Restore the `const drainRelay = async (): Promise<void> => {` wrapper around the drain logic, and place the new in-flight guard inside it.</recommendation>
</violation>
</file>
|
Closing in favor of #3984 — the diff here got corrupted during patch application: it dropped the The underlying root cause this PR identified (15s timeout == 15s poll interval, no in-flight guard) is real and is fixed correctly in #3984, with the Authorization header preserved. |
Summary
This PR addresses
TimeoutError: The operation was aborted due to timeoutevents in theorb-relay-drainscheduled job, which were exacerbated by broker degradation. The solution involves increasing theAbortSignal.timeout, adding an in-flight guard, and adjusting thesetIntervalfrequency to make the drain process more resilient.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.UI Evidence
Required for visible UI, frontend, docs, or extension changes. Attach GitHub-hosted JPG/JPEG or PNG screenshots here; SVG screenshots are not accepted as review evidence. Use a compact table/grid of clickable thumbnails with a short state/title such as "Loaded state", "Empty state", "Error state", "Mobile layout", or "PR sidebar". Prefer annotated screenshots with a colored box, outline, arrow, or highlighter showing what changed. Recordings can be supplemental, but screenshots are still expected for visual review. Do not commit review-only screenshots, recordings, or
docs/review-evidence/**files.<a href="FULL_URL.png"><img src="FULL_URL.png" alt="Loaded state" width="240"></a>Notes
This PR addresses
TimeoutError: The operation was aborted due to timeoutevents occurring in theorb-relay-drainscheduled job, which were exacerbated by broker degradation.Root Cause:
The
drainOrbRelayfunction had anAbortSignal.timeoutof 15 seconds, which was identical to thesetIntervalfrequency of the drain loop. When the Orb Broker API experienced degradation (e.g., returning HTTP 500s or hanging), this led to two main issues:drainOrbRelaycalls: EachsetIntervaltick would initiate a new drain attempt before the previous one could complete or timeout, leading to a pile-up of requests.TimeoutErrorwithout any buffer.Solution:
To make the
orb-relay-drainmore resilient to broker degradation, the following changes were implemented:AbortSignal.timeout: Insrc/orb/broker-client.ts, theAbortSignal.timeoutfor the/v1/orb/relay/pullrequest was increased from15_000ms to30_000ms. This provides a longer window for the broker to respond before the request is aborted.src/server.ts, adrainInFlightboolean flag was introduced around thesetIntervalcallback. If a drain operation is already in progress, subsequent ticks will be skipped, preventing concurrent calls from piling up during slow or unresponsive periods.setIntervalfrequency: ThesetIntervalfor the drain loop insrc/server.tswas also increased from15_000ms to30_000ms. This ensures the poll period matches the new HTTP timeout, preventing new drain attempts from starting before the previous one has had a chance to complete or timeout, even without the in-flight guard.Fixes GITTENSORY-1C