The detached replay buffer is capped, and I documented that it was not - #39
Merged
Conversation
I wrote the retention docs in PR 35. They claimed the replay buffer had no cap of its own, that a runaway remote shell would retain roughly 17 MB across a 15 minute window, and that the detached TTL was therefore the only backstop against such a process. Measured directly, all three are wrong. A detached session is never acknowledged. Its reader waits for buffer space that never frees, so it stops at MAX_BUFFERED_BYTES and the remote process then blocks writing to its own PTY. A runaway producer pins at exactly 4194304 bytes and stays there across every sample; send_next stops at the same number, which shows the reader stopped reading rather than the buffer merely stopping growing. So retention is bounded per session no matter how long the window is, and in aggregate by the server session cap: 256 MiB at the default of 64 sessions. Backpressure is the backstop against a runaway process. The TTL bounds how long a session lives, not how much it holds. This is not only an accuracy fix. "No cap" was the stated reason not to raise the detached TTL further, and that reason does not hold. The measurement is kept as a test, with a positive control: a producer that silently produced nothing would satisfy every bound while proving nothing, so the first sample must be above zero before any bound is asserted. The test also asserts the buffer SETTLES rather than merely staying under the cap, because a buffer still climbing under the limit would pass a bound-only check. I found this by reading MAX_BUFFERED_BYTES rather than re-reading my own summary of it. 195 lib tests green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I got wrong
I wrote the retention docs in #35. They claimed three things:
Measured directly, all three are wrong.
What actually happens
A detached session is never acknowledged, so
drop_acked_chunksnever frees anything. The reader callswait_for_buffer_space, which parks it oncebuffered_bytesreachesMAX_BUFFERED_BYTES. The remote process then blocks writing to its own PTY.Driving an endless producer into a never-attached session:
It pins at exactly 4 MiB.
send_nextstops at the same number, which shows the reader stopped reading rather than the buffer merely stopping growing — that is backpressure working, not a coincidence of timing.Retention is bounded per session regardless of window length, and in aggregate by the server session cap: 256 MiB at the default of 64 sessions.
Why this matters beyond accuracy
"No cap" was the stated reason not to raise the detached TTL further. That reason does not hold. Backpressure is the backstop against a runaway remote process; the TTL bounds how long a session lives, not how much it holds.
I am not proposing a TTL change here. I am removing a blocker that was never real, so that decision can be made on its merits.
The test
The measurement is kept rather than thrown away, with two controls:
How I found it
By reading
MAX_BUFFERED_BYTESinstead of re-reading my own summary of it. Same class of error this repo has been hitting all night — trusting a summary over its source — except the summary was mine.Corrected in
src/config.rs,README.md, andCHANGELOG.md. Each correction states what the old claim was, so the record shows the change rather than quietly reading as though it was always right.Tests
195 lib tests green. Changed files pass rustfmt. No behaviour change — docs and one test.