Skip to content

Commit a620e73

Browse files
committed
fix(api): address review on stream-replay hardening
- CRITICAL (gemini): break the scroll loop on an empty hits page. Relying solely on counter === total could loop forever scrolling empty pages (deletes/merges mid-scroll, or a capped total counter never reaches) — pegging CPU and spamming ES. - Track total hits to (cap + 1) on the replay query (codex): ES only tracks 10k by default, so a 60k-doc replay would slip a 50k cap and silently truncate at 10k. Now the cap is enforced accurately and replays stream to their true end. - Return a failure status on client disconnect (copilot): previously returned status:true, so the caller launched up to 3 follow-up 'fill' replays scrolling the cold tier for a dead socket. - Normalize stream_max_concurrent_replays against NaN/negative (copilot) so a bad config can't silently disable the cap.
1 parent c640b83 commit a620e73

1 file changed

Lines changed: 36 additions & 12 deletions

File tree

src/api/helpers/functions.ts

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,10 @@ export async function streamPastCommon<T extends keyof StreamTypeMap>(
168168
// Bound concurrent replays (per API process). A reconnect storm or many simultaneous
169169
// deep-`start_from` subscriptions would otherwise spawn parallel full-history scrolls that
170170
// all pound the oldest (cold-tier) shards at once. Apply backpressure instead.
171-
const maxConcurrent = fastify.manager.config.api.stream_max_concurrent_replays ?? DEFAULT_MAX_CONCURRENT_REPLAYS;
171+
let maxConcurrent = fastify.manager.config.api.stream_max_concurrent_replays ?? DEFAULT_MAX_CONCURRENT_REPLAYS;
172+
if (!Number.isFinite(maxConcurrent) || maxConcurrent < 1) {
173+
maxConcurrent = DEFAULT_MAX_CONCURRENT_REPLAYS;
174+
}
172175
if (activeHistoryReplays >= maxConcurrent) {
173176
const errorMsg = `Server busy: ${activeHistoryReplays} history replays already running (max ${maxConcurrent}). Please retry shortly.`;
174177
hLog(`[${requestUUID}] Rejected ${dataKind} replay — ${errorMsg}`);
@@ -193,27 +196,36 @@ export async function streamPastCommon<T extends keyof StreamTypeMap>(
193196
let totalFiltered = 0;
194197
let longScroll = false;
195198

199+
// Cap a single replay so it can't scroll the entire history. When unset, fall back to a
200+
// finite default; an explicit -1 opts into unbounded replay but is logged loudly so it
201+
// can't silently melt the cold tier.
202+
let scrollLimit = fastify.manager.config.api.stream_scroll_limit;
203+
if (scrollLimit === undefined || scrollLimit === null) {
204+
scrollLimit = DEFAULT_STREAM_SCROLL_LIMIT;
205+
}
206+
// ES only tracks total hits up to 10k by default — which would let a 60k-doc replay slip a
207+
// 50k cap and silently truncate at 10k. Track up to (cap + 1) so "over the limit" is
208+
// detectable; for unlimited (-1), track to just past the warn threshold.
209+
const trackTotalHits = scrollLimit === -1
210+
? UNBOUNDED_REPLAY_WARN_THRESHOLD + 1
211+
: scrollLimit + 1;
212+
196213
const esQuery = {
197214
index: fastify.manager.chain + `-${dataKind}-*`,
198215
scroll: `${MAX_SCROLL_TIME_SEC}s`,
199216
size: fastify.manager.config.api.stream_scroll_batch || 500,
200-
...search_body
217+
...search_body,
218+
track_total_hits: trackTotalHits
201219
};
202220

203221
const init_response: estypes.SearchResponse<any, any> = await fastify.elastic.search(esQuery);
204222
currentScrollId = init_response._scroll_id;
205223

206224
const totalHits = getTotalValue(init_response);
207225

208-
// Cap a single replay so it can't scroll the entire history. When unset, fall back to a
209-
// finite default; an explicit -1 opts into unbounded replay but is logged loudly so it
210-
// can't silently melt the cold tier.
211-
let scrollLimit = fastify.manager.config.api.stream_scroll_limit;
212-
if (scrollLimit === undefined || scrollLimit === null) {
213-
scrollLimit = DEFAULT_STREAM_SCROLL_LIMIT;
214-
}
226+
// Reject an over-cap replay up front instead of scrolling the whole cold tier.
215227
if (scrollLimit !== -1 && totalHits > scrollLimit) {
216-
const errorMsg = `Requested ${totalHits} ${dataKind}s, limit is ${scrollLimit}.`;
228+
const errorMsg = `Requested at least ${totalHits} ${dataKind}s, limit is ${scrollLimit}. Narrow the range with start_from/read_until.`;
217229
socket.emit('message', {
218230
reqUUID: requestUUID,
219231
type: `${dataKind}_trace`,
@@ -224,7 +236,7 @@ export async function streamPastCommon<T extends keyof StreamTypeMap>(
224236
return {status: false, error: errorMsg};
225237
}
226238
if (scrollLimit === -1 && totalHits > UNBOUNDED_REPLAY_WARN_THRESHOLD) {
227-
hLog(`[WARN][${requestUUID}] Unbounded ${dataKind} replay of ${totalHits} docs (api.stream_scroll_limit=-1) — set a finite limit to protect old/cold indices.`);
239+
hLog(`[WARN][${requestUUID}] Unbounded ${dataKind} replay of ${totalHits}+ docs (api.stream_scroll_limit=-1) — set a finite limit to protect old/cold indices.`);
228240
}
229241

230242
if (totalHits > 10000) {
@@ -248,6 +260,15 @@ export async function streamPastCommon<T extends keyof StreamTypeMap>(
248260

249261
if (rp) {
250262

263+
// Empty page = scroll exhausted. This is the authoritative terminator: relying only
264+
// on `counter === total` can loop forever if the scroll ends early (docs deleted/
265+
// merged mid-scroll, or a capped total that counter never exactly reaches) — an
266+
// infinite empty-scroll loop would peg CPU and spam ES.
267+
if (!rp.hits?.hits || rp.hits.hits.length === 0) {
268+
hLog(`${counter} past ${dataKind}s streamed to ${socket.id} (${totalFiltered} filtered)`);
269+
break;
270+
}
271+
251272
currentScrollId = rp._scroll_id;
252273
const enqueuedMessages: any[] = [];
253274
counter += rp.hits.hits.length;
@@ -314,8 +335,11 @@ export async function streamPastCommon<T extends keyof StreamTypeMap>(
314335
}
315336
}
316337
} else {
338+
// Client gone — return a failure status so the caller stops immediately and
339+
// does NOT launch its follow-up "fill" replays (which would scroll the cold
340+
// tier for a socket that no longer exists). finally still clears the scroll.
317341
hLog(`LOST CLIENT During ${dataKind.toUpperCase()} history replay!`);
318-
break;
342+
return {status: false, error: 'client disconnected', lastTransmittedBlock, counter};
319343
}
320344

321345
if (longScroll) {

0 commit comments

Comments
 (0)