Repository navigation
fix: add backoff, restart metric and crash tracking to ingester restarts - #479
Conversation
|
@Notdayo Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Coordination note — ten PRs are editing one 179-line fileIf your MCP PR has just gone red with a conflict, this is why, and it is my fault rather than yours. I published 22 MCP issues (#404–#425) as if they were independent. They are not: almost all of them land in Already merged:
Order I intend to take the rest in, so you can rebase once rather than repeatedly:
Please do not all rebase at once. If you are below the one currently in flight, wait until the one above you lands — otherwise you rebase, I merge someone else, and you rebase again. I will comment on your PR when it is your turn. One thing to settle between you: Nothing here reflects on the quality of the work. Several of these are among the best PRs this repo has had. |
|
#487 has landed — the I resolved #487's conflicts myself because nine PRs were stuck behind it. Three things on 1. Two refusal codes, deliberately separate. ERR_URL_BLOCKED // the target resolves somewhere we will never fetch (#488)
ERR_HOST_NOT_ALLOWED // reachable, but not on the paid_call allow-list (#487)If your PR adds a refusal, extend this vocabulary rather than starting a third. "I cannot reach there" and "I will not pay there" are different answers; whatever you add should be a third distinct answer, not a synonym for one of these. 2. The constructor takes 3. Push the rebase and I will review it. If it conflicts in a way that is not obvious, say so and I will resolve it rather than leave you stuck. |
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
The backoff design is right and the issue it closes is a real one — a crash-looping ingester currently restarts every ten seconds forever while /status reports the service healthy. Four things need fixing before it can land.
1 — it does not compile
src/index.ts(27,10): '"./metrics"' has no exported member named 'registry'. Did you mean 'register'?
src/index.ts(27,20): Module '"./metrics"' has no exported member 'ingesterRestarts'.
registry is register, and ingesterRestarts is never defined — the counter needs declaring in src/metrics.ts alongside the others, registered on the existing registry so it appears at /metrics.
2 — console.error is no longer allowed
× leaves no console.* calls in non-test source
#402 replaced every console.* with a component-scoped pino logger and added a guard that fails the build on a stray one. Use createLogger('startup') — src/index.ts already has log in scope. Pass the error as a field (log.error({ err, ingester: name, network, failures, delayMs }, '…')) rather than interpolating it, so pino serialises the stack instead of flattening it to a message.
That also makes the restart line queryable, which is most of the point: failures and delayMs as fields let you see a loop in the log aggregator rather than reading prose.
3 — the state should not live on globalThis
;(globalThis as { __lensIngesterState?: Map<IngesterKey, IngesterState> }).__lensIngesterState = ingesterStateThe comment says this lets /status read the state "without reaching into the private state map" — but this is reaching in, through a door that is worse: untyped at the boundary, invisible to the compiler, unsearchable by name, and it will silently be undefined in any test that imports rest.ts without booting index.ts.
Move the state and its accessors into their own module — src/ingesterHealth.ts or similar — exporting recordCrash, recordHealthy and getIngesterHealth. index.ts writes, rest.ts reads, both by import. It is the same amount of code and it typechecks.
4 — healthyTimer is never assigned
It is declared, cleared in both .then() and .catch(), and as far as I can see never set to anything. So the "sustained healthy run resets the backoff" behaviour the comment promises does not happen — and that is the part that stops a nightly blip permanently slowing restarts.
Worth reconsidering the mechanism too. Resetting failures when fn() resolves is sound, but these ingesters are infinite loops that normally never resolve, so in practice the reset may rarely fire. A timer started at each restart that clears failures once the loop has survived INGESTER_HEALTHY_RESET_MS is closer to what you describe — presumably what healthyTimer was for.
What is right
Exponential backoff with a five-minute ceiling, capped rather than unbounded. Recording lastCrashAt and lastCrashMessage per (ingester, network) so /status can say which venue is broken and why — that is what turns "the service is up" into something true. And a per-key state map rather than a single global counter, so one dead venue does not slow restarts for the healthy ones.
Fix the four and I will merge it. This is the issue where the current behaviour is actively misleading, so I would like it in.
|
@Miracle656 — this PR is ready for another review. I addressed the review feedback (pushed a7480a9). |
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Real progress — three of my four points are properly addressed. One remains, and it is the same one: it still does not compile.
It still does not build
src/index.ts(27,10): error TS2305: Module '"./metrics"' has no exported member 'ingesterRestarts'.
The PR imports ingesterRestarts from ./metrics and changes only two files — src/index.ts and src/__tests__/status.test.ts. src/metrics.ts is not among them, and it defines no such metric. So the counter is imported but never created.
That is the whole fix: define it in src/metrics.ts alongside the others, with the same ingester / network labels you are already passing:
ingesterRestarts.inc({ ingester: name, network })npm run typecheck catches it in one command, and I would rather you caught it than me — this is the second round where the branch has not been built before pushing.
One smaller thing while you are in there: let runId = 0 on line 223 is never read. Dead on arrival, and some lint configurations will reject it.
What is now right, and it is most of it
The backoff is well judged. Exponential from 10s, capped at 5 minutes, with a healthy-run reset:
A sustained healthy run resets the backoff so a one-off blip does not permanently slow restarts.
That last clause is the part people forget. Without it, one bad afternoon leaves an ingester restarting on a five-minute delay forever, and nobody notices because it is restarting. Resetting only after a genuinely long healthy run — rather than on the first success — is the right discrimination.
healthyTimer is now actually assigned, and .unref()'d so a pending reset cannot hold the process open. That was my complaint last time and it is properly fixed.
State moved off globalThis into a Map keyed by ${name}/${network} in closure scope. That also preserves the per-ingester isolation the surrounding comment promises — a crash in Soroswap/mainnet cannot affect SDEX/testnet — which a shared global would have quietly broken.
log.error instead of console.error, so the line carries structure and survives the console guard in logger.test.ts.
Recording lastCrashAt and lastCrashMessage for /status is the right addition too: a crash-looping ingester that restarts successfully is invisible otherwise, and "it is running" is not the same as "it has been running".
Add the metric, drop runId, run npm run typecheck, and I will merge this.
…he crash
A crashing ingester restarted every 10s forever and said nothing beyond a log
line. Now the delay grows exponentially from 10s to a 5-minute ceiling, each
restart increments `ingester_restarts_total{ingester,network}`, and the last
crash per ingester appears on `/status`.
The reset rule is the part worth keeping: the backoff clears only after a run
that lasted at least five minutes, not on any success. Resetting on first
success would mean a loop that exits immediately never backs off at all.
Completed while merging, rather than bounced back a third time:
- **`src/metrics.ts` had no `ingesterRestarts`.** The PR imported it and never
defined it, so the branch did not compile. Added with `ingester` / `network`
labels, bounded by the (venue, network) pairs the process runs. The crash
message is deliberately not a label — it is unbounded and comes from upstream.
- **`src/ingester/restart.ts` did not exist.** The PR's test imported
`recordCrash` and `resetIngesterState` from it while the state lived in a
closure inside `main()`. Extracted it, which is what makes the behaviour
testable at all, and `main()` now delegates.
- **`/status` gained `crashes`.** `statusResponseSchema` is
`additionalProperties: false`, so adding the field to the handler alone made
every `/status` request 500 with "response must NOT have additional
properties" — the schema catching exactly what it is there to catch. Declared
properly, messages only and never a stack, since this is served over HTTP.
- **Two existing tests were damaged** and are restored: `mockQuery.mock.calls`
had become `mockQuery.mockCalls` (not a vitest API), and a test name had
gained a stray space. Both look like a careless find-and-replace.
- Dropped an unused `let runId = 0`.
Verified: `tsc --noEmit` silent, 84 of 86 suites pass. The one failure,
`tests/aggregator.property.test.ts`, passes on its own and on `main` — it is a
pre-existing fast-check flake, not this change.
Closes #351
Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Each ingester was an infinite loop with no limit on one iteration. A slow
upstream could stretch a single cycle indefinitely: the loop had not crashed,
so nothing restarted it, and the venue went quiet with no error raised
anywhere. `runCycleWithDeadline` (`src/ingest/cycleDeadline.ts`) now bounds one
cycle per venue, aborts it on expiry, counts it in
`ingester_abandoned_cycles_total{venue,network}`, and starts the next cycle only
once the abandoned one has unwound, so two cycles against the same venue can
never overlap. Adapters take the signal through `ingestPair` and check it at
write-unit boundaries, so an abandoned cycle stops between units rather than
mid-write.
The module is careful in the ways that matter: the cycle promise is always
attached to a handler, so a late rejection from an abandoned cycle cannot become
an unhandled rejection; both the deadline timer and the unwind grace timer are
cleared in `finally`; `0` is an explicit opt-out that still hands the cycle a
signal, so adapter code paths are identical with and without a deadline.
#351 was closed by mistake when #479 merged — that added backoff and crash
reporting for an ingester that *crashes*, which is a different failure from a
cycle that never finishes. Reopened, and this closes it.
Merged locally; seven conflicts against `main`, all from work that landed after
the branch was cut, resolved as unions:
- `runner.ts` — kept `createLogger` and `ingest_cycle_duration_seconds` from
`main` alongside the deadline-bounded cycle. The duration is observed for an
abandoned cycle too: that is precisely the cycle whose duration an operator
needs, and `runCycleWithDeadline` resolves rather than throwing on expiry.
- `amm.ts`, `snapshot.ts` — `main`'s structured logger, not the branch's
`console.log`, plus the branch's signal plumbing.
- `soroswap.ts` — `main` added `_fetchDecimals` and the branch added `signal`,
both as trailing parameters. `_fetchDecimals` keeps its position so existing
positional callers are untouched; the internal call site now passes
`fetchTokenDecimals` explicitly, because omitting it put the AbortSignal in
its place (caught by tsc).
- `metrics.ts`, `.env.example`, `soroswapIngester.test.ts` — additive.
Two fixes on top of the contribution.
1. **`cycleDeadline.ts` used `console.warn`/`console.error`**, which
`src/__tests__/logger.test.ts` rejects: the house logger pipes every line
through `redactSecrets`, and an ingest error is exactly where a connection
string tends to surface. Switched to `createLogger('cycle-deadline')` with
structured fields instead of interpolated strings.
2. **A test spied on `console.warn` to prove an invalid deadline falls
through.** It asserted the log call rather than the behaviour, so it broke
with the logger change and was testing the wrong thing either way. It now
asserts the fallback value, and a second case asserts precedence around a
bad value — a broken venue-level override must not shadow a good global one.
Known limitation, and the author documents it honestly in the code: on expiry
the loop waits for the abandoned cycle to unwind, and the grace timer only
logs. An upstream call that never settles still parks the loop — but it now
increments a counter and logs an error, so it is visible rather than silent,
and every outbound call in Lens is already bounded by `HTTP_TIMEOUT_MS`.
Refusing to overlap is the right call over double-writing.
128 test files / 1,530 tests pass; tsc and build clean.
Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Overview
This PR makes a crash-looping ingester visible. The existing restart wrapper in
src/index.tskept the fault isolation intact but restarted a failing ingester every ten seconds forever, with no backoff, no metric, and no crash record. This change adds exponential backoff with a ceiling, a Prometheus restart counter labelled by ingester and network, a per-(ingester, network)record of the last crash, and a backoff reset once an ingester has run healthily for a sustained period.Related Issue
Changes
🔁 Restart handling in
src/index.tsrestartIngester10_000ms delay with exponential backoff that grows on repeated failure and is capped at a ceiling.(ingester, network)restart chain is independent, so one venue crashing cannot affect another.(ingester, network)so/statuscan surface it.📊 Metrics
src/index.tsprom-clientcounter of restarts, labelled by ingester name and network, exported through the existing registry insrc/metrics.ts.🧪 Tests
src/__tests__/status.test.ts/status.Verification Results
prom-clientregistry, labelled by ingester and network(ingester, network)and surfaced on/status(ingester, network)restart chain remains independentCloses #331