fix(llmloop): scope async memory compression to each RunPerFile conversation

[chethanuk]

fix(llmloop): scope async memory compression to each RunPerFile conversation

Problem

One Runner serves a whole review session, and both internal/agent and internal/scan fan out one goroutine per file over it (default concurrency 8). Async memory compression state, however, lived on the Runner as a single shared slot (compressionMu + pendingJob). As analyzed in #384 (and confirmed by the maintainer), that shared slot breaks per-conversation semantics once concurrency > 1.

Root cause — four concrete defects

1. Cross-file apply — tryApplyPendingCompression had no owner check: file B could consume file A's completed job, splicing [frozen + A's summary + A's active rounds] into B's message history. A review-correctness bug, not just wasted work.
2. Cross-file cancel / replace — the warning-threshold paths canceled whichever file's job happened to be pending, so another file's in-flight compression request died with context canceled (rendered as an HTTP 500 by some gateways); triggerAsyncCompression also overwrote the slot unconditionally, burning the superseded request.
3. Same-call start-then-cancel — the soft-threshold trigger ran before the new assistant/tool messages were appended. If the append crossed the warning threshold, the same addNextMessage call canceled the job it had started microseconds earlier (the cancellation-within-milliseconds seen in the issue).
4. Data race — the pendingJob == nil fast-path read in addNextMessage was unlocked.

Fix

• New compressionState { mu sync.Mutex; pendingJob *compressionJob }, created per RunPerFile call and threaded through triggerAsyncCompression / tryApplyPendingCompression / cancelPendingCompression. Isolation holds by construction: a conversation physically cannot see another conversation's job. The state is GC'd with the call — no cleanup maps, no keying by file path (which would collide if the same path is reviewed twice).
• The nil-pending gate moved from the caller's unlocked read into triggerAsyncCompression as an atomic check-and-set under st.mu (also fixes 4).
• The async trigger moved to the end of addNextMessage, gated on the post-append count sitting strictly between the soft (60%) and warning (80%) thresholds — start-then-cancel within one update is now structurally impossible, and no job starts when the call is about to return false (also fixes 3).
• RunPerFile defers cancelPendingCompression(st), so an in-flight job is aborted when its conversation ends instead of running up to 5 minutes orphaned.
• Aggregate token counters, warnings, and tool-call counts stay Runner-level, exactly as before. No exported API changes; internal/agent and internal/scan are untouched.
• Failure reporting follows ownership: the [ocr] Memory compression failed line is printed by the sync call sites and by the async job only while it still owns the slot. A deliberately cancelled job — superseded by sync compression at the warning threshold, or aborted by RunPerFile's deferred cancel at conversation end — dies silently (its session task record is still written, so nothing is hidden from debugging). Without this, the fix's own intentional cancels would keep printing the issue's exact context canceled symptom whenever a job was still in flight at the warning threshold or when the model called task_done a round after a trigger.

Intentional behavior deltas (all strictly safer): the async snapshot now includes the just-appended round (strictly more history summarized); in-flight jobs are canceled at conversation end; no async job starts on a failing update.

Tests

New regression tests (channel-synchronized, no sleeps):

Test	What it proves
TestCompression_CrossFileIsolation	With file A's compression request held in flight by a gated fake client, file B's cancel + apply touch nothing: A's job stays pending, zero requests canceled (defect 2); also pins that a second trigger while a job is pending is a no-op, not a replacement
└ SummaryAppliesOnlyToOwningConversation (subtest)	After A's job completes, B cannot consume it and B's messages stay byte-identical; A applies its own summary and a message appended after the snapshot survives (defect 1)
TestAddNextMessage_NoStartThenCancelSameCall	Pre-append count in (soft, warn), post-append over warn — the #384 same-file case: exactly one sync compression, zero mid-flight cancellations, no dangling async job (defect 3)
TestRunPerFile_ConcurrentFilesCompression_Race	4 concurrent RunPerFile calls on one Runner crossing both thresholds repeatedly; asserts -race cleanliness end to end (defect 4), and asserts compression requests were actually issued so the test can never silently go vacuous

The RED baseline was demonstrated on unmodified main: a gated-client test showed file B's cancel destroying file A's pending job (file A's job should still be pending after file B's cancel).

All seven existing compression unit tests were updated mechanically to the per-conversation API; the remaining compression edge cases (empty template no-op, LLM error → messages unchanged, empty summary → unchanged, snapshot-suffix preservation, fence stripping) are unchanged and still green.

go test ./internal/llmloop/ -race -count=1   # green
make check && make test && make coverage      # green, coverage 81.1% (≥ 80% gate)
go build ./cmd/opencodereview                 # green

E2E

Mirroring the issue's setup: 2-file review at --concurrency 2 against a local OpenAI-compatible stub, with responses sized so each conversation crosses the soft threshold (async compression) and the warning threshold (sync compression). Result: both files review_item_done, memory_compression_task records present for each file, and zero context canceled / Memory compression failed in stdout or session records.

Fixes #384

[github-actions]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)

[lizhengfeng101]

Review

Excellent concurrency bug fix. The root cause analysis is thorough (four distinct defects cleanly decomposed), the fix eliminates the problem structurally (per-conversation isolation rather than more locks), and the tests are well-designed with channel synchronization instead of sleeps. A few minor nits below, none blocking.

1. copyMessages runs under st.mu in triggerAsyncCompression

st.mu.Lock()
if st.pendingJob != nil { st.mu.Unlock(); return }
msgSnapshot := copyMessages(messages)  // still holding st.mu
...
st.mu.Unlock()

Since st is per-conversation, the only contender is the background goroutine's completion handler, so there's no real performance concern. But copyMessages is unrelated to the mutex-protected state (pendingJob) — it could be moved outside the critical section (check-and-set a sentinel under the lock, unlock, copy, re-lock to install the real job). That said, the current approach is safe and simpler. Just a note, not a blocker.

2. Comment describes lock ordering that doesn't match the code

In the async goroutine:

// cancelPendingCompression clears pendingJob before cancelling

But the actual code in cancelPendingCompression calls cancel() first, then sets st.pendingJob = nil. Both happen under st.mu, so the goroutine can never observe the intermediate state — behavior is correct. The comment just describes the wrong order. Suggest rewording to something like "cancels and clears pendingJob under the lock".

3. Consider a brief note on the pre-append compression failure path

In addNextMessage, when pre-append sync compression fails, execution continues (appends new messages, may retry via the post-append check). This is the right degradation behavior — and an improvement over the old code which silently ignored the error (_, _ = r.runCompression). A one-line comment like "// compression failed; continue with over-limit messages, post-append will retry" would make the intent more discoverable, but this is optional.

Summary

Low-risk, structurally sound fix. The per-conversation compressionState eliminates cross-file interference by construction. Test coverage is thorough — gatedLLMClient is a clean pattern, the cross-file isolation test pins all four defects, and the race test guards against regression. LGTM.

[chethanuk]

Thanks for the thorough review @lizhengfeng101! Items 2 and 3 fixed in aa08bf4: the async-goroutine comment now reads "cancels and clears pendingJob under the lock" (matching the actual order), and addNextMessage notes that a pre-append compression failure continues with over-limit messages and is retried by the post-append check.

For item 1 (copyMessages under st.mu), kept as-is per the inline-thread discussion: the check-first order avoids paying a full-conversation copy on every soft-zone round while a job is already pending (the bot's restructure copies before the pending check), the nil-pending read must stay under st.mu anyway (the unlocked read was defect 4 of #384), and there's no contention to shorten — only the conversation's own goroutine calls the trigger. Happy to restructure if you feel strongly.