Skip to content

perf(core): bound session-list metadata reads to head/tail 64KB; pool buffer; lazy message count#3897

Merged
wenshao merged 13 commits into
QwenLM:mainfrom
qqqys:feat/session-list-perf
May 10, 2026
Merged

perf(core): bound session-list metadata reads to head/tail 64KB; pool buffer; lazy message count#3897
wenshao merged 13 commits into
QwenLM:mainfrom
qqqys:feat/session-list-perf

Conversation

@qqqys

@qqqys qqqys commented May 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

/resume open time scales with total disk bytes today, dominated by per-file readline to count messages and an unbounded Phase-2 fallback when title metadata isn't in the tail window. This PR makes the read path fixed-cost in file size for both common and worst case:

  • Drop the per-file message-count scan from listSessions and findSessionsByTitle. SessionListItem.messageCount is now optional; a new public SessionService.countSessionMessages(sessionId) lets the SessionPreview panel get an exact count lazily.
  • Pool a 64 KB scratch buffer across the per-file metadata reads in both call sites — mirrors claude-code's enrichLogs pattern, eliminates ~1.3 MB of per-/resume Buffer churn.
  • Replace the Phase-2 full-file fallback with a head-window read. The reader now does at most tail 64KB → head 64KB → undefined. The previous full-file scan (capped at 64 MB) is gone.
  • Add a writer-side re-anchor invariant in ChatRecordingService so the title stays in the tail window between lifecycle events. Every 32 KB of non-title content triggers a fresh custom_title append at EOF; sessions that never set a title pay zero overhead.

The reader's tighter bound and the writer's re-anchor are coupled — together they replace "scan as much as it takes" with "scan a fixed 128 KB; the writer maintains the invariant." A title that somehow ends up buried mid-file (legacy data, crashed session) returns undefined and the picker falls back to first-prompt for display, instead of freezing the UI on a multi-MB scan.

Why this matters

For users with hundreds of sessions or large agent transcripts, /resume was visibly stalled at "Loading sessions…" for seconds. The full-file readline was the only step that scaled with file size rather than file count. After this PR, listing latency depends only on session count and is bounded per file.

Compatibility

  • SessionListItem.messageCount becomes ?: number. CLI / VSCode-companion / WebUI consumers already handle the optional case (?? 0, ?? messages.length, typeof === 'number'); only the row badge in SessionPicker needed a guard, included here.
  • Public countSessionMessages(sessionId) keeps the door open for any future preview-style consumer that wants the precise count without re-deriving it.
  • AI titles and manual titles use the same custom_title record + titleSource discriminator and both ride the re-anchor invariant — the picker's "dim auto titles" affordance stays intact across long sessions.

What stays out

  • Tag / agent / mode / PR-link metadata that claude-code re-anchors are not added here — those features don't exist in qwen-code yet, so adding them now would be premature.
  • Multi-process external-writer absorption on re-anchor (claude-code reads the tail before re-appending to merge SDK writes from another CLI tab) is not included. Single-CLI workflows are not affected; multi-process power users are rare in qwen and we can layer this in later if it becomes an issue.

Test plan

Unit

  • tsc --noEmit clean for both packages/core and packages/cli
  • Core test suites passing: 131/131 across sessionService.test.ts, sessionService.rename.test.ts, sessionStorageUtils.test.ts, chatRecordingService.test.ts, chatRecordingService.customTitle.test.ts, chatRecordingService.autoTitle.test.ts
  • New regression pins in those tests for: listing without messageCount, head-window fallback, "buried beyond both windows returns undefined", scratch buffer reuse across tail+head reads, three re-anchor invariant scenarios

E2E

A standalone harness against real node:fs writes covers every contract this PR introduces:

  • 35/35 scenarios pass, exit code 0
  • Reader: R1–R10 cover tail/head/buried/empty/multiple/boundary/truncated
  • Multi-field reader: M1–M4 cover atomicity + head fallback + legacy records + buffer reuse
  • Writer re-anchor: W1–W7 cover threshold trigger, no-spurious, rename mid-session, titleSource preservation
  • Listing: L1–L6 cover latency, regression pin for messageCount=undefined, findSessionsByTitle (second use site)
  • Stress + concurrency: S1 (200 × 3 MB), C1 (10 parallel listSessions)
  • Integration: I1–I5 cover writer + reader + loadSession end-to-end

Headline numbers

Metric Result
Reader single-file worst case 0.72 ms (1 MB file, head fallback)
Reader 5 MB file with tail hit 0.16 ms
listSessions(50) on 50 × 4 MB sessions 55.15 ms median
Same workload, legacy countSessionMessages baseline 141.64 ms median — 2.6× slower
findSessionsByTitle single match 4.46 ms
Stress: 200 × 3 MB sessions, page=50 hot 137 ms
10 concurrent listSessions identical results, no race
Writer overhead from re-anchor <0.5% extra bytes

The 2.6× speedup is measured on 4 MB files. The gap scales linearly with average file size — the new path reads a fixed 128 KB per file regardless. Estimated speedups by session size: ~10× at 20 MB, ~25× at 50 MB.

`listSessions` previously called `countSessionMessages` per file, which
streamed the entire JSONL through `readline` to count unique
user/assistant UUIDs. For a project with N sessions averaging M bytes
each, every /resume open paid O(N · M) wall time before showing the
picker — by far the dominant cost once a project accumulated many
multi-MB sessions.

This change:

- Drops the per-file count from listSessions / findSessionsByTitle.
  `SessionListItem.messageCount` is now optional. Callers that need
  a count call the new public `SessionService.countSessionMessages
  (sessionId)` lazily — typically only when a SessionPreview panel is
  about to display the badge.
- Pools a single 64KB tail-read buffer across the per-file metadata
  reads in listSessions / findSessionsByTitle, mirroring the pattern
  in claude-code's `enrichLogs`. The two helpers in
  `sessionStorageUtils` (`readLastJsonStringFieldSync` and
  `readLastJsonStringFieldsSync`) accept an optional caller-owned
  scratch buffer; one-off callers (rename, single-session lookup) pass
  nothing and keep the original alloc behaviour.
- Updates SessionPicker's row metadata to omit the "N messages"
  segment when `messageCount` isn't available, keeping the visual
  layout intact.

Tests:
- Pin that listSessions does not populate messageCount (regression
  guard against silently re-introducing the per-file scan).
- Smoke test the buffer-pool plumbing — same caller-owned buffer
  handed to two reads of different file sizes returns both correct
  values without state bleed.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qqqys qqqys marked this pull request as draft May 7, 2026 07:35
@qqqys qqqys marked this pull request as ready for review May 7, 2026 07:47
Tightens the title-write / title-read invariant so the picker's
metadata read is bounded to a fixed 2 × 64KB per file, regardless
of session length, with no fallback that scales with file size.

Writer (`ChatRecordingService`):
  - New `bytesSinceTitleAnchor` counter and `TITLE_REANCHOR_BYTES`
    threshold (32 KB, half of LITE_READ_BUF_SIZE).
  - `appendRecord` now updates the counter and, once it crosses the
    threshold while a title is set, re-appends a fresh `custom_title`
    record to EOF (`reanchorTitle`). The recursive append routes
    back through the same tracking path, which sees the title
    record and resets the counter to zero.
  - Sessions that never set a title pay zero overhead — the early
    return in `updateTitleAnchorTracking` short-circuits.

Reader (`sessionStorageUtils`):
  - `readLastJsonStringFieldSync` / `readLastJsonStringFieldsSync`
    now read the file's last 64 KB, fall back to the first 64 KB
    on miss, and return `undefined` if neither contains the field.
    The previous Phase-2 streaming full-file scan (capped at 64 MB)
    is removed entirely.
  - The pooled scratch buffer (already optional) now backs both the
    tail and head reads — only one allocation per `listSessions`
    page even with the head fallback firing.
  - `MAX_FULL_SCAN_BYTES` constant deleted (unused).

The two changes are coupled: the reader's tighter bound only works
because the writer guarantees the title stays in tail. A title
buried mid-file is now intentionally `undefined` (the picker
falls back to `firstPrompt` for display) rather than triggering a
full-file scan that would freeze the UI on long agent transcripts.

Tests:
  - `chatRecordingService.customTitle.test.ts` — three scenarios
    pinning the re-anchor invariant: threshold trigger, no-spurious
    on no-title sessions, no-trigger on small write bursts.
  - `sessionStorageUtils.test.ts` — replaces Phase-2 tests with
    head-window fallback tests; pins the new "buried beyond both
    windows returns undefined" contract; updates buffer-pool reuse
    test to cover both tail and head reads with the same scratch.

E2E coverage (35/35 scenarios on a separate harness against real
fs): R1–R10 reader contract, M1–M4 multi-field, W1–W7 writer
re-anchor, L1–L6 listing latency + regression pins, S1 stress
(200 × 3 MB), C1 concurrency, I1–I5 writer/reader integration.
Measured 2.6× speedup on listSessions(50) over 50 × 4 MB
sessions vs the legacy `countSessionMessages` baseline; speedup
scales linearly with average file size.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qqqys qqqys changed the title perf(core): drop full-file readline + pool tail buffer in listSessions perf(core): bound session-list metadata reads to head/tail 64KB; pool buffer; lazy message count May 7, 2026

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (not mapped to specific diff lines):

  • [Critical] packages/core/src/services/sessionService.ts: New public API countSessionMessages(sessionId) has zero test coverage — no tests for valid sessionId returning correct count, invalid sessionId returning 0, or file-not-found graceful degradation.
  • [Suggestion] packages/core/src/utils/sessionStorageUtils.test.ts: readLastJsonStringFieldsSync gained a scratchBuffer parameter but lacks a corresponding buffer-reuse test (the single-field variant has one).
  • [Suggestion] packages/core/src/services/chatRecordingService.customTitle.test.ts: The falsy titleSource branch in reanchorTitle is untested — only the 'manual' path is covered.

— deepseek-v4-pro via Qwen Code /review

Comment thread packages/core/src/services/chatRecordingService.ts
Comment thread packages/core/src/services/chatRecordingService.ts
Comment thread packages/core/src/utils/sessionStorageUtils.ts
- countSessionMessages: valid id counts unique user/assistant uuids and
  ignores other types/malformed lines; invalid id short-circuits without
  filesystem access; ENOENT degrades to 0 instead of bubbling.
- readLastJsonStringFieldsSync: mirror the single-field variant's
  scratch-buffer reuse test across a tail-hit then head-fallback to
  catch any decode that ignores bytesRead.
- ChatRecordingService re-anchor: legacy resumed session (source
  undefined) must omit titleSource on threshold-triggered re-anchor,
  not silently reclassify as 'manual'.

Co-authored-by: Qwen-Coder <noreply@alibabacloud.com>
qqqys and others added 3 commits May 8, 2026 19:46
`String.length` undercounts the on-disk size of multi-byte payloads
(CJK, emoji are 1 UTF-16 unit but 3 UTF-8 bytes), so a session full
of CJK content could push >96KB of writes past the last anchor before
the 32KB threshold thinks it has — silently drifting the title past
the 64KB tail window the picker scans. Switch the byte counter to
`Buffer.byteLength(..., 'utf8')` for parity with `jsonl.writeLine`.

Also reset `bytesSinceTitleAnchor` to 0 in the reanchor catch:
without it, a failing reanchor pins the counter at the threshold
and turns a single transient I/O fault into a per-record retry
storm. One missed anchor is the right tradeoff — finalize() will
re-emit on the next lifecycle event.

Co-authored-by: Qwen-Coder <noreply@alibabacloud.com>
The 64KB head-window fallback in `readLastJsonStringFieldSync` /
`readLastJsonStringFieldsSync` reads a fixed slice and hands it
straight to the extractor — its trailing bytes can fall mid-record.
A partial line whose `customTitle` value happens to close inside
the buffer but whose body extends past 64KB would otherwise win
the latest-match race and surface as the picker's title.

Drop everything past the last `\n` before extracting (only when the
buffer is shorter than the file — a small file is necessarily whole-
line). Honors the original Phase-2 contract that only complete lines
get a vote, without paying for the deleted full-file scan.

Co-authored-by: Qwen-Coder <noreply@alibabacloud.com>

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up after the three fix commits (2f72b925 + e1f502e2 + f659cd61).

Prior findings — verified fixed

Prior concern Fix How verified
[Critical] UTF-16 vs UTF-8 byte count in bytesSinceTitleAnchor chatRecordingService.ts:754 now uses Buffer.byteLength(JSON.stringify(record), 'utf8') + 1 New CJK test ('汉'.repeat(1500) × 12) — under threshold by UTF-16, over by UTF-8, anchor fires only when bytes are counted right
[Critical] reanchorTitle failure pinned counter at threshold → retry storm chatRecordingService.ts:789 resets bytesSinceTitleAnchor = 0 in catch New test injects a sync throw on every custom_title appendRecord and asserts exactly one failed re-anchor across 25 bulk records
[Suggestion] Head window could pick up a partial straddling record sessionStorageUtils.ts:314-317 and :399-402 truncate rawHead to last newline before extraction New "phantom partial trailing line" test (record begins inside the head window, closing quote past 64KB) — without the trim, phantom would win over complete
[Critical] countSessionMessages had zero coverage New describe('countSessionMessages', ...) block in sessionService.test.ts Three cases: counts uniques + ignores non-message types + tolerates malformed lines, rejects non-UUID input without touching disk, returns 0 on ENOENT
[Suggestion] scratchBuffer reuse not pinned New sentinel-byte reuse tests in both readLastJsonStringFieldSync and readLastJsonStringFieldsSync blocks Pre-fill buffer with 0x55, drive tail-hit → head-fallback on different file sizes; a buggy decode that read past bytesRead would corrupt the result
[Suggestion] Falsy titleSource re-anchor branch untested New "omits titleSource on re-anchor when source is unknown" test Asserts titleSource key is absent (not present-and-undefined) on the re-anchored record

Cross-cutting checks

  • The recursive reanchorTitle → appendRecord → updateTitleAnchorTracking → counter=0 reset path closes correctly on both success and sync-throw paths.
  • writeChain serialization preserves on-disk record ordering even when re-anchor is queued mid-turn.
  • Constructor's resume path (finalize() at :554) re-anchors the title on every session load, so process restarts can't leave the counter in a stale state — closes the "buried-on-resume" worry implicit in the original design.
  • custom_title records becoming intermediate parents in the parentUuid chain is consistent with the pre-existing finalize pattern, and reconstructHistory already filters them out via the record.message === undefined path at sessionService.ts:1142 and the explicit system skip at :1125 (compressed-history branch).

One tiny nit inline (purely optional). LGTM otherwise.

Comment thread packages/core/src/utils/sessionStorageUtils.ts
Comment thread packages/core/src/utils/sessionStorageUtils.ts
Comment thread packages/cli/src/ui/components/SessionPicker.tsx
Comment thread packages/core/src/services/sessionService.ts
Comment thread packages/cli/src/ui/components/SessionPicker.tsx
Comment thread packages/core/src/services/sessionService.ts
qqqys and others added 2 commits May 8, 2026 21:26
…ession-list-perf

# Conflicts:
#	packages/core/src/services/sessionService.ts
…, pin perf contract

Address QwenLM#3897 follow-up review findings:

- SessionPreview footer no longer drops the message count when listSessions
  omits it. The prop is the override path; default falls back to a unique
  user/assistant uuid count derived from the already-loaded conversation,
  matching countSessionMessages semantics with zero extra disk I/O.
- countSessionMessages now scopes to the current project, mirroring the
  first-record cwd check in deleteSession/renameSession/loadSession. A
  valid sessionId from another project sharing the chats dir no longer
  bypasses project boundaries on lazy count.
- New regression tests:
  - SessionPicker row renders cleanly with messageCount === undefined
    (no "messages"/"undefined"/dangling separator)
  - findSessionsByTitle perf contract: matches have messageCount
    undefined and fs.createReadStream is never called
  - countSessionMessages: cross-project sessions return 0 without
    streaming, empty file returns 0
- Update corruption-recovery tests to call private countSessionMessagesFromPath
  (the streaming entry point) instead of the public sessionId-shaped API the
  merge from main pointed them at.

Co-Authored-By: Qwen-Coder <noreply@qwen.com>
@qqqys

qqqys commented May 8, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up after 2b7b656fd. All four findings from the latest review addressed.

Findings — addressed

Prior concern Fix How verified
[Critical] SessionPicker.tsx:95 preview drops message count when listSessions omits it SessionPreview.tsx now derives the count from the already-loaded ResumedSessionData.conversation.messages, filtering on type === 'user' || type === 'assistant' and deduping by uuid — same semantics as countSessionMessages, zero extra I/O. The prop stays as an override path. New SessionPreview.test.tsx case: render with messageCount prop omitted, footer still shows 2 messages, no undefined
[Suggestion] sessionService.ts:370 countSessionMessages lacks project scoping Public method now reads the first record via jsonl.readLines(filePath, 1) and returns 0 unless getProjectHash(firstRecord.cwd) === this.projectHash, mirroring deleteSession / renameSession / loadSession. The private countSessionMessagesFromPath keeps doing the streaming pass and is unchanged. New sessionService.test.ts case: cross-project record makes countSessionMessages return 0 and fs.createReadStream is not called (short-circuit before the expensive part). Empty-file path also covered.
[Suggestion] SessionPicker.tsx:97 messageCount === undefined render path is the new prod default but not tested New render test in StandaloneSessionPicker.test.tsx: SessionListItem with messageCount: undefined and gitBranch: 'feature-branch' — asserts time + branch render, no messages, undefined, or doubled · separator. Picks up any future regression that re-introduces undefined messages or a dangling middle dot
[Suggestion] sessionService.ts:973 findSessionsByTitle perf contract unprotected for second call site New test in sessionService.rename.test.ts: matched items asserted to have messageCount === undefined, and fs.createReadStream spy asserted not-called for the duration of the findSessionsByTitle call. Re-introducing the per-match readline count would now fail loudly

Cross-cutting

  • vi.restoreAllMocks in afterEach keeps the context-sensitive getProjectHash mock from the cross-project test from leaking into other suites.
  • countSessionMessagesFromPath (the streaming/parseLineTolerant private helper) keeps its existing behavior; the corruption-recovery integration tests in sessionService.corruption.test.ts now address it directly via the same as unknown as Privates cast pattern, since the public countSessionMessages(sessionId) enforces the UUID pattern + project scoping that those tests deliberately bypass.
  • tsc --noEmit clean for both packages/core and packages/cli. Full suites green: 642 / 642 core, 1166 / 1166 cli.

The optional : rawHead dead-branch nit in sessionStorageUtils.ts:317 / :402 was left as-is (purely defensive, no behavior change).

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Two additional stale comments reference removed behavior:

  1. sessionStorageUtils.ts:226 — Section header says // File I/O — tail-first scan with full-file fallback, but the full-file fallback was replaced by a head-window. Should say // File I/O — tail-first scan with head-window fallback.

  2. sessionService.ts:180 — JSDoc on readSessionTitleFromFile says "falls back to a full-file scan when the tail has no match", but the underlying function now uses a bounded head window, not a full-file scan.

[Suggestion] ACP renameSession handler bypasses ChatRecordingService. The ACP agent's rename path (acpAgent.ts:~494) creates a new SessionService and writes the title directly via jsonl.writeLineSync. If a ChatRecordingService is actively recording the same session, its currentCustomTitle is never updated, so the new re-anchor invariant re-emits the old title every 32KB of writes, silently reverting the rename. Other ACP handlers correctly look up the live session; the rename handler should route through ChatRecordingService.recordCustomTitle when a live session exists.

— glm-5.1 via Qwen Code /review

Comment thread packages/core/src/services/chatRecordingService.ts
// per-file metadata read. Without pooling, each session in the
// page allocs+GCs a fresh 64KB Buffer; for a typical page of 20
// that's ~1.3MB churn per /resume open. Phase-2 (full-file
// fallback) inside the helper still allocs per chunk because

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This comment references Phase-2 (full-file fallback) and Phase-2 is rarely entered, but Phase-2 was replaced by a single head-window read in this PR. The helper now does one bounded read with the shared scratchBuffer, not chunk-based streaming. The last two lines of this comment should be removed.

— glm-5.1 via Qwen Code /review

Comment thread packages/core/src/services/chatRecordingService.ts
qqqys and others added 2 commits May 9, 2026 11:13
The session metadata reader uses two bounded 64KB windows (tail + head)
since the perf rework on this branch — never a full-file scan. Two
comments still described the prior behavior. Reported in MR review.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
ACP renameSession constructed a fresh SessionService and wrote the
custom_title record straight to disk. When the same session was live
in this process, ChatRecordingService.currentCustomTitle stayed at
the old value, so the next title re-anchor (every 32KB) or finalize()
re-emitted the stale title at EOF and silently reverted the rename.

Now we look up the live ChatRecordingService first and route through
recordCustomTitle, which keeps the in-memory cache and the on-disk
record in sync. The SessionService path remains for the non-live case
(e.g., another client renaming a backgrounded session). Reported in
MR review.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Comment thread packages/cli/src/acp-integration/acpAgent.ts
Comment thread packages/core/src/utils/sessionStorageUtils.ts
Comment thread packages/core/src/services/chatRecordingService.ts
Comment thread packages/cli/src/acp-integration/acpAgent.ts
Comment thread packages/cli/src/acp-integration/acpAgent.ts

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings from second-opinion review (glm-5.1):

  • [Suggestion] sessionStorageUtils.test.ts — The multi-field variant readLastJsonStringFieldsSync lacks a "buried beyond both head and tail windows" test. The single-field variant has an explicit anti-test that constructs a file >2×LITE_READ_BUF_SIZE with the target in the middle, asserting undefined. The multi-field variant has the same head/tail windowing logic but the return emptyResult path is untested. A regression in the multi-field head-window logic could silently break title+source extraction with no test failing.

  • [Note — false positive in open comment (acpAgent.ts:500)] The open Critical comment claims this.sessions.get(sessionId)?.getConfig().getChatRecordingService() throws TypeError when the session is absent. This is incorrect — JavaScript's ?. short-circuits the entire remaining chain, so undefined?.getConfig().getChatRecordingService() evaluates to undefined without error. Verified by two independent review agents. No code change needed; consider resolving the comment.

* Two bounded windows, never a full-file scan:
* 1. Scan the last LITE_READ_BUF_SIZE bytes of the file. This is the
* common path because `ChatRecordingService` re-anchors metadata
* records to EOF every {@link TITLE_REANCHOR_BYTES} (≤ tail-window

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] {@link TITLE_REANCHOR_BYTES} references a constant defined in chatRecordingService.ts (a different package). The {@link} won't resolve — IDE hover/intellisense can't navigate to it. Replace with an inline description like "every 32KB (TITLE_REANCHOR_BYTES)".

— glm-5.1 via Qwen Code /review

*
* Without this, a long agentic turn that streams >64KB of tool
* output could push the only `custom_title` record past the 64KB
* tail window, forcing the picker into a full-file fallback.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This JSDoc says "forcing the picker into a full-file fallback," but the full-file fallback was removed by this PR — the picker now does a bounded head-window read (64KB from offset 0) or returns undefined. The stale wording could mislead a future reader into thinking full-file scanning still exists.

Suggested change
* tail window, forcing the picker into a full-file fallback.
forcing the picker into a head-window fallback (or returning undefined if the title is beyond both windows).

— glm-5.1 via Qwen Code /review

Comment thread packages/core/src/utils/sessionStorageUtils.ts

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving against HEAD 3ca0829a.

6 prior [Critical] threads — all resolved:

  • 4 fixed: UTF-8 byte counting (e1f502e2), reanchor failure reset (e1f502e2), lastRecordUuid preserved across reanchor (96ed42cb), preview message count via SessionPreview in-memory compute (2b7b656f).
  • 1 documented as bounded acceptable cost: statSync/readSync race + readLatestTailIfGrown one-shot recovery, JSDoc spells out the trade-off.
  • 1 retracted: my mis-read of optional-chain short-circuit semantics on acpAgent.ts:500 (Session.getConfig() is non-nullable, so the chain is safe as written).

11 [Suggestion] threads — all resolved with explicit dispositions:

  • 2 fixed: head-window line-truncation (f659cd61), project-scope check on countSessionMessages (2b7b656f).
  • 3 author trade-off accepted: defensive : rawHead ternary, double-stringify cost, 64KB literal in TITLE_REANCHOR_BYTES comment.
  • 6 deferred to follow-up: 4 test-coverage gaps (picker undefined-messageCount render, findSessionsByTitle perf contract, ACP recording-disabled fallback, multi-field bounded read tail-coverage edge), async-write outcome surfacing in recordCustomTitle, stale fileSize guard at line 334 for the rare <64KB→>128KB growth race.

CI green on all four platforms. Ready to land.

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