Skip to content

fix: Shared-shell replay shifts 1 MiB on every output chunk - #1111

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-8f3c2c04
Closed

fix: Shared-shell replay shifts 1 MiB on every output chunk#1111
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-8f3c2c04

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • Keep a logical start index for the shared-shell replay buffer instead of shifting the retained 1 MiB window after every small PTY output chunk.
  • Reclaim dropped storage only after a full replay window has accumulated, while preserving baseOffset and nextOffset semantics.
  • Account for the logical prefix in chunked snapshots and collaboration protocol redaction/replacement.
  • Add a regression test that crosses multiple batched compactions with 32 KiB chunks and verifies exact retained bytes, offsets, reset behavior, redaction, and chunked replay.
  • Add a 256 MiB sustained-small-chunk benchmark.

Why this is high-value

The Unix PTY producer reads up to 32 KiB at a time. Once the replay buffer reached 1 MiB, each ordinary output callback previously copied the entire retained window while holding the shell mutex. That meant about 32 MiB copied for each 1 MiB of continued output, delaying snapshots, browser input, lifecycle operations, and collaboration state during verbose builds, tests, searches, and log streams.

The logical prefix makes eviction constant-time per chunk and performs one roughly 1 MiB compaction per additional 1 MiB of output instead of one per 32 KiB chunk: 32x fewer replay-window shifts in the steady producer case. The end-to-end append benchmark improved from a median 520.8 ms (515 MB/s) to 425.3 ms (631 MB/s) for 256 MiB on an i9-14900K, while replay storage remains bounded to roughly two windows between compactions.

Validation

  • go test ./cmd -run '^TestServeShellReplay(ResetAndExit|CompactsDroppedPrefixesInBatches)$' -count=1
  • go test -race ./cmd -run '^TestServeShellReplay(ResetAndExit|CompactsDroppedPrefixesInBatches)$' -count=1
  • go test ./cmd -run '^$' -bench '^BenchmarkServeShellReplaySmallChunks$' -benchtime=1x -benchmem -count=3
    • Before median: 520768260 ns/op, 515.46 MB/s
    • After median: 425333161 ns/op, 631.12 MB/s
  • go build ./...
  • go vet ./...
  • go test ./... ran successfully for the changed cmd package and all other root-module packages except an unrelated existing generated-frontend budget failure in internal/serveui: app.js gzip 138137 > 135000 and app.css gzip 33383 > 33000. This branch changes no frontend source or bundle-budget code.

@SamSaffron

Copy link
Copy Markdown
Owner

Closing at the maintainer’s request in favor of a consolidated implementation on the current working tree (not merged or pushed yet). Retained as an 8/10 priority: batch shared-shell replay compaction rather than copying 1 MiB for every small PTY chunk. Includes offset/redaction/chunked replay coverage and an additional large-write-after-compaction regression. Full Go suite, relevant race tests, build, vet, and diff checks pass.

@SamSaffron SamSaffron closed this Sep 5, 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.

2 participants