fix(qoder-cn): bound unbounded session anchor map and SQLite backlog reads - #227
fix(qoder-cn): bound unbounded session anchor map and SQLite backlog reads#227Snssn wants to merge 1 commit into
Conversation
… reads Two structures in the qoder / qoder-cn collection path had no upper bound, so the long-lived daemon's RSS grew with usage instead of staying flat. `QoderCnTraceInput.sessionAnchor` was a plain Map that was only ever written and read, never deleted: every new IDE session added a permanent entry for the lifetime of the process. It now uses a capacity-bounded LRU map (500 entries), where a read counts as use so an actively merged session is never dropped. There is deliberately no idle TTL — a user can leave a session open and resume it later, and expiring the anchor on a timer would re-parent the resumed turns onto a new turn id, splitting one conversation into two traces. `BaseSqliteInput.collect()` called `readNewRows()`, and both subclasses issued `WHERE rowid > ? ORDER BY rowid ASC` with no LIMIT. A collector that fell behind would materialise the entire backlog in one cycle — every row, every resulting entry, and the whole flusher hand-off at once. The row limit now lives in the base class (`readNewRows(lastRowId, limit)`) so no subclass can reintroduce an unbounded query, and a cycle drains up to 10 batches of 1000 rows, persisting the cursor after each batch so a stop mid-catch-up replays nothing. `qoder-work-sqlite` already had `LIMIT 1000`; this brings the other two in line. Measured on a 60k-row backlog: peak heap 40.4 MB batched vs 221.7 MB for the previous single unbounded read, and the batched figure is flat in backlog size rather than linear in it. Also removes `BoundedTtlCache` from `utils/git-context.ts`. It was exported but imported nowhere, and its eviction compared against the hardcoded module constant instead of the instance's own capacity, so any other capacity was either never bounded or silently trimmed to 256.
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Fixes two unbounded data structures in the long-lived collector daemon that caused RSS to grow with usage: sessionAnchor (plain Map → BoundedLruMap(500)) and readNewRows (unbounded SQL → batched reads of 1000 rows × 10 batches/cycle). The measured peak heap reduction from 221.7 MB to 40.4 MB on a 60k-row backlog is compelling.
Findings
- [Info]
base-sqlite-input.ts:52— MovingLIMITinto the base class abstract method signature is a strong structural guard against regression. - [Info]
bounded-lru-map.ts:38— The delete-during-iteration pattern is safe in ES6+ but could benefit from a brief inline comment. - [Info]
qoder-cn-trace-input.ts:50— No-idle-TTL decision is well-justified; eviction log throttle is a nice touch.
Review Notes
Strengths:
- Crash safety: cursor persisted per batch, not per cycle — a shutdown mid-catch-up replays nothing.
- The
BoundedLruMapis a clean, dependency-free implementation with proper LRU semantics (read = use via delete+reinsert). - Test coverage is thorough: 24 new tests covering batching edge cases, LRU eviction, session anchor bounds, and output equivalence.
- The drive-by removal of unused
BoundedTtlCachekeeps the codebase clean.
No blocking concerns. The code is well-structured, the measurements validate the approach, and the tests assert behavioral equivalence with the previous unbounded implementation.
Automated review by github-manager-bot
| let rows: SqliteRow[]; | ||
|
|
||
| for (const row of rows) { | ||
| try { |
There was a problem hiding this comment.
Good design choice — moving the LIMIT into the base class signature makes it impossible for subclasses to reintroduce unbounded queries. The abstract method contract (limit: number as a required parameter) enforces this at compile time.
| /** Returns how many entries were evicted to stay within capacity. */ | ||
| set(key: string, value: V): number { | ||
| this.entries.delete(key); | ||
| this.entries.set(key, value); |
There was a problem hiding this comment.
The for (const k of this.map.keys()) + this.map.delete(k) pattern is safe in ES6+ (Map iterators skip deleted keys correctly), but worth a brief code comment noting this, since it's a common source of confusion in code reviews.
There was a problem hiding this comment.
[not-fixed] 同意此建议，本轮核实 bounded-lru-map.ts:38-45 的驱逐循环仍无该注释。
证据: head=dadd3b32 src/utils/bounded-lru-map.ts 的 set() 中 for (const oldest of this.entries.keys()) { ... this.entries.delete(oldest) } 边遍历边删，行为在 ES6+ 安全但缺注释。属 Low 级可读性建议，不阻断合入，建议顺手补一行说明。
Generated by LoongSuite-Pilot Code Review Agent
| // through sessions drops the least-recently-used anchors instead of growing | ||
| // this map forever. | ||
| private readonly sessionAnchor = new BoundedLruMap<{ turnId: string; maxStep: number }>( | ||
| SESSION_ANCHOR_LIMIT, |
There was a problem hiding this comment.
The decision to omit an idle TTL is well-justified in the PR description and the code comment. The ANCHOR_EVICTION_LOG_INTERVAL_MS throttle is a nice touch to avoid log spam during catch-up.
Snssn
changed the title
🔍 Code Review Summary评审对象
Lifecycle Verdict
崩溃语义三段核验：collect 中途→未 emit 未 save，干净重放；emit 后 save 前→整段重放（at-least-once，与旧代码一致，非回归）；save 后→干净。cursor 推进移出 try 还顺带修掉旧代码「尾部坏行永久卡住 cursor」的隐患。 Merge Gate（合入门禁）APPROVE-READY ✅（阻断级别 Critical/High/Medium 均为 0；Lifecycle 三项全 PASS。Low 不阻断合入。） 总体结论可以合入。两处 Low 均为可读性/注释表述问题，不影响功能与正确性，建议作者顺手处理：
Highlights（正向实践）
评审报告详见: |
linrunqi08
left a comment
linrunqi08
left a comment
There was a problem hiding this comment.
✅ Medium/High 及以上问题为 0，Lifecycle 三项全 PASS，本轮评审通过（approved）。仅余 2 处 Low（注释表述 + 一行可读性注释），不阻断合入。详见评审摘要。
Generated by LoongSuite-Pilot Code Review Agent
3 participants
Summary
Two structures in the qoder / qoder-cn collection path have no upper bound, so the
long-lived daemon's RSS grows with usage instead of staying flat. Both are fixed here.
1.
QoderCnTraceInput.sessionAnchorgrew forever. It is a plainMapthat is onlyever written and read, never deleted — every new IDE session adds a permanent entry for
the lifetime of the process.
It now uses a capacity-bounded LRU map (500 entries, matching the existing
WINDOW_STATE_LIMITconvention inqoder-work-log/qoder-work-trace-input.ts). A readcounts as use, because the call site mutates the stored anchor in place rather than
re-inserting it, so an actively merged session is never dropped.
There is deliberately no idle TTL. A capacity cap already fully solves the unbounded
growth, whereas expiring on a timer would add a data-quality regression: a user can leave
a session open over lunch and resume it, and a dropped anchor would re-parent the resumed
turns onto a new turn id — splitting one conversation into two traces.
2.
readNewRowsread the entire backlog in one shot.BaseSqliteInput.collect()called
readNewRows(lastRowId), and both subclasses (qoder-cn-sqlite,qoder-sqlite)issued
WHERE rowid > ? ORDER BY rowid ASCwith noLIMIT. A collector that fellbehind — daemon stopped or paused while the IDE kept writing — would materialise every
backlogged row, transform all of them, and hand the whole array to the flushers at once.
token_infovalues are JSON blobs, so this is the path that can actually reach gigabytes.The row limit now lives in the base class (
readNewRows(lastRowId, limit)) so no subclasscan reintroduce an unbounded query. A cycle drains up to 10 batches of 1000 rows and
persists the cursor after each batch, so a stop mid-catch-up replays nothing. Draining
multiple batches per cycle avoids the other failure mode: a single 1000-row batch per 30s
poll would need over four hours to work through a 500k-row backlog.
qoder-work-sqlitealready hadLIMIT 1000; this brings the other two implementations inline and makes the bound a property of the base class rather than of each subclass
remembering to add it.
Measured effect
On a seeded 60,000-row backlog:
The batched figure is flat in backlog size while the previous one is linear in it, so the
gap widens as the backlog grows. Emitted
AgentActivityEntrycontent is unchanged — a testasserts the batched output is identical to a single unbounded read.
Drive-by removal
BoundedTtlCacheinutils/git-context.tsis deleted. It was exported but importednowhere, and its eviction called the module-level
evictStaleEntries, which comparesagainst the hardcoded
MAX_CACHE_ENTRIES(256) rather than the instance's ownmaxEntries— so any instance built with a different capacity was either never bounded orsilently trimmed to 256.
gitContextCachekeeps its own eviction, since git context mustexpire relative to the fetch rather than the last read.
Test plan
npm run typecheckcleannpm run buildpassestests/unit/utils/bounded-lru-map.test.ts(10 cases): capacity never exceeded,LRU eviction order, read-counts-as-use survival through churn, the boundary where a
burst larger than the whole map does evict a recently used entry, in-place mutation
retained
tests/unit/inputs/base-sqlite-input-batching.test.ts(8 cases): multi-batchdrain in ascending rowid order, no query exceeds the batch limit, stop at the
per-cycle budget and resume with no gap or replay, per-batch cursor persistence,
rows yielding no entry still advance the cursor, a failing later batch keeps
already-collected entries without rewinding the cursor, and equivalence with a
single unbounded read
tests/unit/inputs/qoder-cn-trace-input.test.ts(5 new cases): orphanturns merge into a live anchor with contiguous step numbering across cycles, an
anchor survives a long idle gap, a forgotten anchor is evicted and the later orphan
turn then keeps its own ids, capacity is never exceeded, and an anchor read each
cycle survives heavy churn
qoder-cn-trace,qoder-cn-sqlite-reader,qoder-sqliteandqoder-work-sqlitesuites pass unchanged (66/66 across the affected files)Note on the full suite: 10 test files fail on unmodified
mainin my local environment(mostly
tests/unit/hooks/**.mjsfiles, plushermes-agent-event-log-flow,file-pipelineanddaemon-cache-dir). I baselined the same subset with these changesstashed and got the same failures, and none of those files reference any module touched
here — so they are pre-existing/environmental rather than caused by this PR.
Follow-ups not in this PR
readHookJsonl/base-hook-input.ts:160allocateBuffer.alloc(stat.size - offset),reading the whole unread tail of the hook JSONL in one go. That is a transient spike
rather than a monotonic leak, and it is shared by every hook input, so it deserves its
own change.
one dominated a specific multi-GB outlier still needs a heap snapshot from an affected
host.