Skip to content

feat(terminal): publish the replay's real start offset so mid-replay drops resume (issue #94) - #107

Merged
linletian merged 3 commits into
developfrom
feat/issue-94-replay-start-offset
Oct 9, 2026
Merged

linletian merged 3 commits into
developfrom
feat/issue-94-replay-start-offset

Conversation

@linletian

Copy link
Copy Markdown
Owner

Summary

Closes #94.

After #87 the streaming handshake replay (ttyHandshakeReplayBudget = 8 MB, ~128 × 64 KB frames) widened the "replay painted, sync not yet arrived" death window from one ≤64 KB frame to the full budget. The client's handshakeSynced latch deliberately blocks frame-by-frame cursor advancement until the closing sync (a stale since is silently clamped by RingBuffer.ReadSince, so accumulating from the request could publish a cursor over bytes never delivered) — so a connection dying mid-replay re-pulls everything from its old cursor, which under sustained network degradation does not converge.

Fix (the issue's minimal form, landed one step cheaper)

The catch-up loop in completeHandshake now brackets the replay with two sync frames:

Key design choice: reuse the sync type instead of a new frame type + capability. Any recipient provably parses "sync" already (caps=sync or the inferred explicit-since opt-in from #98 — and every catch-up request presents since by definition, so no catch-up client can be a pre-whitelist page that would paint the frame into the terminal). The shipped client's existing handler — adopt the ABSOLUTE offset, lift the latch — is precisely the wanted semantics, so the client needed zero code changes and even a never-refreshed v0.5.1 page benefits from a new daemon. "start":true exists for consumers that must know WHEN the replay ends (test harnesses, third-party clients); the UI deliberately ignores it, and S + Σ(frame bytes) always equals the closing offset.

Non-streaming paths — the tail replay (single ≤64 KB frame), since=-2 (zero binary frames by design, #86 untouched) and the caught-up-at-head exit — still send the closing sync alone. completeHandshake stays single-shot per connection.

Acceptance criteria (issue #94)

  • ✅ Mid-replay drop resumes from rendered bytes — TestHandleInstanceTTYWS_MidReplayResumeContinuesFromRenderedBytes + a new node --test case pinning the reconnect's since at the rendered cursor.
  • ✅ Stale since clamped server-side; the published start is the clamped value, never raw — TestHandleInstanceTTYWS_ReplayStartOffsetPublishesClampedStart.
  • ✅ completeHandshake remains single-shot per connection — unchanged; both call sites still guarded by !handshakeComplete.
  • ✅ since=-2 path unaffected (zero binary frames, closing sync alone) — TestHandleInstanceTTYWS_StartSyncOnlyOnTheStreamingCatchUpPath.
  • ✅ Contract in docs/API.md (TTY WS) + docs/ARCHITECTURE.md; mid-replay resume tests in internal/app.

Test notes

  • New server tests above; the 8 MB budget test now also pins start 0 + delivered budget == closing offset and that the re-request's start IS the published cursor.
  • Harness: dialHandshakeFrames stops only at the sync WITHOUT "start":true and fails if a start frame ever arrives after a replay chunk.
  • go test ./... and go test -race ./internal/app/ ./internal/ui/ pass; node --test total went 67 → 68 (CHANGELOG count bumped, TestTerminalStatusChangelogCount green).

…drops resume (issue #94)

After #87 the streaming handshake replay widened the "painted but not yet
synced" death window from one <=64KB frame to the full 8MB budget: the
client's handshakeSynced latch blocks frame-by-frame cursor advancement
until the closing sync, because RingBuffer.ReadSince silently clamps a
stale since and accumulating from the request could publish a cursor over
bytes never delivered. A drop mid-replay therefore re-pulled everything
from the old cursor — which does not converge under sustained network
degradation.

The catch-up loop now brackets the replay with TWO sync frames:
{"type":"sync","offset":S,"start":true} ahead of the FIRST binary
chunk — S = first chunk's next - len(body), so S and the chunk come from
the same ReadSince call (the clamped oldest-live value whenever clamping
happened, never the raw request, no new Manager surface) — and the
unchanged closing sync after the last chunk.

The frame reuses the "sync" type instead of adding a frame type plus a
capability gate: any recipient provably parses "sync" already (caps=sync
or the inferred explicit-since opt-in, #98; every catch-up request
presents since by definition), and the shipped client's existing handler
— adopt the ABSOLUTE offset, lift the latch — is exactly the wanted
semantics, so the client needed zero code changes and even a
never-refreshed v0.5.1 page benefits from a new daemon. "start":true
exists for consumers that must know when the replay ends; the UI ignores
it. Tail / since=-2 / caught-up-at-head paths still send the closing sync
alone; completeHandshake stays single-shot per connection.

Server: internal/app/app.go (completeHandshake). Tests:
TestHandleInstanceTTYWS_ReplayStartOffsetPublishesClampedStart,
TestHandleInstanceTTYWS_MidReplayResumeContinuesFromRenderedBytes,
TestHandleInstanceTTYWS_StartSyncOnlyOnTheStreamingCatchUpPath, the
extended 8MB budget test, and a new node --test case driving start sync
-> replay frames -> mid-replay drop -> reconnect since. Harness:
dialHandshakeFrames stops only at the sync WITHOUT "start":true and
fails on a start frame arriving after any chunk. Contract documented in
docs/API.md (TTY WS) and docs/ARCHITECTURE.md.
@linletian

Copy link
Copy Markdown
Owner Author

独立评审(Qwen3.8-Flash-Next)

结论:LGTM,两处 minor。 核心设计经得起推敲:S = 首块的 next − len(body) 与首块同源同一次 ReadSince,钳制的值天然不会回显 raw since;start sync 先于 chunk 落线,钳制/未钳制两种情况下「死在 start sync 与 chunk 之间 → 重连 since=S → 重放恰为未送达字节」均正确;客户端零代码改动的论证成立(catch-up ⇒ since 呈现 ⇒ syncCap 推断成立,#98 泄漏类不适用)。本地独立验证:go build、gofmt、go test ./internal/app/、go test -race ./internal/app/ ./internal/ui/ 全绿,node --test 68/68 且与 CHANGELOG 计数守卫一致。

Minor 1 — app.go:2792 的 if syncCap 在此路径恒真(防御性死代码)。 能进入 catch-up else 分支必有 since >= 0,即 since 参数存在,而 sincePresent 已强制 syncCap = true(app.go:2440-2441)。另外 startPublished = true 在守卫之外无条件置位(:2760),若该守卫未来某天可达为假,变量语义将与线上实际不符。按本仓库对不可达防御路径显式标注的惯例(如 "unreachable through normal flows"),建议去掉守卫或补一行注释点明。

Minor 2 — 防御性 break 的倒退角落(:2804-2811 与 :2793 的交互)。 若 misbehaving kind 返回 next <= cursor 且 body 非空,则 S = next − len(body) 会低于客户端自己发出的 since:start sync 让客户端游标瞬时倒退,若恰死在 start sync 与首 chunk 之间,重连将以 since=S 重复渲染 [S, since)。有界、可自愈,且旧行为(整段重拉)本来更糟,不构成阻塞;但块内注释「corrected by that same final absolute value」只覆盖了 closing sync 的纠偏,未点出这一瞬时倒退角落,建议补一句。

@linletian

Copy link
Copy Markdown
Owner Author

Independent review — PR #107 (issue #94)

Evidence (run on this branch): go test ./... green · node --test internal/ui/testdata/terminal_status.test.mjs 68/68 · gofmt -l internal/ clean.

What I checked and believe correct:

  • S = next - len(body) really is the clamped start: RingBuffer.ReadSince returns start + avail with a body of exactly avail bytes (ringbuffer.go:155-163), so S equals the requested since when nothing clamped and the oldest live byte when it did — never the raw request.
  • startPublished must be separate from first, and is: the FIX-F branch clears first before any chunk exists (app.go:2745), so gating on first would have silently skipped the start frame there. Correct call.
  • Both completeHandshake call sites are still guarded by !handshakeComplete (single-shot), -2 / tail / caught-up-at-head emit no start frame, and the if syncCap gate can't withhold it from a client that should get it.
  • The "zero client change" claim holds against the actual [BUG] WS handshake replays the last 64KB on every reconnect, duplicating terminal output #87-era handler (0bf3b36: adopt absolute offset + lift latch + return) — I read that diff rather than trusting the PR description.

Findings

1. docs/API.md still documents the old single-sync contract in three places this PR didn't touch. (main finding)

  • Handshake Protocol step 4→5 (line 661): "Server sends … sync … the end offset of that replay" — a third-party client implementing the numbered protocol never learns a start frame precedes the replay, which is precisely the consumer start:true was added for.
  • Message Types → Sync (line 690): "sent after the handshake replay" and "replay frames received BEFORE the sync never touch the cursor — the sync publishes the authoritative end of the whole replay in one step" — both now false on the streaming path, and they directly contradict the new paragraph at line 609 and the updated comment in index.html.
  • Example Flow diagram (line 744): one sync after the binary replay, no start frame.
  • Same singular-tense drift at line 531 ("sync at the end of the last chunk") and line 578 ("The sync offset is the end of the last chunk").

2. A catch-up-path chunk that ships with NO start frame — the since > head degrade path. (contract gap)
app.go:2709-2722 writes the tail as a binary chunk from inside the catch-up branch and then breaks, so it only ever gets the closing sync. But API.md:630-632 enumerates the exceptions as paths that "emit no streamed chunk", which this is not. It's reachable and pinned today by TestHandleInstanceTTYWS_CursorAheadOfHeadFallsBackToTail (since=999). Either emit {"start":true} there too (S = head - len(tail)) or add it as a fourth exception — note dialHandshakeStart can only fail on a start frame that arrives late, never on a missing one, so no current test will catch the inconsistency either way.

3. "S + Σ(replay frame bytes) always equals the closing offset" is not always. (accuracy)
Stated absolutely at API.md:629, ARCHITECTURE.md:116 and in CHANGELOG. It breaks when the ring evicts past cursor between two loop iterations: the next ReadSince clamps to the new oldest-live byte, so that chunk starts above cursor, the sum lands short, and the closing sync jumps the gap. Rare (needs a full ring rollover inside an 8 MB replay) and harmless — the skipped bytes are undeliverable anyway, which is the existing #87 rule — but "always" should be softened or scoped.

4. Stale comment. tty_ws_test.go:1514: "sync is written exactly once, in completeHandshake" and "a second one here would advertise a cursor past bytes the client never received". It's now written twice per handshake. The assertion itself (no sync after the handshake) is still right; the rationale isn't.

5. Nit. app.go:2792 — the if syncCap around the start frame is unreachable-false: this branch requires since >= 0, which implies the query key since is present, which is already half of syncCap. Harmless, but it reads as if the start frame is optional.

6. Accepted edge, no change requested. Lifting the latch at chunk 1 means the cursor now counts replay bytes the decoder had buffered but never painted: a drop that lands between a frame ending mid-UTF-8-sequence and the next one discards those 1-3 lead bytes (resetTTYOutputDecoder nulls the decoder in ws.onclose) while the cursor has already stepped past them, so the character renders as U+FFFD instead of being re-delivered by the full re-pull. Pre-existing on the live path, cosmetic, and the split character is unrecoverable either way — flagging only so it's a recorded trade-off rather than an unnoticed one.

— MiMo-V2.6-Flash, independent reviewer

…d degrade, doc contract sweep (issue #94)

Two independent reviews (Qwen3.8-Flash-Next, MiMo-V2.6-Flash); all
findings verified accurate before fixing except one wrong file cite.

R2#2 (main): the cursor>head degrade wrote its tail binary chunk from
inside the catch-up branch WITHOUT a start frame, contradicting the
documented "exceptions emit no streamed chunk" rule. Now emits
{"type":"sync","offset":head-len(tail),"start":true} ahead of the
tail, so EVERY replay addressed to a cursor-bearing client carries a
start frame; only the cursor-less first-connect tail goes without.
Pinned by the extended TestHandleInstanceTTYWS_CursorAheadOfHeadFallsBackToTail.

R1M1/R2#5: the start-frame emit is now an emitStartSync closure whose
comment states syncCap is PROVABLY true at both call sites (catch-up
requires an explicit since, already the inferred opt-in half) and the
guard stays as defensive depth against future inference changes.

R1M2: documented the defensive next<=cursor corner where S dips below
the client's own since (bounded, self-healing re-render of [S, since)).

R2#3: softened "S + sum(frames) always equals the closing offset" in
API.md and CHANGELOG — a full ring rollover mid-replay clamps a later
chunk above the running cursor and the closing sync jumps the gap
(truthfully). ARCHITECTURE.md carried no such claim (wrong cite).

R2#1: API.md's numbered handshake protocol, Message Types -> Sync and
the example flow diagram now describe the start/closing sync pair; two
singular phrasings ("with sync at the end", "The sync offset is")
now name the CLOSING sync.

R2#4: tty_ws_test.go's overflow-teardown comment no longer claims sync
is written exactly once (assertion — no sync on the LIVE stream —
unchanged).

R2#6: recorded the accepted UTF-8 split trade-off in the client's latch
comment (pre-existing on the live path, cosmetic, unrecoverable either
way).
@linletian

Copy link
Copy Markdown
Owner Author

评审回复与修复(commit 92e99b4)

两条评审逐条核实后:7 条中 6 条准确并已修复,1 条部分不准(文件引用错误)。

Review 1(Qwen3.8-Flash-Next)

  • Minor 1(syncCap 恒真) ✅ 准确。已重构为 emitStartSync 闭包,注释明确标注:两个调用点都在 catch-up 分支内,必有显式 since ⇒ syncCap 恒真;守卫保留为防御纵深(防止未来推断逻辑变更导致泄漏),并说明了 startPublished 无条件置位与此语义一致。
  • Minor 2(防御路径瞬时倒退) ✅ 准确。已在块内注释记录:next <= cursor 时 S 可能低于客户端自己的 since,死在起始帧与首块之间会以回退游标重渲染 [S, since) —— 有界、可自愈,仍严格优于改前的整段重拉。

Review 2(MiMo-V2.6-Flash)

  1. API.md 旧单 sync 契约三处 ✅ 准确。编号协议步骤(新增第 4 步起始帧,后续步骤重编号)、Message Types → Sync(重写为双帧 + 闩锁语义 + closing 绝对权威)、示例流程图(补起始帧行)均已更新;:531、:578 两处单数表述改为命名 closing sync。
  2. cursor>head 降级路径缺起始帧(main finding) ✅ 准确,采纳"发射起始帧"方案而非加文档例外:降级路径现在在写 tail 前发 {"type":"sync","offset":head-len(tail),"start":true},凡面向持游标客户端的重放都带起始帧,例外只剩无游标首连 tail。[BUG] WS handshake replays the last 64KB on every reconnect, duplicating terminal output #87 时代客户端最终状态不变(closing sync 仍钉 head),中间状态更诚实。TestHandleInstanceTTYWS_CursorAheadOfHeadFallsBackToTail 已扩展钉住 S=0(head 10 − len 10,harness 以 -1 区分"无起始帧")。
  3. "S + Σ 恒等于 closing" ✅ 对 API.md / CHANGELOG 准确,已改为"除非环形缓冲区在重放中途整圈翻滚、逐出未发送字节 —— 后续 chunk 钳到新的 oldestLive,和短缺,closing sync(绝对权威)跳过缺口"。⚠️ ARCHITECTURE.md 引用有误:该文件并无此断言(已 grep 确认 Σ/always equals 无匹配),其 "re-pins the end absolutely" 表述本就兼容漂移,未改动。
  4. tty_ws_test.go:1514 过时注释 ✅ 准确。断言(live 流上不得出现 sync)不变,理由已改写为"握手 sync 帧(feat(terminal): publish the replay's real start offset so clients can advance the cursor frame-by-frame #94 起最多两帧)全部写在 completeHandshake 内、SubscribeOutput 之前"。
  5. syncCap 恒真 ✅ 同 R1M1,一并修复。
  6. UTF-8 跨帧截断 ✅ 准确且接受现状。已按"记录在案"补进客户端闩锁注释:跨帧断连丢弃 1-3 个前导字节(解码器随 onclose 重置)而游标已越过,字符渲染为 U+FFFD —— 与 live 路径自始相同的权衡,装饰性,且该字符两种情况下都不可恢复。

验证:gofmt -l internal/ 干净,go build / go vet 通过,go test ./... 全绿,go test -race ./internal/app/ ./internal/ui/ 通过,node --test 68/68,CHANGELOG 计数守卫通过。

@linletian

Copy link
Copy Markdown
Owner Author

第二轮评审(Qwen3.8-Flash-Next)— commit 92e99b4

结论:两条 minor 均按所述落实,降级路径新行为正确,全量验证独立复跑绿。新发现 1 处 minor(文档陈旧引用)+ 2 处可选 nit,不阻塞合并。

本轮独立验证:

  • 我提的 Minor 1/2 修复到位:emitStartSync 闭包 + "syncCap PROVABLY true" 注释 + 防御纵深理由;防御 break 角落的瞬时倒退 [S, since) 已入注释。
  • R2#2 行为变更逐行核过:S = head − len(tail) 与 tail 出自同一次 Tail consult,恒等自洽且非负(len(tail) ≤ used ≤ head);空 tail 不发帧,与「无 chunk 无起始帧」一致;degrade 后紧跟 break,每握手至多一个起始帧,与 streaming 循环的 startPublished 互斥无漏。客户端语义闭环:adopt S → tail 帧计数到 head → closing 重钉;死在起始帧与 tail 之间 → since=S 重放恰为未送达字节。S=0 被测试钉住且与 harness 的 -1「无帧」哨兵可区分。
  • 作者对 MiMo chore: add CI workflow and comprehensive tests #3 的反驳属实:grep 'Σ|always equals' 在 ARCHITECTURE.md 新旧两版均 0 命中,系引用错文件。
  • 全套复跑:go vet、gofmt 干净、go test ./... 全绿、go test -race ./internal/app/ ./internal/ui/ 通过、node --test 68/68、计数守卫通过。

新发现(minor,文档)——docs/API.md:680:步骤 4 写着 "Same opt-in as the closing sync in step 5",但本轮重编号后 closing sync 是步骤 6(步骤 5 现在是二进制重放帧)。陈旧交叉引用,恰是编号协议文档最不该错的地方——改成 step 6 即可。

可选 nit(不阻塞):① index.html UTF-8 注释结尾 "unrecoverable either way" 指代悬空——若读作「#94 前后两种情况」则与前句「pre-#94 full re-pull redelivered it」直接矛盾,作者本意应是「(replay 路径、live 路径)两条都已越过前导字节的路径」,建议点名。② TestHandleInstanceTTYWS_StartSyncOnlyOnTheStreamingCatchUpPath 的名字/注释仍说『streaming 才发起始帧』,而本轮契约已改为『面向持游标客户端的每次重放都发』(degrade 单帧也发)——各断言仍准确,但命名宜换成 cursor-bearing 措辞以免下位读者被误导。

…gling referent, boundary-test rename (issue #94)

All three round-2 findings verified accurate:

- API.md's step 4 referenced "the closing sync in step 5" after the
  renumbering moved the closing sync to step 6 (step 5 is the binary
  replay) — a stale cross-reference in exactly the numbered-protocol
  doc that can least afford one.
- The client latch comment's "unrecoverable either way" had a
  dangling referent: read as "pre/post-#94" it contradicts the
  preceding clause ("the pre-#94 full re-pull redelivered it"). Now
  names the two paths that have already stepped the cursor past the
  lead bytes — the replay path post-#94 and the live path.
- The boundary test's name/comment still said "only STREAMING
  replays announce a start", but the contract since the R2#2 fix is
  "every replay chunk addressed to a cursor-bearing client" (the
  single-frame degrade included). Renamed to
  TestHandleInstanceTTYWS_StartSyncOnlyForChunkedReplaysWithACursor
  with the comment spelling out both sides and the tail-path failure
  message now naming the cursor-less reason. CHANGELOG reference
  updated.
@linletian

Copy link
Copy Markdown
Owner Author

第二轮评审回复与修复(commit ab02724)

三条新发现逐条核实:全部准确,均已修复。

  1. API.md:680 步骤交叉引用陈旧 ✅ 准确。重编号后步骤 4 仍写 "the closing sync in step 5",但 closing sync 已是步骤 6(步骤 5 是二进制重放)。已改为 step 6。已同时复查全文其它步骤交叉引用(步骤 6 内 "the start announcement of step 4" 正确)。

  2. index.html "unrecoverable either way" 指代悬空 ✅ 准确。读作「feat(terminal): publish the replay's real start offset so clients can advance the cursor frame-by-frame #94 前后两种行为」确实与前句「pre-feat(terminal): publish the replay's real start offset so clients can advance the cursor frame-by-frame #94 full re-pull redelivered it」矛盾。已点名本意:两条已越过前导字节的路径 —— feat(terminal): publish the replay's real start offset so clients can advance the cursor frame-by-frame #94 后的 replay 路径与自始如此的 live 路径,字符在两者上均不可恢复。

  3. 边界测试命名落后于契约 ✅ 准确。R2#2 修复后契约已是「面向持游标客户端的每个重放块都前置起始帧」(degrade 单帧也发),原名 ...StartSyncOnlyOnTheStreamingCatchUpPath 会误导。已重命名为 TestHandleInstanceTTYWS_StartSyncOnlyForChunkedReplaysWithACursor,文档注释重写为双侧表述(正向:streaming 由 resume/预算测试钉、degrade 单帧由 CursorAheadOfHeadFallsBackToTail 钉;本测试钉三条负向路径:无游标 tail / -2 / 已追平),tail 断言的失败信息改为点名「无游标可推进」这一真正原因。CHANGELOG 中的引用已同步。

同时确认:本轮对 R2#2 行为变更的逐行复核结论(S 与 tail 同源同次 consult、空 tail 不发帧、与 startPublished 互斥)与我方实现一致;对 MiMo #3 引用错文件的确认也一致。

验证:gofmt -l internal/ 干净,go build 通过,go test ./... 全绿,重命名后的两个边界测试单独复跑通过。

(流程备注:本次提交最初落在分离 HEAD 上 —— 工作期间检出状态被切换过;已将分支快进到该提交并推送,无历史改动。)

@linletian
linletian merged commit 4391ba5 into develop Oct 9, 2026
4 checks passed
@linletian linletian mentioned this pull request Oct 9, 2026
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.

1 participant