refactor(sse): split chatCore.ts pure helpers into chatCore/ modules (−561 LOC) - #4159
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refactors chatCore.ts by extracting several helper functions into dedicated modules under open-sse/handlers/chatCore/ and adding corresponding unit tests under tests/unit/. The review feedback identifies a potential memory leak in upstreamTimeouts.ts where an anonymous event listener registered on signal inside abortPromise is not cleaned up after the promise race completes, and provides a suggestion to remove it in the finally block.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const abortPromise = new Promise<never>((_, reject) => { | ||
| signal.addEventListener("abort", () => reject(createAbortError(signal)), { once: true }); | ||
| }); | ||
|
|
||
| try { | ||
| return await Promise.race([execute(combinedController.signal), timeoutPromise, abortPromise]); | ||
| } finally { | ||
| if (timeoutId) clearTimeout(timeoutId); | ||
| if (abortListener) signal.removeEventListener("abort", abortListener); | ||
| if (timeoutAbortListener) { | ||
| timeoutController.signal.removeEventListener("abort", timeoutAbortListener); | ||
| } | ||
| } |
There was a problem hiding this comment.
The anonymous event listener registered on signal inside abortPromise is never removed when the promise race completes. If signal is a long-lived object or if many requests share/re-use signals, this will cause a memory leak.
To prevent this, store a reference to the listener function and remove it in the finally block, similar to how abortListener and timeoutAbortListener are cleaned up.
let abortPromiseListener: (() => void) | null = null;
const abortPromise = new Promise<never>((_, reject) => {
abortPromiseListener = () => reject(createAbortError(signal));
signal.addEventListener("abort", abortPromiseListener, { once: true });
});
try {
return await Promise.race([execute(combinedController.signal), timeoutPromise, abortPromise]);
} finally {
if (timeoutId) clearTimeout(timeoutId);
if (abortListener) signal.removeEventListener("abort", abortListener);
if (abortPromiseListener) signal.removeEventListener("abort", abortPromiseListener);
if (timeoutAbortListener) {
timeoutController.signal.removeEventListener("abort", timeoutAbortListener);
}
}…(−561 LOC) Extracts the pure, side-effect-free helpers out of the chatCore.ts god-file into focused open-sse/handlers/chatCore/ modules (headers, logTruncation, memoryExtraction, nonStreamingSse, passthroughToolNames, upstreamTimeouts), each with its own unit test. Behavior-preserving — chatCore.ts re-imports them. Re-synced cleanly onto release/v3.8.29: the prior branch tip carried a stale-merge regression that re-bloated open-sse/config/freeModelCatalog.data.ts (462 -> 4530 lines) and reverted the free-tier refresh + provider/docs changes. This commit keeps ONLY the chatCore split (zero contamination).
15f591f to
c2e6b61
Compare
…(−561 LOC) (diegosouzapw#4159) Integrated into release/v3.8.29 (re-synced: kept only the chatCore split, dropped the stale-merge contamination)
Objetivo
Encolhimento estrutural do maior god-file de hot-path do
open-sse/:chatCore.ts5980 → 5419 LOC (−561), extraindo 6 grupos coesos de helpers internos puros para módulos focados emopen-sse/handlers/chatCore/, continuando o padrão de modularização #3598/#3821 (que já criouidempotency.ts/sanitization.ts/semanticCache.ts/memorySkillsInjection.ts).Zero mudança de comportamento — todos os corpos foram movidos verbatim; as únicas edições são
export, ajuste de path de import, remoção das defs + import-back, e 1 comentário.Módulos novos (cada um com teste de seam dedicado)
chatCore/logTruncation.tschatcore-log-truncation.test.tschatCore/memoryExtraction.tschatcore-memory-extraction.test.tschatCore/passthroughToolNames.tschatcore-passthrough-tool-names.test.tschatCore/headers.tschatcore-headers.test.tschatCore/nonStreamingSse.tschatcore-non-streaming-sse.test.tschatCore/upstreamTimeouts.tschatcore-upstream-timeouts.test.tsProtocolo lossless (por commit)
Move-not-copy +
git diff= só relocação + grep de símbolos + typecheck + lint +check:cycles+ file-size encolhe (ratcheado p/ baixo). Nenhum módulo importa de volta dechatCore.ts(sem ciclo).Verificação
typecheck:corelimpo ·lint0 errors ·check:cyclesOK ·file-sizesem--updatePASSA (chatCore não cresceu, baseline ratcheado 5980→5419)test:unit→ℹ fail 0(1 cancel dequota-spend-recorder= race do--test-force-exitsob carga, arquivo não-relacionado, passa isolado 5/5)test:vitest→ 187/187Nota sobre
#3501Vários comentários do
file-size-baseline.jsonadiavam "structural shrink of chatCore.ts tracked in #3501" — mas #3501 era o épico doproviders/[id]/page.tsx(ProviderDetailPageClient.tsx), concluído (META ≤800, fase 1t) e fechado; nunca cobriu o chatCore. Este PR é esse shrink adiado; um comentário novo no baseline registra isso (sem reescrever os históricos).Escopo / próximos
Fora deste PR (sub-projetos futuros): decompor a orquestração do
handleChatCore; split docombo.ts; curadoria da listamutatedo Stryker.