Skip to content

fix(ModelRungGuard): seek the transcript tail instead of reading the whole file - #1786

Closed
AntSentry wants to merge 1 commit into
danielmiessler:mainfrom
AntSentry:fix/modelrungguard-tail-read
Closed

fix(ModelRungGuard): seek the transcript tail instead of reading the whole file#1786
AntSentry wants to merge 1 commit into
danielmiessler:mainfrom
AntSentry:fix/modelrungguard-tail-read

Conversation

@AntSentry

Copy link
Copy Markdown

liveModel() in ModelRungGuard.hook.ts computes a 256 KB tail offset and then readFileSyncs the entire transcript to reach it — O(session length) work for a fixed-size read.

Nobody is currently paying for this, and I want to be upfront about that: the hook ships in install/hooks/ but is registered in neither settings.system.json nor settings.enhancements.json, and the hooks README has no row for it. So this is a latent defect in shipped-but-unwired code, not a live regression. It bites whoever wires the hook up themselves — which is how I found it.

The measurement

On a 152 MB transcript, each implementation run in its own process so the numbers don't share cache effects:

per call (warm, 5-run avg) process RSS
current (readFileSync) 22.1 ms 183 MB
this patch (openSync/readSync seek) 0.1 ms 30 MB

The hook runs on every UserPromptSubmit, so the cost scales with session length. In practice it doesn't look like a performance bug — it looks like an intermittent hook timeout that only appears late in long sessions, which is a considerably more annoying thing to debug.

Behaviour preservation

I ran the old and new implementations side by side over every transcript larger than 100 KB on one machine — 7763 files, 1.1 MB to 152 MB:

differential: 7763/7763 agree, 0 disagree

That covers the null path (no assistant message yet), partial trailing lines at the tail boundary, and files smaller than the tail window.

Scope

One function. The import list gains openSync/readSync/closeSync; readFileSync stays because settingsPin() still uses it. Nothing else in the file changes, and no other file is touched.

One question for you, separate from the patch

Is this hook meant to be registered? It ships but nothing wires it, which is presumably why the defect has gone unnoticed. Two coherent answers and I have no stake in which:

  • Wire it — then this fix matters before anyone turns it on, and the hooks README wants a UserPromptSubmit row.
  • Drop the file — then close this PR; a shipped hook that nothing registers is dead weight either way, and that's a cleaner outcome than a fixed hook nobody runs.

Happy to follow up with either. If you'd rather not carry a fix for code you're about to delete, say so and I'll close it myself.

…whole file

liveModel() computes a 256KB tail offset and then readFileSync's the entire
transcript to reach it - O(session length) work for a fixed-size read. The hook
runs on every UserPromptSubmit, so the cost grows with the session and only
bites late in long ones, where it presents as a flaky hook timeout rather than
a bug.

Measured on a 152MB transcript (isolated processes, warm cache):

  before   22.1 ms/call   183 MB RSS
  after     0.1 ms/call    30 MB RSS

Behaviour-preserving: a differential run of the old and new implementations
over every transcript >100KB on one machine agrees on all 7763 files, covering
the null path, partial trailing lines, and files smaller than the tail window.

Nothing else in the file changes.
@danielmiessler

Copy link
Copy Markdown
Owner

Good fix, and your differential run over 7,763 transcripts is the kind of verification we wish every PR carried. The same seek-the-tail implementation landed in source a few hours before this PR via @elhoim's #1738 (credited at the fix site), so closing this one as already-landed rather than porting a second copy. The latent-defect framing (shipped-but-unwired) was accurate and appreciated — you're in the README credits for the report.

Sign up for free to 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.

2 participants