Spill scrollback beyond the memory window to a per-session disk cache - #8
Merged
Conversation
`ScrollbackBuffer` kept a capped ring and simply lost anything older, so the in-memory cap was also the maximum scroll depth. It now keeps the same modest window in memory and hands evicted lines to an `IScrollbackSpill`, and serves a range from either half transparently — the foundation the windowed output feed needs, since appending to a `MarkupControl` re-parses its whole content and the view will soon be fed only the rows it draws. - `StyledLineCodec` — length-prefixed binary payload for a `StyledLine`: spans, styles, interactions and the rule colour, losslessly. Not the Tui's Spectre markup, which Core must not depend on and which cannot express a palette index or a span's interaction anyway. - `FileScrollbackSpill` — segmented store: 16-byte header, then records framed with a length prefix and a CRC-32, so a torn or externally damaged record is detected and read back blank instead of returned as garbage. Every record's offset is held in memory, so lines i..j cost one `pread` of exactly their byte span, flat in depth. Bounded by lines and bytes together, reclaimed by unlinking the oldest segment rather than rewriting anything. - Lifetime: an ephemeral per-session, per-window cache under `$XDG_CACHE_HOME` (`%LOCALAPPDATA%`, `~/Library/Caches`), created on the first eviction, deleted on dispose, and purged of whatever a crash left behind on the first store of a new process. A held `.lock` file keeps two instances off each other's files and tells a live store from an abandoned one. This is not the session log — `PlainTextLogSink`/`HtmlLogSink` stay opt-in, formatted and kept. - Failure: no disk error reaches the caller. The first one logs once through the session's `ILogger`, deletes the cache and degrades to memory-only for the rest of the session; not one live line is lost. - `ScrollbackBuffer` indices are now absolute, so a scroll position keeps meaning the same line, `GetRange` is capped at `MaxRangeLines` so no caller can materialise the whole history by accident, and the in-memory ring is a growable circular array instead of a `LinkedList` — the old one allocated a node per line and walked from the head, so serving the newest 40 of 20,000 lines touched 19,960 nodes. Measured: 22 ns/line memory-only, 820 ns/line spilling (1.2M lines/s, ~60% of it the encode); a 40-line range is 0.3 us from memory and 47 us from disk at 10k, 100k or 190k deep alike; a full 4,096-line page 1.8 ms. Core 474 tests green (was 424); the other four suites unchanged and green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
WalkthroughChangesThe PR adds configurable per-session file-backed scrollback. Scrollback spill storage
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SessionManager
participant WorldSession
participant ScrollbackBuffer
participant FileScrollbackSpill
SessionManager->>WorldSession: Open with spill options
WorldSession->>FileScrollbackSpill: Create enabled spill
WorldSession->>ScrollbackBuffer: Initialize with spill
ScrollbackBuffer->>FileScrollbackSpill: Store evicted lines
ScrollbackBuffer->>FileScrollbackSpill: Read historical ranges
WorldSession->>ScrollbackBuffer: Dispose on session close
ScrollbackBuffer->>FileScrollbackSpill: Remove cache
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/SharpMUTerm.Core/Session/WorldSession.cs (3)
521-532: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDispose
_telnetbeforeScrollbackor unsubscribeOnDisconnectedfirst.TelnetSession.DisposeAsync()callsDisconnectAsync(), which raisesDisconnectedduring teardown, andWorldSessionstill handles that event by appending toScrollback. WithScrollback.Dispose()running first, a final disconnect can write into an already-disposed spill cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SharpMUTerm.Core/Session/WorldSession.cs` around lines 521 - 532, Update WorldSession.DisposeAsync to dispose _telnet before disposing Scrollback, or unsubscribe the OnDisconnected handler before Scrollback.Dispose. Ensure any Disconnected event raised during TelnetSession.DisposeAsync cannot append to the already-disposed scrollback cache, while preserving the existing log detachment and cleanup behavior.
60-72: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThread
AppConfiguration.ScrollbackSpillthrough session creation.WorldSessiondefaults tonew ScrollbackSpillOptions(), andSharpMUTermApp.OpenSession(...)still passes only lines/text/input/telnet. That makes scrollback spill disk-backed for every session and bypasses the user-facingScrollbackSpillsetting.src/SharpMUTerm.Core/Session/WorldSession.cs:60-72,src/SharpMUTerm.Tui/SharpMUTermApp.cs:853-866🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SharpMUTerm.Core/Session/WorldSession.cs` around lines 60 - 72, Thread the configured AppConfiguration.ScrollbackSpill through SharpMUTermApp.OpenSession into the WorldSession constructor, instead of relying on WorldSession’s default ScrollbackSpillOptions. Update the session-creation call and constructor argument flow so each session uses the user-facing spill configuration.
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDispose
_telnetbefore releasingScrollback.WorldSession.DisposeAsync()still leavesOnDisconnectedsubscribed, so a disconnect raised during_telnet.DisposeAsync()can callPrintSystem()after the scrollback spill has already been disposed. Add a_disposedguard inScrollbackBufferas defense in depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SharpMUTerm.Core/Session/WorldSession.cs` at line 1, Update WorldSession.DisposeAsync() to dispose _telnet before releasing Scrollback, ensuring OnDisconnected cannot access an already-disposed scrollback during telnet shutdown. Add a _disposed guard to ScrollbackBuffer so PrintSystem() and related operations safely no-op after disposal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SharpMUTerm.Core/Text/FileScrollbackSpill.cs`:
- Around line 300-339: The Read method holds _gate during disk I/O and decoding,
blocking Append; resolve and snapshot the required segment byte spans while
locked, then release _gate before calling ReadFromSegment or otherwise
performing reads. Preserve the existing range clamping, corruption handling,
ordering, and empty-fill behavior, ensuring the captured segment data remains
valid while the unlocked read executes.
- Around line 632-638: Refactor FillEmpty to accept a single count parameter and
add exactly that many StyledLine.Empty entries. Update callers in Read and
ReadFromSegment to pass end - index, want, and want - done respectively,
preserving their existing output behavior while removing the ambiguous from/to
index-based API.
In `@src/SharpMUTerm.Core/Text/ScrollbackBuffer.cs`:
- Around line 250-287: Add disposed-state tracking to TextScrollbackBuffer and
have Dispose mark the buffer disposed while releasing and clearing the spill
resource. Guard public operations such as Append, Clear, and index/query methods
before they can reach _spill, throwing ObjectDisposedException for post-dispose
use; ensure repeated Dispose calls remain safe.
In `@src/SharpMUTerm.Core/Text/ScrollbackSpillOptions.cs`:
- Around line 22-27: Update the XML summary for MaxLines to describe the lower
scroll-depth bound as approximately MaxLines minus one segment’s worth of lines,
rather than subtracting SegmentMegabytes. Keep the existing explanation about
dropping the oldest segment and reclaiming space without rewriting the file.
In `@tests/SharpMUTerm.Core.Tests/Text/ScrollbackSpillTests.cs`:
- Around line 564-639: Add a bounded timeout to
AppendingWhileReading_IsSafeAndNeverServesATornRange so deadlocks or livelocks
fail the test promptly. Apply the timeout to the writer and reader completion
waits, and ensure timeout failures clearly identify which concurrent operation
did not finish while preserving the existing assertions.
- Around line 42-70: Add the test framework’s [NotInParallel] attribute to
DefaultRoot_IsACacheLocationAndHonoursXdgCacheHome, which mutates the
process-wide XDG_CACHE_HOME environment variable; leave the test logic
unchanged.
In `@tests/SharpMUTerm.Core.Tests/Text/StyledLineCodecTests.cs`:
- Around line 130-141: Replace the catch-all flag around StyledLineCodec.Decode
with TUnit’s direct throw assertion, matching the existing pattern in
ScrollbackSpillTests. Verify the truncated payload’s actual exception type and
assert that specific type; use ThrowsException only if the codec can
legitimately produce multiple types.
---
Outside diff comments:
In `@src/SharpMUTerm.Core/Session/WorldSession.cs`:
- Around line 521-532: Update WorldSession.DisposeAsync to dispose _telnet
before disposing Scrollback, or unsubscribe the OnDisconnected handler before
Scrollback.Dispose. Ensure any Disconnected event raised during
TelnetSession.DisposeAsync cannot append to the already-disposed scrollback
cache, while preserving the existing log detachment and cleanup behavior.
- Around line 60-72: Thread the configured AppConfiguration.ScrollbackSpill
through SharpMUTermApp.OpenSession into the WorldSession constructor, instead of
relying on WorldSession’s default ScrollbackSpillOptions. Update the
session-creation call and constructor argument flow so each session uses the
user-facing spill configuration.
- Line 1: Update WorldSession.DisposeAsync() to dispose _telnet before releasing
Scrollback, ensuring OnDisconnected cannot access an already-disposed scrollback
during telnet shutdown. Add a _disposed guard to ScrollbackBuffer so
PrintSystem() and related operations safely no-op after disposal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18e6332a-782f-4db7-b581-d3d25db5821f
📒 Files selected for processing (13)
CLAUDE.mdsrc/SharpMUTerm.Core/Configuration/AppConfiguration.cssrc/SharpMUTerm.Core/Session/SessionManager.cssrc/SharpMUTerm.Core/Session/WorldSession.cssrc/SharpMUTerm.Core/Text/FileScrollbackSpill.cssrc/SharpMUTerm.Core/Text/IScrollbackSpill.cssrc/SharpMUTerm.Core/Text/ScrollbackBuffer.cssrc/SharpMUTerm.Core/Text/ScrollbackSpillOptions.cssrc/SharpMUTerm.Core/Text/StyledLineCodec.cstests/SharpMUTerm.Core.Tests/Session/WorldSessionScrollbackTests.cstests/SharpMUTerm.Core.Tests/Text/ScrollbackBufferTests.cstests/SharpMUTerm.Core.Tests/Text/ScrollbackSpillTests.cstests/SharpMUTerm.Core.Tests/Text/StyledLineCodecTests.cs
Comment on lines
+300
to
+339
| public void Read(long start, int count, List<StyledLine> into) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(into); | ||
| if (count <= 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| lock (_gate) | ||
| { | ||
| if (!_healthy || _disposed) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var index = Math.Max(start, _first); | ||
| var end = Math.Min(start + count, _end); | ||
| while (index < end) | ||
| { | ||
| // A fault inside the loop (below) leaves nothing readable; the range still has to come | ||
| // back the length the caller's clamp implies, or a paged view's rows shift under it. | ||
| var segment = _healthy ? FindSegment(index) : null; | ||
| if (segment is null) | ||
| { | ||
| if (_healthy) | ||
| { | ||
| NoteCorruption($"no segment holds line {index}"); | ||
| } | ||
|
|
||
| FillEmpty(index, end, into); | ||
| return; | ||
| } | ||
|
|
||
| var local = (int)(index - segment.FirstIndex); | ||
| var want = (int)Math.Min(end - index, segment.Offsets.Count - local); | ||
| ReadFromSegment(segment, local, want, into); | ||
| index += want; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
_gate is held across the disk reads, so a page-down blocks the inbound line path.
Read performs up to MaxRangeLines worth of pread + decode while holding the same lock Append needs, so a UI paging read can stall the network/parser thread for the duration of the I/O. It is bounded and correct today; if that latency ever shows up, the usual shape is to resolve the byte spans under the lock and do the pread/decode outside it (segment handles are already immutable once created, only EnforceBounds retires them).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SharpMUTerm.Core/Text/FileScrollbackSpill.cs` around lines 300 - 339, The
Read method holds _gate during disk I/O and decoding, blocking Append; resolve
and snapshot the required segment byte spans while locked, then release _gate
before calling ReadFromSegment or otherwise performing reads. Preserve the
existing range clamping, corruption handling, ordering, and empty-fill behavior,
ensuring the captured segment data remains valid while the unlocked read
executes.
Comment on lines
+632
to
+638
| private static void FillEmpty(long from, long to, List<StyledLine> into) | ||
| { | ||
| for (var i = from; i < to; i++) | ||
| { | ||
| into.Add(StyledLine.Empty); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
FillEmpty is called with two different meanings for the same parameters.
Read (Line 329) passes absolute line indices, while ReadFromSegment (Lines 578, 585, 620) passes counts. Both happen to work because only the difference is used, but the helper's from/to names invite an off-by-N the next time either caller changes.
♻️ Proposed refactor — make it count-based and drop the index-flavoured call
- private static void FillEmpty(long from, long to, List<StyledLine> into)
+ /// <summary>Appends <paramref name="count"/> blank lines so a clamped range keeps its length.</summary>
+ private static void FillEmpty(long count, List<StyledLine> into)
{
- for (var i = from; i < to; i++)
+ for (var i = 0L; i < count; i++)
{
into.Add(StyledLine.Empty);
}
}Callers become FillEmpty(end - index, into) (Line 329), FillEmpty(want, into) (Lines 578, 585) and FillEmpty(want - done, into) (Line 620).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SharpMUTerm.Core/Text/FileScrollbackSpill.cs` around lines 632 - 638,
Refactor FillEmpty to accept a single count parameter and add exactly that many
StyledLine.Empty entries. Update callers in Read and ReadFromSegment to pass end
- index, want, and want - done respectively, preserving their existing output
behavior while removing the ambiguous from/to index-based API.
Comment on lines
+250
to
+287
| /// <summary>Removes every line, in memory and on disk. Absolute indices restart at zero.</summary> | ||
| public void Clear() | ||
| { | ||
| lock (_gate) | ||
| { | ||
| return _lines.ToArray(); | ||
| Array.Clear(_ring); | ||
| _head = 0; | ||
| _count = 0; | ||
| _memoryStart = 0; | ||
| _total = 0; | ||
| _spill?.Clear(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary>Removes all lines.</summary> | ||
| public void Clear() | ||
| /// <summary>Releases the spill, deleting its cache. The in-memory window is unaffected.</summary> | ||
| public void Dispose() | ||
| { | ||
| lock (_gate) | ||
| { | ||
| _lines.Clear(); | ||
| _spill?.Dispose(); | ||
| } | ||
| } | ||
|
|
||
| private static void ValidateCount(int count) | ||
| { | ||
| if (count < 0) | ||
| { | ||
| throw new ArgumentOutOfRangeException(nameof(count)); | ||
| } | ||
|
|
||
| if (count > MaxRangeLines) | ||
| { | ||
| throw new ArgumentOutOfRangeException( | ||
| nameof(count), | ||
| count, | ||
| $"A single range is limited to {MaxRangeLines} lines; request history in pages."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Dispose() doesn't guard against post-dispose use.
Releases the spill, deleting its cache. The in-memory window is unaffected. But _spill isn't nulled and there's no _disposed flag: any later Append, Clear, or index query that touches _spill (e.g. _spill?.Append(evicted) in AppendLocked, _spill?.Clear() here, _spill.IsHealthy/spill.Count in OldestIndexLocked) will hit a disposed spill object. Depending on FileScrollbackSpill's post-dispose behavior, this could throw from an unrelated call site instead of a clear ObjectDisposedException on the buffer itself. See the related finding on WorldSession.DisposeAsync for a concrete path that can trigger this.
🛡️ Suggested guard
+ private bool _disposed;
+
public void Dispose()
{
lock (_gate)
{
+ if (_disposed)
+ {
+ return;
+ }
+
+ _disposed = true;
_spill?.Dispose();
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SharpMUTerm.Core/Text/ScrollbackBuffer.cs` around lines 250 - 287, Add
disposed-state tracking to TextScrollbackBuffer and have Dispose mark the buffer
disposed while releasing and clearing the spill resource. Guard public
operations such as Append, Clear, and index/query methods before they can reach
_spill, throwing ObjectDisposedException for post-dispose use; ensure repeated
Dispose calls remain safe.
Comment on lines
+22
to
+27
| /// <summary> | ||
| /// Maximum lines held on disk. Reaching it drops the oldest segment, so the achievable scroll | ||
| /// depth oscillates between roughly <c>MaxLines - SegmentMegabytes</c>' worth of lines and | ||
| /// <see cref="MaxLines"/>; that is the price of reclaiming space without rewriting the file. | ||
| /// </summary> | ||
| public int MaxLines { get; set; } = 200_000; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring mixes lines and megabytes.
Maximum lines held on disk. The next sentence says the scroll depth oscillates between MaxLines - SegmentMegabytes and MaxLines, but SegmentMegabytes is a size (MB), not a line count — subtracting it from MaxLines doesn't make dimensional sense. Likely meant "one segment's worth of lines".
📝 Suggested doc fix
/// <summary>
/// Maximum lines held on disk. Reaching it drops the oldest segment, so the achievable scroll
- /// depth oscillates between roughly <c>MaxLines - SegmentMegabytes</c>' worth of lines and
+ /// depth oscillates between roughly <c>MaxLines</c> minus one segment's worth of lines and
/// <see cref="MaxLines"/>; that is the price of reclaiming space without rewriting the file.
/// </summary>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// <summary> | |
| /// Maximum lines held on disk. Reaching it drops the oldest segment, so the achievable scroll | |
| /// depth oscillates between roughly <c>MaxLines - SegmentMegabytes</c>' worth of lines and | |
| /// <see cref="MaxLines"/>; that is the price of reclaiming space without rewriting the file. | |
| /// </summary> | |
| public int MaxLines { get; set; } = 200_000; | |
| /// <summary> | |
| /// Maximum lines held on disk. Reaching it drops the oldest segment, so the achievable scroll | |
| /// depth oscillates between roughly <c>MaxLines</c> minus one segment's worth of lines and | |
| /// <see cref="MaxLines"/>; that is the price of reclaiming space without rewriting the file. | |
| /// </summary> | |
| public int MaxLines { get; set; } = 200_000; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/SharpMUTerm.Core/Text/ScrollbackSpillOptions.cs` around lines 22 - 27,
Update the XML summary for MaxLines to describe the lower scroll-depth bound as
approximately MaxLines minus one segment’s worth of lines, rather than
subtracting SegmentMegabytes. Keep the existing explanation about dropping the
oldest segment and reclaiming space without rewriting the file.
Comment on lines
+42
to
+70
| [Test] | ||
| public async Task DefaultRoot_IsACacheLocationAndHonoursXdgCacheHome() | ||
| { | ||
| // A cache directory, never the config or data one: the contents are disposable by design. | ||
| await Assert.That(FileScrollbackSpill.DefaultRoot).Contains("SharpMUTerm"); | ||
| await Assert.That(FileScrollbackSpill.DefaultRoot).Contains("scrollback"); | ||
|
|
||
| if (OperatingSystem.IsWindows()) | ||
| { | ||
| await Assert.That(FileScrollbackSpill.DefaultRoot).Contains("cache"); | ||
| return; | ||
| } | ||
|
|
||
| var previous = Environment.GetEnvironmentVariable("XDG_CACHE_HOME"); | ||
| try | ||
| { | ||
| Environment.SetEnvironmentVariable("XDG_CACHE_HOME", "/xdg-cache-under-test"); | ||
| await Assert.That(FileScrollbackSpill.DefaultRoot) | ||
| .IsEqualTo(Path.Combine("/xdg-cache-under-test", "SharpMUTerm", "scrollback")); | ||
|
|
||
| Environment.SetEnvironmentVariable("XDG_CACHE_HOME", null); | ||
| await Assert.That(FileScrollbackSpill.DefaultRoot) | ||
| .Contains(OperatingSystem.IsMacOS() ? Path.Combine("Library", "Caches") : ".cache"); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable("XDG_CACHE_HOME", previous); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Target file ==\n'
cat -n tests/SharpMUTerm.Core.Tests/Text/ScrollbackSpillTests.cs | sed -n '1,220p'
printf '\n== Parallelization / env-var usage search ==\n'
rg -n --hidden --glob 'tests/**/*.cs' '\[NotInParallel\]|\[Parallel|XDG_CACHE_HOME|Environment\.SetEnvironmentVariable|DefaultRoot' tests/SharpMUTerm.Core.TestsRepository: SharpMUSH/SharpMUTerm
Length of output: 12655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Test project files ==\n'
git ls-files 'tests/**/*.csproj' 'tests/**/*.cs' | sed -n '1,200p'
printf '\n== Parallelization controls and env-var reads across tests ==\n'
rg -n --hidden --glob 'tests/**/*.cs' --glob 'tests/**/*.csproj' '\[NotInParallel\]|\[Parallel|XDG_CACHE_HOME|GetEnvironmentVariable\(|SetEnvironmentVariable\(|DefaultRoot|FileScrollbackSpill' testsRepository: SharpMUSH/SharpMUTerm
Length of output: 12781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== DefaultRoot implementation ==\n'
rg -n --hidden -C 4 'DefaultRoot|XDG_CACHE_HOME|Environment\.GetEnvironmentVariable' tests srcRepository: SharpMUSH/SharpMUTerm
Length of output: 10556
Serialize this test tests/SharpMUTerm.Core.Tests/Text/ScrollbackSpillTests.cs:43 mutates process-wide XDG_CACHE_HOME, so parallel tests can observe the temporary value and flake. Add [NotInParallel].
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/SharpMUTerm.Core.Tests/Text/ScrollbackSpillTests.cs` around lines 42 -
70, Add the test framework’s [NotInParallel] attribute to
DefaultRoot_IsACacheLocationAndHonoursXdgCacheHome, which mutates the
process-wide XDG_CACHE_HOME environment variable; leave the test logic
unchanged.
Comment on lines
+564
to
+639
| [Test] | ||
| public async Task AppendingWhileReading_IsSafeAndNeverServesATornRange() | ||
| { | ||
| using var root = new TempRoot(); | ||
| using var spill = new FileScrollbackSpill(Options(root.Path)); | ||
| using var buffer = new ScrollbackBuffer(capacity: 64, spill); | ||
|
|
||
| const int total = 20_000; | ||
| Exception? failure = null; | ||
| using var done = new ManualResetEventSlim(false); | ||
|
|
||
| var writer = new Thread(() => | ||
| { | ||
| try | ||
| { | ||
| for (var i = 0; i < total; i++) | ||
| { | ||
| buffer.Append(Line(i)); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| failure = ex; | ||
| } | ||
| finally | ||
| { | ||
| done.Set(); | ||
| } | ||
| }); | ||
|
|
||
| var reads = 0; | ||
| var reader = new Thread(() => | ||
| { | ||
| try | ||
| { | ||
| while (!done.IsSet) | ||
| { | ||
| var oldest = buffer.OldestIndex; | ||
| var end = buffer.TotalLines; | ||
| if (end - oldest < 200) | ||
| { | ||
| Thread.Yield(); | ||
| continue; | ||
| } | ||
|
|
||
| // A window at depth, chosen while lines are still arriving behind it. | ||
| var start = oldest + (end - oldest) / 3; | ||
| var range = buffer.GetRange(start, 40); | ||
| for (var i = 0; i < range.Count; i++) | ||
| { | ||
| if (range[i].Text != $"line {start + i}") | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Range at {start} returned '{range[i].Text}' at offset {i}"); | ||
| } | ||
| } | ||
|
|
||
| reads++; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| failure = ex; | ||
| } | ||
| }); | ||
|
|
||
| writer.Start(); | ||
| reader.Start(); | ||
| writer.Join(); | ||
| reader.Join(); | ||
|
|
||
| await Assert.That(failure).IsNull(); | ||
| await Assert.That(reads).IsGreaterThan(0); | ||
| await Assert.That(buffer.TotalLines).IsEqualTo((long)total); | ||
| await Assert.That(spill.IsHealthy).IsTrue(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add a timeout to the concurrent read/write stress test.
A future regression that introduces a deadlock or livelock in ScrollbackBuffer/FileScrollbackSpill would hang this test indefinitely rather than fail fast, stalling the whole CI run instead of surfacing a clear failure.
♻️ Suggested fix
[Test]
+[Timeout(30_000)]
public async Task AppendingWhileReading_IsSafeAndNeverServesATornRange()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [Test] | |
| public async Task AppendingWhileReading_IsSafeAndNeverServesATornRange() | |
| { | |
| using var root = new TempRoot(); | |
| using var spill = new FileScrollbackSpill(Options(root.Path)); | |
| using var buffer = new ScrollbackBuffer(capacity: 64, spill); | |
| const int total = 20_000; | |
| Exception? failure = null; | |
| using var done = new ManualResetEventSlim(false); | |
| var writer = new Thread(() => | |
| { | |
| try | |
| { | |
| for (var i = 0; i < total; i++) | |
| { | |
| buffer.Append(Line(i)); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| failure = ex; | |
| } | |
| finally | |
| { | |
| done.Set(); | |
| } | |
| }); | |
| var reads = 0; | |
| var reader = new Thread(() => | |
| { | |
| try | |
| { | |
| while (!done.IsSet) | |
| { | |
| var oldest = buffer.OldestIndex; | |
| var end = buffer.TotalLines; | |
| if (end - oldest < 200) | |
| { | |
| Thread.Yield(); | |
| continue; | |
| } | |
| // A window at depth, chosen while lines are still arriving behind it. | |
| var start = oldest + (end - oldest) / 3; | |
| var range = buffer.GetRange(start, 40); | |
| for (var i = 0; i < range.Count; i++) | |
| { | |
| if (range[i].Text != $"line {start + i}") | |
| { | |
| throw new InvalidOperationException( | |
| $"Range at {start} returned '{range[i].Text}' at offset {i}"); | |
| } | |
| } | |
| reads++; | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| failure = ex; | |
| } | |
| }); | |
| writer.Start(); | |
| reader.Start(); | |
| writer.Join(); | |
| reader.Join(); | |
| await Assert.That(failure).IsNull(); | |
| await Assert.That(reads).IsGreaterThan(0); | |
| await Assert.That(buffer.TotalLines).IsEqualTo((long)total); | |
| await Assert.That(spill.IsHealthy).IsTrue(); | |
| } | |
| [Test] | |
| [Timeout(30_000)] | |
| public async Task AppendingWhileReading_IsSafeAndNeverServesATornRange() | |
| { | |
| using var root = new TempRoot(); | |
| using var spill = new FileScrollbackSpill(Options(root.Path)); | |
| using var buffer = new ScrollbackBuffer(capacity: 64, spill); | |
| const int total = 20_000; | |
| Exception? failure = null; | |
| using var done = new ManualResetEventSlim(false); | |
| var writer = new Thread(() => | |
| { | |
| try | |
| { | |
| for (var i = 0; i < total; i++) | |
| { | |
| buffer.Append(Line(i)); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| failure = ex; | |
| } | |
| finally | |
| { | |
| done.Set(); | |
| } | |
| }); | |
| var reads = 0; | |
| var reader = new Thread(() => | |
| { | |
| try | |
| { | |
| while (!done.IsSet) | |
| { | |
| var oldest = buffer.OldestIndex; | |
| var end = buffer.TotalLines; | |
| if (end - oldest < 200) | |
| { | |
| Thread.Yield(); | |
| continue; | |
| } | |
| // A window at depth, chosen while lines are still arriving behind it. | |
| var start = oldest + (end - oldest) / 3; | |
| var range = buffer.GetRange(start, 40); | |
| for (var i = 0; i < range.Count; i++) | |
| { | |
| if (range[i].Text != $"line {start + i}") | |
| { | |
| throw new InvalidOperationException( | |
| $"Range at {start} returned '{range[i].Text}' at offset {i}"); | |
| } | |
| } | |
| reads++; | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| failure = ex; | |
| } | |
| }); | |
| writer.Start(); | |
| reader.Start(); | |
| writer.Join(); | |
| reader.Join(); | |
| await Assert.That(failure).IsNull(); | |
| await Assert.That(reads).IsGreaterThan(0); | |
| await Assert.That(buffer.TotalLines).IsEqualTo((long)total); | |
| await Assert.That(spill.IsHealthy).IsTrue(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/SharpMUTerm.Core.Tests/Text/ScrollbackSpillTests.cs` around lines 564 -
639, Add a bounded timeout to
AppendingWhileReading_IsSafeAndNeverServesATornRange so deadlocks or livelocks
fail the test promptly. Apply the timeout to the writer and reader completion
waits, and ensure timeout failures clearly identify which concurrent operation
did not finish while preserving the existing assertions.
Comment on lines
+130
to
+141
| var threw = false; | ||
| try | ||
| { | ||
| StyledLineCodec.Decode(payload, 0, payload.Length / 2); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| threw = true; | ||
| } | ||
|
|
||
| // The store's job is to never surface garbage; the codec's job is to refuse to invent it. | ||
| await Assert.That(threw).IsTrue(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the throw directly instead of a catch-all flag.
catch (Exception) passes on any failure, including one unrelated to decoding (e.g. an ArgumentException from the MemoryStream range). TUnit's Throws assertion — already used in ScrollbackSpillTests — is both tighter and shorter.
♻️ Proposed refactor
- var threw = false;
- try
- {
- StyledLineCodec.Decode(payload, 0, payload.Length / 2);
- }
- catch (Exception)
- {
- threw = true;
- }
-
// The store's job is to never surface garbage; the codec's job is to refuse to invent it.
- await Assert.That(threw).IsTrue();
+ await Assert.That(() => StyledLineCodec.Decode(payload, 0, payload.Length / 2))
+ .Throws<EndOfStreamException>();Confirm the exact exception type a truncated payload surfaces (EndOfStreamException from BinaryReader, or InvalidDataException/FormatException from the 7-bit prefix) and pin that type, or use ThrowsException() if it can legitimately vary.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var threw = false; | |
| try | |
| { | |
| StyledLineCodec.Decode(payload, 0, payload.Length / 2); | |
| } | |
| catch (Exception) | |
| { | |
| threw = true; | |
| } | |
| // The store's job is to never surface garbage; the codec's job is to refuse to invent it. | |
| await Assert.That(threw).IsTrue(); | |
| // The store's job is to never surface garbage; the codec's job is to refuse to invent it. | |
| await Assert.That(() => StyledLineCodec.Decode(payload, 0, payload.Length / 2)) | |
| .Throws<EndOfStreamException>(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/SharpMUTerm.Core.Tests/Text/StyledLineCodecTests.cs` around lines 130 -
141, Replace the catch-all flag around StyledLineCodec.Decode with TUnit’s
direct throw assertion, matching the existing pattern in ScrollbackSpillTests.
Verify the truncated payload’s actual exception type and assert that specific
type; use ThrowsException only if the codec can legitimately produce multiple
types.
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
1 participant
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.
Core.ScrollbackBufferkept a capped ring ofStyledLineand lost everything older, so the in-memory cap was also the maximum scroll depth. It now keeps the same modest window in memory, hands evicted lines to anIScrollbackSpill, and serves a range from either half transparently — the foundation the windowed output feed needs, since appending to aMarkupControlre-parses its whole content (10.8 ms for 1,000 lines, 88–116 ms for 20,000).Pure
SharpMUTerm.Core: nothing undersrc/SharpMUTerm.Tui/is touched.Format
StyledLineCodec— a length-prefixed binary payload per line: spans, per-span style, interactions (kind, target, hint, prompt-only) and the trigger rule colour, losslessly. Strings are length-prefixed UTF-8, so text needs no escaping and may contain control bytes, newlines, CJK, or emoji with joiners.Not the Tui's Spectre markup: markup is a
SharpMUTerm.Tuiconcern (MarkupFormatter) that Core must not depend on, and it is also lossy — it cannot distinguish a palette index from the RGB a theme resolves it to, has to escape[, and drops a span's hint and prompt-only flag. Not JSON lines either: 2–3× the bytes, escaping on the write path, and no way to tell a truncated record from a damaged one.Store
FileScrollbackSpill— a series of segment files, each a 16-byte magic/version header followed by records framed asuint32 length | payload | uint32 CRC-32. A short, self-inconsistent, or checksum-failing record is detected and read back asStyledLine.Empty, so damage becomes a blank row rather than an exception or garbage, and the range keeps its length so a paged view's rows do not shift.Every record's byte offset is held in memory (8 bytes a line), so lines i..j are one
preadof exactly their byte span — flat in depth, never a scan. The index is never rebuilt from disk because it never outlives the process that wrote it.Lifetime and bound
An ephemeral per-session, per-window cache under
$XDG_CACHE_HOME/SharpMUTerm/scrollback(%LOCALAPPDATA%\SharpMUTerm\cache\scrollback,~/Library/Caches/...), created on the first eviction, deleted on dispose, and purged of whatever a crash left behind on the first store created in a new process. A.lockfile held withFileShare.Nonekeeps two instances off each other's files and lets a purge tell a live store from an abandoned one. Not the session log —PlainTextLogSink/HtmlLogSinkstay separate, opt-in, formatted and kept.Bounded by lines and bytes (
ScrollbackSpillOptions, default 200,000 lines / 64 MB / 4 MB segments). Space is reclaimed by unlinking the oldest segment whole, so the cost is O(1) rather than a rewrite per line; the price is that the bound is enforced at segment granularity.Failure
No disk error reaches the caller. The first one logs once through the session's
ILogger(so it lands inClientDiagnostics), deletes the cache, and degrades to memory-only for the rest of the session. Not one live line is lost. Covered by tests: unusable directory, read-only directory, cache deleted underneath a running store, corrupted record, truncated tail, replaced segment file.API
Indices are now absolute, so a scroll position keeps meaning the same line:
TotalLines,OldestIndex,AvailableLines,SpilledLines,IsSpilling,GetRange(long start, int count),GetTail(int count).GetRangeis capped atMaxRangeLines(4,096) so no caller can materialise 100k lines by accident;Snapshot()is documented as the in-memory window only. All members are thread-safe; appends and reads are serialised on one lock with the spill's I/O under it.Measurements
The per-line write cost is not material: at 0.82 µs it is ~1/13th of the 10.8 µs/line the UI already spends re-parsing markup, and a busy MU* firehose of 1,000 lines/s spends 0.08% of a core on it.
Also, per the Boy Scout rule
The in-memory ring was a
LinkedList<StyledLine>— a heap node per line, andGetRangewalked from the head every call, so serving the newest 40 of 20,000 lines touched 19,960 nodes. Replaced with a geometrically-grown circular array.WorldSession.Loggerwas a plain auto-property, so anything built in the constructor could never see the app's logger; it now forwards to the scrollback buffer.Follow-up for the Tui
AppConfiguration.ScrollbackSpillexists but the Tui does not pass it yet, so a user's settings for it are not honoured —SessionManager.Open(..., spill: _config.ScrollbackSpill)is the one-line change, deliberately left out of this PR to stay clear of the concurrent output-pane work.Verification
dotnet build SharpMUTerm.slnx -c Releaseclean, 0 warnings. Core 474 (was 424), Graphics 83, Scripting 42, Web 30, Tui 788 — all green.🤖 Generated with Claude Code
https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
Summary by CodeRabbit