Skip to content

emrg: fix double-accumulated reasoning in llm.jsonl (O(n²) blowup) - #943

Merged
argszero merged 2 commits into
masterfrom
feature/fix-reasoning-double-accumulation
Aug 23, 2026
Merged

emrg: fix double-accumulated reasoning in llm.jsonl (O(n²) blowup)#943
argszero merged 2 commits into
masterfrom
feature/fix-reasoning-double-accumulation

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fix double-accumulated reasoning in llm.jsonl (O(n²) blowup)

Problem

emrg/server/daemon.py appended every per-chunk cumulative reasoning snapshot to reasoning_parts and joined them at the end. But llm.py already accumulates reasoning internally and yields the full think text so far on every chunk (chunk["reasoning"] = "".join(reasoning_parts), llm.py:366).

Appending N cumulative snapshots and joining them re-accumulates the text N times — O(n²) growth:

  • 47 real thinking tokens → 29,640 chars in the record (~630×)
  • worst single llm.jsonl record: 9,968,917 chars (~10 MB)
  • ~190 MB of disk across 3 rotations

Fix

Take the last snapshot — it is the complete think text:

full_reasoning=reasoning_parts[-1] ifreasoning_partselseNone

A comment in the code documents the llm.py accumulation contract so this cannot regress silently.

Verification

  • uv run pytest tests/ -q → 1006 passed, 1 skipped
  • from emrg.client.app import run_client + python -m emrg --help OK
  • The llm.py accumulation contract itself is already covered by tests/test_llm.py (test_stream_accumulates_reasoning_content, test_stream_accumulates_openai_reasoning, test_stream_no_reasoning_means_none)

Ref: host rant 2026-08-23T10:15:06 (P0).

…nt 2026-08-23T10:15:06)
daemon.py appended every per-chunk cumulative reasoning snapshot and
joined them, re-accumulating what llm.py already accumulates
(chunk['reasoning'] = full think text so far, llm.py:366). For 47
real thinking tokens this produced a 29640-char record (630x); the
worst llm.jsonl record hit ~10MB with ~190MB across 3 rotations.
Take the LAST snapshot instead: it is the complete think text.
@argszero

Copy link
Copy Markdown
OwnerAuthor

✅ LGTM — cycle 1287. The daemon-side double-accumulation is confirmed against the llm.py contract (chunk['reasoning'] is already the cumulative snapshot, llm.py:366); taking the last snapshot yields the complete think text. Single accumulation site in daemon.py — fix is complete.

@argszero

Copy link
Copy Markdown
OwnerAuthor

✅ LGTM — cycle 1288. Fresh-eyes re-review of the final head (835ebb0, after master merge): the one-line fix correctly takes the last cumulative snapshot (chunk['reasoning'] is the full think text per llm.py:366, so [-1] is complete and O(n) instead of O(n²)). Empty reasoning_parts → None preserves the no-reasoning behavior; all three downstream full_reasoning sites (assistant-message persist + llm.jsonl logs) are unchanged. The conflict resolution also correctly kept #942's cache-pct debug line. CI test + test-windows pass on head; local pytest 1008 passed + 1 skipped.

@argszero

Copy link
Copy Markdown
OwnerAuthor

✅ LGTM — cycle 1289. Third consecutive fresh-eyes review (head 835ebb0, CI test + test-windows green, MERGEABLE). The fix takes the last cumulative reasoning snapshot (chunk['reasoning'] = full think text per llm.py:366), eliminating the O(n²) double-accumulation that inflated a 47-token think to a 29,640-char llm.jsonl record; empty reasoning_parts → None preserves prior behavior. No ❌ since the first LGTM — merge condition (3 consecutive ✅ from different cycles) satisfied.

@argszero
argszero merged commit 6155ab7 into masterAug 23, 2026
2 checks passed
@argszero
argszero deleted the feature/fix-reasoning-double-accumulation branch August 23, 2026 04:28
argszero added a commit that referenced this pull request Aug 23, 2026
Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@argszero