Stop shredding every compressed session by settling our own phases (#164)

* Stop shredding every compressed session by settling our own phases

Five Evennia games have had their CODEBASE stored as "enniaA 5.0.1" and
"enniaF 6.0.0 (rev ea0da3ed8)R ##D HRINFO0m" for months. It is not an
Evennia quirk and it is not a parser gap: everything those servers said
after their connect screen arrived shredded, with fragments of two replies
overlaid on each other.

    RAW[14] Ne, : ekMUD                              (Name: TrekMUD)
    RAW[17] Vsion 6: enniaA.1.0!                     (Version: Evennia 6.1.0)
    RAW[20] RRRRmmand 'WHVERSION scnot available.    (two replies in one line)

The cause is ours. Every phase ended in FlushPendingLineAsync, which pushed
a newline through InterpretAsync to shake loose a line the server never
terminated. That is the *inbound* channel — the same one the read loop feeds
— and on an MCCP2 session an inflater sits at the head of it. So the newline
was never delivered to the line buffer at all; it was spliced into the
middle of the peer's deflate stream as though the peer had sent it, and the
inflater's state never recovered.

Ruled out in this order, each with a run rather than an argument:

  - Evennia. Captured the raw compressed stream off trekmud.com:1701 with a
    plain socket; Python's zlib decodes it perfectly.
  - TelnetNegotiationCore's inflater. Fed that same captured stream one byte
    at a time through MCCPInflateTransform: 703 of 703 bytes identical to
    zlib's output.
  - TelnetNegotiationCore generally. A minimal TNC client reads the block
    cleanly against the live server, with MCCP alone, with all twelve of this
    probe's plugins, with this probe's exact send sequence, and with the same
    lock-polling — and it is the same on 2.11.0 and 2.12.0.
  - Our plugin set, our builder, our buffer handling. Reduced the probe to a
    builder chain identical to that clean client and it still shredded;
    replaced the phase logic with fixed delays, same builder, and it stopped.

The fix is not to skip the flush when compressed — that trades one loss for
another, and the guard that keeps a busy DIKU from reading as a measured
zero depends on seeing an unterminated prompt. TelnetNegotiationCore 2.12.0
added PacketPatchProtocol, which infers a prompt boundary from silence on the
interpreter's own byte-processing loop, where the line buffer has exactly one
writer and nothing is pushed into the peer's stream. It retires itself the
moment a server marks a real prompt with IAC GA or IAC EOR. So: take the new
version, register the plugin, and delete the hand-rolled version of it.

A prompt lands in LastPromptBytes rather than through OnSubmit — the library
will not pretend a prompt is a line — so the probe appends it, in the order
taken. Deduplicated by identity, not content: every take allocates a fresh
array, and a server repeating its gate is showing it again, which is what
ARepeatingGateStopsAtTheRoundBound needs to see.

ProbeOptions.PromptHold is 500ms, the library's default and what TinTin++
and Mudlet both use; shorter splits a line at any server that pauses
mid-output. A phase waits it out rather than racing it, and only when
HasPartialLine says something is actually being held — which is false for
the overwhelming majority of settles.

ACompressedSessionIsNotShreddedByOurOwnSettling fails on the old behaviour
and passes on the new; FakeGame gains an AnnouncesMccp mode so the fixture
really compresses. Verified live: all five Evennia games now read
Evennia 6.1.0 / 5.0.1 / 6.1.0 (rev e174e49f5) / 6.0.0 (rev ea0da3ed8).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Say the settle rule once, instead of waiting for it afterwards

The wait for a held line was a step tacked onto the end of every flush,
which read as though the probe were re-implementing something the library
already does. It is not that — it is a rule about when a phase is over — so
it belongs in the one place that decides that.

A phase is not settled while the library is holding an unterminated line.
The two clocks start at different moments: SettleAsync's at the last line,
PacketPatchProtocol's at the fragment that arrived after it. Ours therefore
always expires first, and a phase that ended there would push its own prompt
into the next phase's slice — the misattribution the cursors exist to
prevent. MaxPhase still bounds it, so a server holding a fragment for ever
is not waited on for ever.

Fast() now holds for the same 120ms as its QuietPeriod, mirroring
production's 500/500. The suite previously passed on a margin — an 80ms hold
against a 120ms settle — which is not the ratio that ships and so proved
nothing about it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Take the prompt where the library hands it over, not at a phase boundary

OnPrompt is the hook for this, and the probe was already using it — for EOR
and Suppress Go-Ahead, to record the boolean fact that a server marks its
prompt boundaries. It just never took the text.

All three of the library's boundaries behave the same way: IAC EOR, IAC GA
and PacketPatchProtocol inferring one from silence each take the standing
partial line into LastPromptBytes and then call back, on the interpreter's
own byte-processing loop. None of them submits it — a prompt is not a line
and the library will not pretend otherwise — so nothing reaches OnSubmit.

So all three now hand it to one PromptSink, which appends it to the probe's
line list. That the callback runs on the byte-processing loop is the whole
reason it belongs there: the prompt lands between the lines either side of
it, in the order it was taken, rather than being swept up afterwards at the
next settle. Which in turn deletes the identity-dedupe the sweep needed —
the callback fires once per prompt, so there is nothing to deduplicate — and
the `taken` parameter that was threaded through five signatures to carry it.

This also covers a marked prompt, which the previous version reached only by
accident. TNC 2.12.0 routes IAC GA through TakePartialLineAsPrompt too, so a
server that ends its prompts with GA had exactly the same problem as one
that ends them with nothing, and neither is special-cased now.

FlushPendingLineAsync is what its name says again: wait for the interpreter
to finish processing. The settle rule added in the previous commit stays —
it is about when a phase is over, not about collecting anything.

ACompressedSessionIsNotShreddedByOurOwnSettling still fails on the old
behaviour and passes on the new. Crawl 485 three times over, Catalog 640,
Crawler 332, Discovery 335, Web 1153.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: HarryCordewener

Directory.Packages.props

@@ -11,7 +11,7 @@
       IS the capability measurement (spec §6.1), so what a server offers is observed rather than
       taken from a game's own MSSP claim.
     -->
-    <PackageVersion Include="TelnetNegotiationCore" Version="2.11.0" />
+    <PackageVersion Include="TelnetNegotiationCore" Version="2.12.0" />
 
     <!-- Logging abstractions (TelnetNegotiationCore takes an ILogger). -->
     <PackageVersion Include="Microsoft.Extensions.Logging.Abstractions" Version="10.0.2" />

src/MUI.Crawl/Telnet/ProbeOptions.cs

@@ -31,6 +31,18 @@ public sealed record ProbeOptions
     /// </remarks>
     public TimeSpan QuietPeriod { get; init; } = TimeSpan.FromMilliseconds(500);
 
+    /// <summary>
+    /// How long a server must stay silent on an unterminated line before it is taken as a prompt.
+    /// </summary>
+    /// <remarks>
+    /// Handed to TelnetNegotiationCore's <c>PacketPatchProtocol</c>, which does the holding on its
+    /// own byte-processing loop. 500 ms is the library's default and the convention the hobby's
+    /// clients settled on — TinTin++'s packet patch and Mudlet's posting timer both use it. Shorter
+    /// would split a line at any server that pauses mid-output, so a phase waits this out rather
+    /// than racing it, and only when the library says it is holding a line.
+    /// </remarks>
+    public TimeSpan PromptHold { get; init; } = TimeSpan.FromMilliseconds(500);
+
     /// <summary>
     /// How long to wait for a phase to produce anything at all before concluding it never will.
     /// </summary>

src/MUI.Crawl/Telnet/TelnetProbe.cs

@@ -100,7 +100,11 @@ public async Task<ProbeResult> ProbeAsync(ProbeTarget target, CancellationToken
                 ? client.ConnectAsync([.. target.Addresses], target.Port, budget.Token)
                 : client.ConnectAsync(target.Host, target.Port, budget.Token));
 
-            var built = await Build(seen, lines).BuildAndStartAsync(client, budget.Token);
+            var prompts = new PromptSink(lines);
+            var built = await Build(seen, lines, prompts).BuildAndStartAsync(client, budget.Token);
+
+            // Before anything can be read from the socket in practice — see PromptSink.Reads.
+            prompts.Reads(built.Interpreter);
 
             // Must be disposed *before* the socket it reads. The interpreter owns a byte channel, the
             // draining task, and every plugin — MCCP's holds zlib streams that nothing else reclaims,
@@ -249,7 +253,10 @@ or IOException
     /// and for what recording the placeholder cost.
     /// </remarks>
     private async Task SettleInitialBannerAsync(
-        TelnetInterpreter telnet, Func<int> arrived, List<byte[]> lines, CancellationToken cancellationToken)
+        TelnetInterpreter telnet,
+        Func<int> arrived,
+        List<byte[]> lines,
+        CancellationToken cancellationToken)
     {
         await SettleAsync(telnet, arrived, 0, _options.SilenceGrace, cancellationToken);
 
@@ -741,6 +748,16 @@ private async Task SettleAsync(
                 continue;
             }
 
+            // Not settled while the library is still holding an unterminated line. The two clocks
+            // start at different moments — ours at the last line, PacketPatchProtocol's at the
+            // fragment that arrived after it — so ours always expires first, and a phase that ended
+            // here would push its own prompt into the next phase's slice. MaxPhase still bounds the
+            // wait, so a server that holds a fragment for ever is not waited on for ever.
+            if (telnet.HasPartialLine)
+            {
+                continue;
+            }
+
             if (DateTime.UtcNow - lastChange >= (seen == 0 ? grace : _options.QuietPeriod))
             {
                 break;
@@ -764,10 +781,53 @@ private async Task SettleAsync(
     /// nothing goes on the wire and line assembly/IAC/encoding stay the library's; when the buffer is
     /// empty the library discards it and no line is produced.
     /// </remarks>
-    private static async Task FlushPendingLineAsync(TelnetInterpreter telnet)
+    private static Task FlushPendingLineAsync(TelnetInterpreter telnet) =>
+        telnet.WaitForProcessingAsync(maxWaitMs: 500, additionalDelayMs: 25).AsTask();
+
+    /// <summary>
+    /// Where a prompt the library took goes: onto the end of this probe's line list.
+    /// </summary>
+    /// <remarks>
+    /// All three of the library's prompt boundaries — <c>IAC EOR</c>, <c>IAC GA</c>, and
+    /// <c>PacketPatchProtocol</c> inferring one from silence — take the standing partial line into
+    /// <c>LastPromptBytes</c> and then call back. They do not submit it: a prompt is not a line and
+    /// the library will not pretend otherwise, so nothing reaches <c>OnSubmit</c> and the probe would
+    /// never see an unterminated login prompt at all.
+    /// <para>
+    /// Every callback runs on the interpreter's own byte-processing loop, which is what makes this
+    /// the right place to do it: the prompt lands in <c>lines</c> in the order it was taken, between
+    /// the lines either side of it, rather than being swept up afterwards at a phase boundary.
+    /// </para>
+    /// </remarks>
+    private sealed class PromptSink(List<byte[]> lines)
     {
-        await telnet.InterpretAsync(NewLine);
-        await telnet.WaitForProcessingAsync(maxWaitMs: 500, additionalDelayMs: 25);
+        private TelnetInterpreter? _telnet;
+
+        /// <summary>
+        /// Names the interpreter to read prompts from, once the builder has produced it.
+        /// </summary>
+        /// <remarks>
+        /// Assigned immediately after the build rather than during it, because the callbacks are
+        /// registered before the interpreter they read from exists. Nothing is lost in between: the
+        /// earliest prompt a peer can produce needs either a marker it has not sent yet or
+        /// <c>PromptHold</c> of silence, and this runs in the same continuation as the build.
+        /// </remarks>
+        public void Reads(TelnetInterpreter telnet) => _telnet = telnet;
+
+        public ValueTask TakeAsync()
+        {
+            if (_telnet?.LastPromptBytes is not { IsEmpty: false } prompt)
+            {
+                return ValueTask.CompletedTask;
+            }
+
+            lock (lines)
+            {
+                lines.Add(prompt.ToArray());
+            }
+
+            return ValueTask.CompletedTask;
+        }
     }
 
     /// <summary>
@@ -784,7 +844,7 @@ private static async Task FlushPendingLineAsync(TelnetInterpreter telnet)
     /// separate statement apiece; the TelnetNegotiationCore plugin builders are already self-typed
     /// fluent, so this is the shape the library wants, not a new abstraction layered over it.
     /// </remarks>
-    private TelnetInterpreterBuilder Build(Observations seen, List<byte[]> lines)
+    private TelnetInterpreterBuilder Build(Observations seen, List<byte[]> lines, PromptSink prompts)
     {
         void Note(string protocol) => seen.Note(protocol);
 
@@ -846,7 +906,7 @@ private TelnetInterpreterBuilder Build(Observations seen, List<byte[]> lines)
             {
                 seen.Prompts = true;
                 Note("EOR");
-                return ValueTask.CompletedTask;
+                return prompts.TakeAsync();
             });
 
         // The other prompt marker, and the one most of this hobby actually uses. RFC 854 makes a bare
@@ -863,7 +923,7 @@ private TelnetInterpreterBuilder Build(Observations seen, List<byte[]> lines)
             .OnPrompt(() =>
             {
                 seen.Prompts = true;
-                return ValueTask.CompletedTask;
+                return prompts.TakeAsync();
             });
 
         // CharsetProtocol takes its order as a property rather than a builder call, and exposes
@@ -912,7 +972,15 @@ private TelnetInterpreterBuilder Build(Observations seen, List<byte[]> lines)
             .AddPlugin(goAhead)
             .AddPlugin(new Watched.Naws(Note))
             .AddPlugin(terminalType)
-            .AddPlugin(new Watched.Echo(Note));
+            .AddPlugin(new Watched.Echo(Note))
+
+            // What FlushPendingLineAsync used to do by hand, done on the interpreter's own
+            // byte-processing loop where the line buffer has exactly one writer — and, crucially,
+            // without pretending the peer sent anything. It retires itself the moment a server
+            // marks a real prompt with IAC GA or IAC EOR.
+            .AddPlugin(new PacketPatchProtocol()
+                .WithHoldTime(_options.PromptHold)
+                .OnPrompt(prompts.TakeAsync));
     }
 
     /// <summary>

tests/MUI.Crawl.Tests/Telnet/ProbeSessionTests.cs

@@ -34,6 +34,7 @@ public class ProbeSessionTests
         PollInterval = TimeSpan.FromMilliseconds(15),
         Timeout = TimeSpan.FromSeconds(20),
         MsspSettleGrace = TimeSpan.FromMilliseconds(400),
+        PromptHold = TimeSpan.FromMilliseconds(120),
     };
 
     [Test]
@@ -587,6 +588,76 @@ await Assert.That(game.Received.Where(line => line.Trim().Length > 0)
             .All(line => TelnetProbe.PermittedCommands.Contains(line.Trim()))).IsTrue();
     }
 
+    /// <summary>
+    /// A compressed session is not corrupted by the probe settling its own phases.
+    /// </summary>
+    /// <remarks>
+    /// The bug this fixture exists for. Every phase used to end by pushing a newline into
+    /// <c>InterpretAsync</c> to shake loose a line the server never terminated — but that is the
+    /// <em>inbound</em> channel, the one an MCCP inflater sits at the head of, so on a compressed
+    /// session the byte was spliced into the middle of the peer's deflate stream. The inflater never
+    /// recovered: every reply afterwards arrived shredded, with fragments of two of them overlaid.
+    /// <para>
+    /// Found on the five Evennia games in the catalogue, whose <c>CODEBASE</c> had been stored as
+    /// <c>enniaA 5.0.1</c> and <c>enniaF 6.0.0 (rev ea0da3ed8)R ##D HRINFO0m</c> for months. Neither
+    /// Evennia nor TelnetNegotiationCore was at fault: the same captured stream decodes perfectly
+    /// through Python's zlib, through TNC's own inflate transform byte for byte, and through a plain
+    /// TNC client against the live server.
+    /// </para>
+    /// </remarks>
+    [Test]
+    public async Task ACompressedSessionIsNotShreddedByOurOwnSettling()
+    {
+        await using var game = new FakeGame
+        {
+            AnnouncesMccp = true,
+            Banner = "Welcome to Mortal Realms\r\nMrMud 1.4\r\n",
+            BannerTail = "By what name do you wish to be known? ",
+            WhoReply = "Player Name        On For Idle\r\n7 Players logged in, 22 record, no maximum.\r\n",
+            InfoReply = "### Begin INFO 1\r\nName: Mortal Realms\r\nConnected: 7\r\nVersion: Evennia 6.1.0\r\n### End INFO\r\n",
+        };
+
+        var result = await new TelnetProbe(Fast()).ProbeAsync(game.Target);
+
+        await Assert.That(result.Outcome).IsEqualTo(ProbeOutcome.Answered);
+
+        // The precondition, asserted rather than assumed: this session really was compressed.
+        await Assert.That(result.OfferedOptions).Contains("MCCP2");
+
+        // Every reply intact, and read rather than merely present.
+        await Assert.That(result.Who.Count).IsEqualTo(7);
+        await Assert.That(result.Info).Contains("Name: Mortal Realms");
+        await Assert.That(LoginCommandReading.MeaningfulCodebase(result.Info, result.Version))
+            .IsEqualTo("Evennia 6.1.0");
+        await Assert.That(LoginCommandReading.ConnectedPlayers(result.Info)).IsEqualTo(7);
+    }
+
+    /// <summary>
+    /// And the unterminated line survives compression too, which is what the newline was for.
+    /// </summary>
+    /// <remarks>
+    /// The fix is not "stop flushing on a compressed session" — that would trade one loss for
+    /// another, and the guard that keeps a busy DIKU from reading as a measured zero depends on
+    /// seeing exactly this kind of unterminated prompt. TelnetNegotiationCore 2.12.0's
+    /// <c>PacketPatchProtocol</c> infers the boundary from silence on its own byte-processing loop,
+    /// where the line buffer has one writer and nothing is pushed into the peer's stream.
+    /// </remarks>
+    [Test]
+    public async Task AnUnterminatedPromptIsStillDeliveredOnACompressedSession()
+    {
+        await using var game = new FakeGame
+        {
+            AnnouncesMccp = true,
+            Banner = "Welcome to Nowhere\r\nA quiet little place.\r\n",
+            BannerTail = "By what name do you wish to be known? ",
+        };
+
+        var result = await new TelnetProbe(Fast()).ProbeAsync(game.Target);
+
+        await Assert.That(result.OfferedOptions).Contains("MCCP2");
+        await Assert.That(result.Banner).Contains("By what name do you wish to be known?");
+    }
+
     /// <summary>
     /// A server that has negotiated MSSP but whose report is still in flight is waited for, rather
     /// than having the flush read <c>Supported</c> at whatever instant it happens to still be empty.
@@ -1432,6 +1503,16 @@ public FakeGame()
         /// </remarks>
         public bool AnnouncesMssp { get; init; }
 
+        /// <summary>
+        /// Whether the server compresses everything after its option handshake, as MCCP2 servers do.
+        /// </summary>
+        /// <remarks>
+        /// Load-bearing rather than decoration: with compression on, anything the probe pushes into
+        /// its own inbound channel is spliced into the peer's deflate stream, and every reply after
+        /// it comes out shredded. That is what this fixture exists to catch.
+        /// </remarks>
+        public bool AnnouncesMccp { get; init; }
+
         /// <summary>
         /// How long <see cref="AnnouncesMssp"/>'s report is withheld after our <c>DO MSSP</c> arrives
         /// — <c>WILL MSSP</c> still negotiates on connect, same as always, so this models a server
@@ -1602,6 +1683,11 @@ private async Task ServeOverTelnetAsync(TcpClient client)
                 builder = builder.AddPlugin(mssp);
             }
 
+            if (AnnouncesMccp)
+            {
+                builder = builder.AddPlugin(new MCCPProtocol());
+            }
+
             var built = await builder.BuildAndStartAsync(client, _stopping.Token);
 
             await using var telnet = built.Interpreter;