feat(matches): backoff and circuit breaker for the external result feed - #1790
Open
bilhokista wants to merge 4 commits into
Open
feat(matches): backoff and circuit breaker for the external result feed#1790bilhokista wants to merge 4 commits into
bilhokista wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1620.
What this changes
HttpExternalResultFeedClient.fetchResults()was a barefirstValueFrom(httpService.get(...))with no error handling: a single upstream blip failed a poll, and a sustained outage meant every scheduled poll kept hammering a service that was already down.Backoff reuses the existing
withRetry/computeBackoffDelayinbackend/src/common/retry.util.tsrather than introducing a second backoff implementation, so the feed behaves likesoroban.service.tsandnotifications/email.service.tsalready do. The issue describes the client as having "no backoff", which is accurate — but the helper it needs was already in the codebase and simply not wired in.Circuit breaker is new:
backend/src/common/circuit-breaker.ts, placed alongsideretry.util.ts. Retry handles one bad call; the breaker handles a bad upstream. It is deliberately small — one upstream, one instance, constructed by the client that owns the call — rather than a decorator or a module.closed→ calls pass through, consecutive failures countedopen→ calls rejected locally withCircuitOpenErrorforopenMshalf-open→ one probe admitted; success closes, failure re-opens for a full windowRetry sits inside the breaker, so one poll gets one retry budget and the breaker counts polls rather than individual attempts.
Error classification (
isTransientFeedError): 408/425/429/5xx and requests that never got a response are retried; other 4xx are not, because a wrong URL or a rejected credential will be rejected identically on every attempt.Health signal is exposed as
client.getHealth(): BreakerHealth—{ state, consecutiveFailures, retryAfterMs, lastError }.One scope decision worth flagging
The issue lists
backend/src/healthin its file list. I did not wire the feed intoHealthService: that service composes injected Terminus indicators, and adding the feed changes which dependencies/healthreports on — an architecture call for a maintainer, not a side effect of this PR. The signal is exposed and ready to consume; I'm happy to add the indicator in this PR if you'd like it here, and there is a comment in the client saying so.Tests
backend/src/common/circuit-breaker.spec.ts(new, 6 cases) drives the breaker through an injected clock, so no test sleeps through a realopenMs:external-result-feed.client.spec.tsadds status-classification cases, "retries a transient failure", "does not retry a rejected credential", "opens after repeated failures and stops calling the feed", and "closes again when the half-open probe succeeds". The existing URL/credential test is unchanged and still passes.Honest note on verification: I could not run the backend Jest suite locally (no full workspace install). I did two things instead: every changed file was parsed with the TypeScript compiler API, and the breaker's state machine was transpiled and executed standalone against the same 14 assertions the spec makes — all passed. CI is the real check on the Nest-level tests.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CrfEY1tvXrbeMDAUzxfuk7