Repository navigation
fix: bound generation history and flush per turn to stop Stop-hook OOM on long sessions - #122
Conversation
Every generation serialized the full session history (earlier turns plus earlier steps of the current turn) onto its span, so memory grew quadratically with session length and long sessions crashed the Stop hook with a V8 heap OOM or an oversized export request. Keep only the most recent messages that fit in max_history_chars (default 200000, 0 disables) and replace the rest with a single omitted-messages marker. Messages are never cut, and tool spans keep their full output. Co-authored-by: Hassieb Pakzad <hassiebp@users.noreply.github.com>
The Stop hook converted every pending turn, flushed once, and only then marked turns in the sidecar. A backlog therefore sat in memory all at once and was delivered all-or-nothing: any crash, timeout or failed export left every turn unmarked, so the next hook retried an even larger backlog. Flush after each turn and mark it as soon as its export succeeds, so a backlog drains incrementally and only one turn's spans are held at a time. Co-authored-by: Hassieb Pakzad <hassiebp@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4381a3fdd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| current: readonly ChatMlMessage[], | ||
| maxChars: number, | ||
| ): ChatMlMessage[] { | ||
| const all = [...earlier, ...current]; |
There was a problem hiding this comment.
For sessions with many turns or tool calls, max_history_chars does not bound the memory used here because [...earlier, ...current] first copies every historical message, and convertRollout has already retained a separate cumulative historyPrefixes array for every turn. The number of retained references remains quadratic in session length, and each generation temporarily allocates another full-history array before discarding old entries, so sufficiently long sessions can still hit the OOM this change is intended to prevent. Walk the two inputs backward or maintain a bounded rolling history without materializing all prior messages.
Useful? React with 👍 / 👎.
|
|
||
| uploaded.add(turn.turnId); | ||
| exportedTurnIds.push(turn.turnId); | ||
| await options.onTurnExported?.(turn.turnId); |
There was a problem hiding this comment.
When one parent turn contains a long subagent rollout, this callback is reached only after emitTurn finishes recursively converting every child turn; recursive convertRollout calls use parentObservation, whose branch never invokes onTurnExported. Consequently all nested spans remain buffered together, so a single parent turn with enough subagent activity can still exhaust memory or exceed the exporter body limit. Nested completed turns need a flush path distinct from the outer-turn sidecar marking callback.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| while (start > 0) { | ||
| const chars = serializedChars(all[start - 1]); | ||
| if (used + chars > maxChars) break; | ||
| used += chars; | ||
| start--; | ||
| } | ||
| if (start === 0) return all; |
There was a problem hiding this comment.
🔴 Users with one oversized message lose the whole conversation from generation inputs, including the current turn's own user prompt. Only an omission marker is left. In boundedConversation, the loop breaks at the first message that does not fit, so one message over max_history_chars (default 200000) drops every older message too. Fix: always keep the current turn's user message and the newest message even when over budget, and skip an oversized message instead of stopping, so smaller recent messages still fit.
Why this was flagged
The trigger is a Codex turn whose user message, or whose latest tool output, serializes to more than max_history_chars (200000 by default). The PR's own 500 KB tool-output workload is this case. boundedConversation (trace.ts:396-402) walks backward and breaks as soon as used + chars > maxChars. If the newest message is the oversized one, start stays at all.length. generationInput then gets only the [N earlier message(s) omitted ...] system marker, with no user prompt and no preceding assistant or tool messages. Every later generation in that turn has the same problem, because the oversized message stays the newest-but-growing tail. On the base branch, each LLM generation span showed the full conversation, including the turn's user message. Nothing else keeps the current user message in the generation input: turnMessages (trace.ts:525) only feeds boundedConversation.
Verification: The trigger is any single message whose JSON.stringify length is over max_history_chars (default 200000). In boundedConversation (trace.ts:396-402), the walk stops at the first message that does not fit. When that is the newest, the returned array is only the omission marker. The base generationInput always pushed userMessage(turn) and the prior steps.
| while (start > 0) { | ||
| const chars = serializedChars(all[start - 1]); | ||
| if (used + chars > maxChars) break; | ||
| used += chars; | ||
| start--; | ||
| } | ||
| if (start === 0) return all; | ||
| return [ | ||
| { | ||
| role: "system", | ||
| content: `[${start} earlier message(s) omitted: the conversation exceeds max_history_chars (${maxChars})]`, | ||
| }, | ||
| ...all.slice(start), |
There was a problem hiding this comment.
🟡 (optional) Trimmed generation inputs can start with tool results whose assistant tool call was dropped, so the stored conversation is not valid ChatML. In boundedConversation, the newest-first loop stops at any message boundary. When the cut falls between an assistant tool_calls message and its role: "tool" replies, those replies stay but their call is replaced by the omitted marker. Fix: after the loop, advance start past leading tool messages so a call and its results are kept or dropped together, and count the skipped ones in the marker's N.
Why this was flagged
The trigger is any Codex turn whose history goes over max_history_chars (default 200000) where a step has tool calls, which is the normal shape of a long session. The loop at trace.ts:396-401 keeps the longest suffix of whole messages that fits. A single large assistantMessage carrying tool_calls, or a budget boundary that lands inside one step's toolMessages run, leaves all[start] as a role: "tool" message whose tool_call_id belongs to an omitted assistant message. The input stored on the LLM span is then [system prompt, omission marker, tool, tool, ..., assistant, ...]. Playground replay, dataset export or evals that pass this input to an OpenAI-compatible API get the rejection "messages with role 'tool' must be a response to a preceding message with 'tool_calls'". On the base branch, generation inputs always contained every assistant tool_calls message ahead of its results. No other check repairs the sequence, because generationInput and attachToolDefinitions copy the trimmed list as it is.
Verification: Severity: nit. In plugins/tracing/src/trace.ts, boundedConversation walks all from newest to oldest, stops at whatever message no longer fits and never checks roles. Before this change, generationInput always emitted whole assistant-plus-tools groups, so every stored input was a well-formed ChatML sequence.
Closes LFE-17701. Closes #119.
Problem
On long Codex sessions the Stop hook aborts with a V8 heap OOM, and from then on none of that session's turns reach Langfuse.
Every generation's input is the full conversation so far: earlier turns plus earlier steps of the current turn, tool outputs included. Each input is
JSON.stringify'd onto its span, so memory grows roughly quadratically with session length. All pending turns are converted before a single flush at shutdown, and only then are turns marked in the sidecar.That gives three ways to fail, each of which leaves every turn unmarked and the next hook with an even bigger backlog:
abort()that the hook's try/catch can't catch.RangeError: Invalid string length: the batched OTLP request body passes V8's ~512 MB string limit. This happens even without an OOM.forceFlushwaits out the 30 s export timeout, which is over the 30 s Codex hook timeout.Changes
1. Bound the conversation attached to each generation (
max_history_chars, default200000,0disables).[N earlier message(s) omitted …]system marker.langfuse.jsonorLANGFUSE_CODEX_MAX_HISTORY_CHARS. Invalid values fall back to the default without discarding the rest of the config file.2. Flush and mark each turn before converting the next.
convertRollouttakes anonTurnExportedcallback. The hook uses it toflush()and thenmarkTurnUploadedper turn.Measurements
Built hook, synthetic rollouts, local sink that accepts uploads,
--max-old-space-size=1536:Invalid string length, 33 s, nothing uploadedInvalid string length, 36 s, nothing uploadedTests
config.test.ts: the default value, reading from the file and from env, and invalid values being ignored without dropping the file.trace.test.ts: the budget covering earlier turns and current-turn steps, the exact marker text, and0meaning no cap.hook-command.test.ts: the end-to-end hook delivers turn 1, the sink rejects turn 2, and the sidecar contains onlyturn-1.mainand pass here.pnpm lintandpnpm test(125 tests) pass.Linear Issue: LFE-17701