Conversation
|
| Filename | Overview |
|---|---|
| static/sessions.js | Coordinates concurrent session loads and preserves bounded message windows during refresh and pagination. |
| static/messages.js | Keeps stream settlement and session recovery bounded while checking pane and stream ownership. |
| static/commands.js | Routes retry and undo through the bounded session reload path. |
| static/ui.js | Preserves transcript windows during reconnect, edit, and regenerate workflows. |
Reviews (7): Last reviewed commit: "test(fix-chat-msg-dump): align settlemen..." | Re-trigger Greptile
nesquena-hermes
left a comment
There was a problem hiding this comment.
🔬 Gate review (Codex) — SHIP ONLY WITH FIXES (3 reproduced bounded-window regressions)
Thanks @akay64 — the core bounded-window preservation is largely sound (195 targeted tests pass, syntax + ESLint + diff-check clean), and the goal is right (keep the window bounded so we don't reintroduce the #5966-class memory blowup). But the gate reproduced three silent regressions on the lifecycle paths before this can ship:
-
[SILENT — lost mutation,
/undoshows stale]static/sessions.js:1480+static/messages.js:7270— a same-session mutation reload (e.g./undo) returns early when_loadingSessionIdis already set, so the window refresh is dropped. Reproduced with a Node probe:/undoreported success while the old messages stayed visible. Fix: allow mutation reloads to supersede the current generation, and queue one post-load refresh whensession-updatedreports a newer count instead of returning solely because_loadingSessionIdis set. -
[SILENT — window collapse on reconnect]
static/ui.js:9199— reconnect recovery collapses an expanded bounded window back to 30._messageReloadLimitForSession()has no captured hint here, so a 90-row loaded window requests 30 and silently removes 60 visible rows (invalidating scroll position). Verified with the actual helper. Fix: derive the limit from_settledSessionMessageWindowLimit(null,{forceBounded:true}), or capture+clear the current-window hint around this request. -
[SILENT — scroll hijack]
static/messages.js:5414— follow-scroll intent is captured before the awaited bounded-window fetch and applied at:5623afterward, so if the reader scrolls up during the request, completion still forces them back to the bottom. Fix: re-capture/revalidate_shouldFollowMessagesOnDomReplace()after the await + ownership check, immediately before DOM replacement.
All three are the "correct window content, wrong lifecycle handling" class — lost messages on undo, dropped rows on reconnect, scroll yanked from under the reader. Once mutation/refresh work is superseded-not-dropped, the reconnect limit honors the current window, and scroll-follow is re-evaluated post-await, re-gate. Solid direction on a genuinely valuable perf fix.
|
@nesquena-hermes Thanks for the review. I addressed all three lifecycle regressions on the PR branch:
I also added focused regression coverage for the mutation queue, reconnect window preservation, and deferred scroll-follow race, plus updated older structural tests to reflect the loadSession coordinator/core split. |
🔬 Re-gate (Codex) — all three lifecycle regressions resolved, PASSThanks @akay64. I re-pulled the branch at 1. [was: await loadSession(activeSid,{force:true,keepStaleUntilLoaded:true,externalRefreshReason:'undo'});When a same-session load is already active, 2. [was: reconnect collapses 90→30] — FIXED. const refreshLimit=(typeof _messageReloadLimitForSession==='function'
? _messageReloadLimitForSession(sid) : 30)||30;and 3. [was: scroll yanked to bottom] — FIXED. One minor, non-blocking noteThe VerificationRegression coverage lands in Net: the "correct window content, wrong lifecycle handling" class is closed — undo refreshes, reconnect keeps the expanded window, scroll-follow is re-evaluated post-await. Direction and execution both look right. Re-gate: PASS from my side, pending your merge call. |
@nesquena-hermes I would suggest to wait, I have run into a case where /retry and /undo wont do a full dump of the transcript, but they will add a batch of 30 to the total. The behavior is better than before, so it wont dump 1500 messages in but also I would not consider it done yet. Id suggest to allow me to run another hardening pass and update this PR before we merge. You can also take the decision to merge as is as the current behavior is a LOT better than not having it at the moment and I will open another PR after hardening. Your call. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
🔬 Re-gate round 2 (Codex) — all 3 prior findings FIXED ✅, one new rotated-continuation edge
Excellent progress @akay64 — all three round-1 regressions are verified fixed:
- ✅
/undo,/retry, same-session mutation reload — mutation refresh now queues behind an active same-session load and awaits the bounded follow-up (no more stale-visible-after-success). - ✅ Reconnect preserves an expanded window — the current renderable window determines the request limit (90 stays 90, not collapsed to 30).
- ✅ Reader scrolling up during the terminal fetch — follow intent is re-evaluated after the await and stays sticky through DOM replacement (no forced scroll-to-bottom).
One new edge the coordinator introduced, on the context-compression rotation path:
- [SILENT — rotated continuation shows stale parent window]
static/messages.js:5429— after a compression session rotation, the settled-window fetch usesactiveSidinstead of the completed/rotatedcompletedSid, so the continuation session's messages + tool calls get overwritten with the stale parent-window data. Reproduced (Node): the resulting session id wascontinuation-newbut its visible transcript wasSTALE PARENT WINDOWinstead ofFINAL ANSWER. The current tests encode theactiveSidfetch, so they pass while missing this. Fix:_fetchSettledSessionMessageWindow(completedSid, completedSession)(use the completed/rotated sid, not the active one), and add a rotated-compression regression attests/test_settled_session_window.py:160asserting the bounded window + final answer come from the continuation session (not the parent).
That's the last one — a one-token sid fix (activeSid → completedSid) plus the rotated-session test. Everything else (stale-generation guarding, duplicate-refresh suppression, full-render promotion, bounded cap per #5966/#5974, and all other stream/reattach/activity paths) verified clean across 95 bounded-lifecycle + 198 streaming/compression tests. Fix the rotation sid + add the test and re-gate — should be green next round.
|
Changes:
Verification passed:
|
|
@nesquena-hermes Just wanted to check in if this is good to go now? |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Exact-head gate certificate: RED ⛔
Contributor SHA: c9d98d4c6e8a9918312599925720fb392a26c240
The earlier four lifecycle findings are fixed at this head. Current-master integration exposes two additional silent regressions, plus a mechanical rebase/CI blocker.
Blocking findings
-
Per-session SSE can remain permanently closed after a truncated turn settles (
static/messages.js:5785,:5806,:5826;_resumeSessionStreamAfterLiveChatat:7543)._clearOwnerInflightState()schedules the session-stream resume before the awaited bounded-window fetch.- The zero-delay callback runs while
S.activeStreamIdstill owns the chat stream, so_chatStreamActiveForSession(sid)returns true and the callback exits. S.activeStreamIdis cleared only after the await, and no later resume is scheduled. Background completion andsession-updatedevents can then be lost until navigation/reload.- Fix: resume
completedSidafter terminal ownership is cleared and final state is applied, on fetch success and fallback paths. Add a delayed-fetch regression test that proves the session SSE restarts after settlement.
-
Reconnect/manual recovery can silently discard loaded transcript rows (
static/sessions.js:3149-3182,:3238-3247;static/messages.js:6580;static/ui.js:9560-9568).- A fully loaded 100-row pane with
_messagesTruncated=falseproducesmsg_limit=30; replacement drops 70 visible rows. - An expanded 600-row pane produces
msg_limit=600; the backend clamps this to_MAX_MSG_LIMIT=500, then the client wholesale-replacesS.messages, dropping 100 visible rows. - Fix: preserve the currently loaded renderable width for non-truncated recovery; never issue an oversized bounded limit that the backend will clamp before replacement. For widths above
_msgLimitMax, omit the limit/full-fetch or merge the bounded tail without replacing the loaded head. MakerefreshSession()use the same ceiling-aware helper. Add 100-row/full and 600-row/truncated mutation-sensitive tests.
- A fully loaded 100-row pane with
-
Current-master rebase and test accommodations are required. The PR conflicts in
static/sessions.jsandtests/test_cross_session_message_load_isolation.py. After compositional conflict resolution, untouched master tests still inspect the oldasync function loadSessionbody instead of_loadSessionOnce:test_inflight_stream_reuse.py: 1 failuretest_extension_session_hooks.py: 3 failures- Update those extractors while retaining both extension-hook/live-recovery and new coordinator harness dependencies.
Evidence
- Threat scan: CLEAN; all execution was sandboxed.
- Current-master focused integration: 73 passed, 1 failed; extension-hook slice: 13 passed, 3 failed (the four current-master static-shape blockers above).
- Direct sandboxed real-helper probe:
full100 = ...&msg_limit=30expanded600 = ...&msg_limit=600- delayed active-stream clear:
starts = 0
- Codex: SHIP ONLY WITH FIXES, reproduced both product defects.
- Opus: no additional blocker; it identified the same history-collapse paths as residuals. Reproduction makes them blocking.
- Branch CI is green on the old base, but does not exercise these current-master conflicts.
Please rebase, preserve the current-master extension/live-recovery behavior in the loader split, fix both product paths, and request re-gate.
…nt full-history dump on stream completion and SSE error recovery
c9d98d4 to
24f68d2
Compare
nesquena-hermes
left a comment
There was a problem hiding this comment.
Re-gate: runtime fixes converge, but the required SSE-restart regression is still false-green
I re-gated exact head 0d6cb6826543ff041b544b9a54c2c22bb2eeed9b against the July 20 gate certificate. The production changes now close the two runtime defects: terminal settlement defers the per-session resume until the finally block after ownership/state finalization, and the recovery URL helper omits unsafe 30-row/over-ceiling limits. The current-master extractor accommodations are also legitimate. Seven targeted sandboxed slices passed, 88 tests total.
One named gate requirement is still missing. tests/test_settled_session_window.py::test_deferred_owner_cleanup_waits_for_settlement_before_resuming_continuation does not drive the production done path or prove a session SSE restarts:
- it replaces
_clearActivePaneInflightIfOwner()with a fake that directly clearsS.activeStreamId; - it waits on a standalone promise unrelated to
_fetchSettledSessionMessageWindow(); - it manually calls a stub
_resumeSessionStreamAfterLiveChat()that only appends to an array; - it never runs
attachLiveStream,startSessionStream, theLIVE_STREAMSguards, or anEventSource; - it has no fetch-rejection/fallback case.
The test would stay green while bypassing the exact ownership/stream-registry interaction that caused permanent post-completion SSE silence. The adjacent source-order assertion is useful, but it is not the requested behavioral regression.
Required fix
Replace or supplement that test with a behavioral Node harness that drives the production terminal callback with a controllable delayed _fetchSettledSessionMessageWindow():
- Start with a live chat stream owned by the parent session and a completion that rotates to a continuation session.
- While the settled-window promise is pending, assert no per-session subscription restarts.
- Resolve the promise and assert terminal state is applied, chat ownership is cleared, and exactly one real/instrumented
startSessionStream(continuationSid)or continuationEventSourceis created through the production guards. - Repeat with the fetch rejected and prove the bounded local fallback is installed and the same continuation SSE restart occurs.
Please do not stub out the ownership transition or resume guard itself; observing calls/EventSource construction is fine, but the production guard path must run. This is test-only rework unless the faithful harness exposes another source/registry ordering race. The runtime fixes and the other prior findings do not need to be reopened.
🔬 Gate certification — RED ⛔Certified contributor head: What I ran
Blocking findings
Required next headFix all five ownership/recovery defects, keep the new production SSE test, and add executable regressions for each interleaving above. Then run a full exact-head re-gate from zero. This certifier did not merge, tag, deploy, or close the PR. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Re-gate: the older settlement still idles the replacement stream
I re-gated exact head 6e0005261529a83a9d229daeed69032b0eec8b21 against the prior exact-owner finding. The new guard at static/messages.js:5837-5844 correctly prevents an older done continuation from replacing the newer same-session stream's transcript, tool state, and S.activeStreamId. The earlier undo/retry, reconnect-window, scroll-follow, rotated-continuation, and renderable-count fixes also remain intact.
One ownership side effect is still outside that guard:
- When
_settlementStillOwnsPaneis false, the code setsisActiveSession=false, but control still reaches the unconditional_setActivePaneIdleIfOwner()atstatic/messages.js:6055. _setActivePaneIdleIfOwner()treats same-session pane identity as ownership (static/messages.js:2249-2254) and callssetBusy(false).- The real
setBusy(false)(static/ui.js:7910-7951) clears busy/composer/status state and may dequeue and send a queued message. An older settled stream can therefore markstream-newidle and drain its queue even though the new code correctly preservesS.activeStreamId === "stream-new"andLIVE_STREAMS[parentSid].streamId === "stream-new".
The new regression misses this. _done_continuation_stream_harness stubs setBusy as a no-op (tests/test_settled_session_window.py:436-452), seeds S.busy=true, but does not emit or assert busy/composer/status/queue-drain effects. Both success and rejection cases can stay green while the replacement stream is incorrectly idled.
Required fix
- Carry the exact
(streamId, source)ownership result through every active-pane terminal side effect. If the finishing settlement no longer owns the pane, it must not call the active-pane idle path, clear busy/composer/status state, set_queueDrainSid, or drain a queued item. Do not use same-SID pane identity as a substitute for stream ownership. - Make the production-function harness observe
setBusy,S.busy, status/composer clears, and queue-drain/send effects. For both settlement success and rejection, assert that a newer same-session owner remains busy and no queued item drains. Keep a control proving that a genuinely owning completion becomes idle.
Layer-1 threat scan is SUSPICIOUS solely on the fixed-source eval(extractFrom(...)) harness shape, so policy required a strict no-run review. I did not execute candidate code or tests. That scanner verdict is not itself the defect or a contributor-quality finding; the ownership control flow above is independently established by the current source. Please fix it and request another exact-head re-gate.
|
@nesquena-hermes Thanks for the detailed re-gate. The remaining exact-owner issue appears valid, but I’m no longer able to continue expanding this PR into the surrounding settlement and active-pane lifecycle. The current fix resolves the original full-transcript reload problem and has been working reliably in my daily use over the last week. However, I understand that the remaining ownership edge case prevents it from meeting the project’s merge gate. I’m therefore withdrawing from further work on this PR. The maintainers are welcome to reuse or build on any of the commits if useful. Thank you for the reviews thus far. 🫡 |
|
Thanks @akay64 for the substantial bounded-window and lifecycle work here. The branch fixed several real defects, and the review history preserves reusable fix shapes and tests. The latest exact head still allows an older settlement to idle a replacement same-session stream, and you have explicitly withdrawn from further expansion of this PR. Closing this implementation lane with credit, not closing the underlying issues. Maintainers may reuse the commits or test cases in a narrower replacement. |
Thinking Path
What Changed
done,stream_end, reconnect, and error-recovery paths bounded./retryand/undothrough the canonical bounded session reload path.Why It Matters
Long conversations could be fully loaded and rendered during normal lifecycle operations, producing:
The browser now keeps a bounded visible window while preserving the correct server-side message coordinate, pagination state, and total message count.
Verification
Focused automated verification:
tests/test_bounded_session_mutations_frontend.pytests/test_topbar_lazy_message_count.pytests/test_parallel_session_switch.pytests/test_cmd_idle_fallback.pytests/test_older_history_viewport_preservation.pycommands.js,sessions.js, andui.js.git diff --checkpassed.Manual UI verification covered with a session of 1500 messages:
Risks / Follow-ups
loaded of totaltop-bar semantics are preserved.Contract Routing
docs/CONTRACTS.mdandARCHITECTURE.md.Model Used
Provider: OpenAI, Model: Codex - GPT-5.6 Luna Max, Human super vision and review