fix(split-pane): leave the owed marker to a trailing refresh, and say what a pull's request phase holds (#524 review)

- Stale second marker above a trailing refresh's replay: xterm parses
  write() on a later tick while clear() is synchronous, so a marker stamped
  in a load's finally, just before _endBufferLoad() starts the trailing
  refresh, landed in the freshly cleared buffer above that refresh's replay.
  _stampMarkerIfOwed() now returns early while a refresh is pending; that
  refresh re-owes the marker on a closed socket and writes the one copy
  below its own replay. Pinned by marker-count assertions on the two
  existing trailing-refresh tests plus a new async-parse fake (writes
  parsed on a later tick, clear() synchronous) for back-to-back refreshes
  and a pull with a queued refresh and a close mid-pull; all four fail
  without the guard. Also checked against a real @xterm/headless 6.0.0.
- Marker withheld for up to the 45 s request budget: kept the behaviour and
  made the comment and the docs truthful. The pull's request phase holds no
  live output, but it holds the single-flight flag, so a coalesced {t:'r'}
  refresh and a close's owed marker wait for the response. Writing the
  marker at once during that phase would need a separate "awaiting
  response" state and, with a refresh pending, reopens the same
  write-vs-clear() race as above; a Codeman restart resets the in-flight
  request along with the socket, so that pull fails at once and stamps.
- Stale comments: _onSocketClosed() now says the deferral covers any load,
  _writeDisconnectedMarker() points at _stampMarkerIfOwed(), and the pull's
  finally comment describes the hand-off to a trailing refresh.
- Invariants doc: dropped "the initial load" from the loads a close can land
  in (connect() awaits it before creating the socket), reworded the
  "nested refresh stamps its own" sentence to describe the guard, and noted
  what the request phase holds.

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

src/web/public/terminal-split.js

@@ -307,10 +307,13 @@
     }
 
     // The socket's close, split out of connect() so the tests can drive it.
-    // While a history pull is running the marker waits for the pull's finally
-    // block: written now, it would sit above the output the pull is still
-    // holding (flushed after it on a skip, a downgrade or a failed fetch) or
-    // land in the middle of a chunked replay.
+    // While any load runs (a history pull or a `{t:'r'}` refresh) the marker is
+    // only owed, and that load's finally block settles it (_stampMarkerIfOwed()):
+    // written now, it would sit above the output a pull is still holding (flushed
+    // after it on a skip, a downgrade or a failed fetch), above a refresh's
+    // replay, or in the middle of a chunked replay. A pull still waiting for its
+    // response holds the marker too, for as long as the request takes (up to its
+    // budget, see _pullHistory()).
     _onSocketClosed() {
       this._wsReady = false;
       this._wsClosed = true;
@@ -320,16 +323,21 @@
 
     // Settles a marker the pane owes: set when a close lands during a load (the
     // replay would otherwise sit below it) or when a load wipes the terminal on
-    // a closed socket. Called from each load's own finally, before a trailing
-    // refresh starts, so a nested refresh stamps its own.
+    // a closed socket. Called from each load's own finally, just before
+    // _endBufferLoad() starts any trailing refresh.
     _stampMarkerIfOwed() {
+      // A trailing refresh is about to clear() synchronously, while xterm parses
+      // a write() on a later tick: a marker written here would land in the
+      // freshly cleared buffer ABOVE that refresh's replay, a second, stale copy.
+      // The refresh re-owes the marker on a closed socket and stamps it itself.
+      if (this._bufferRefreshPending && !this._destroyed) return;
       const owed = this._markerOwed;
       this._markerOwed = false;
       if (owed && this._wsClosed && !this._destroyed) this._writeDisconnectedMarker();
     }
 
-    // Extracted so both _onSocketClosed() and a history pull that ends on a
-    // closed socket can write it (see _pullHistory()'s finally block).
+    // Extracted so both _onSocketClosed() and a load that ends owing it on a
+    // closed socket can write it (see _stampMarkerIfOwed()).
     _writeDisconnectedMarker() {
       this.terminal?.write('\r\n\x1b[2m[Pane B disconnected — close and reopen the split to reconnect]\x1b[0m\r\n');
     }
@@ -452,12 +460,15 @@
       let replayed = false;
       let capturedAt = 0;
       // Two budgets on one signal. The request itself gets the primary pane's
-      // (CodemanFetchDeadline, constants.js): nothing is held while it runs. Once
-      // the headers land live output IS held, so the body read gets the short one
-      // instead: a body that hangs would otherwise freeze the pane for the long
-      // budget. Aborting lands in the catch below, which releases the flag and
-      // the queue. AbortSignal.timeout() alone cannot be re-armed, hence the
-      // controller; without AbortController the pull simply has no deadline.
+      // (CodemanFetchDeadline, constants.js): live output is not held while it
+      // runs, but the single-flight flag is, so a coalesced `{t:'r'}` refresh and
+      // the marker owed by a close (_onSocketClosed()) both wait for it, at worst
+      // for that whole budget. Once the headers land live output IS held, so the
+      // body read gets the short one instead: a body that hangs would otherwise
+      // freeze the pane for the long budget. Aborting lands in the catch below,
+      // which releases the flag and the queue. AbortSignal.timeout() alone cannot
+      // be re-armed, hence the controller; without AbortController the pull
+      // simply has no deadline.
       const controller = global.AbortController ? new global.AbortController() : null;
       let abortTimer = null;
       const armDeadline = (ms) => {
@@ -539,7 +550,8 @@
         // it while a load runs), and a replay's own `\x1bc` (flagged above) wipes
         // one written before it, which would paint a fresh, current-looking
         // history while onData keeps silently dropping every keystroke on the
-        // dead socket. A trailing refresh (_endBufferLoad) settles its own.
+        // dead socket. With a trailing refresh pending (_endBufferLoad) the marker
+        // is left to that refresh, which writes it below its own replay.
         this._stampMarkerIfOwed();
         this._endBufferLoad();
       }

test/split-pane-terminal-unit.test.ts

@@ -805,8 +805,11 @@ describe('SplitTerminalPane scroll-to-top history pull', () => {
   });
 
   it('back-to-back refreshes on a closed socket leave exactly one marker, at the end', async () => {
-    // R1's finally runs the trailing refresh R2; each load settles its own
-    // marker, so R1 never stamps onto R2's freshly cleared terminal.
+    // R1's finally runs the trailing refresh R2, so R1 leaves the owed marker to
+    // R2 instead of stamping it: in real xterm R1's write would still be queued
+    // when R2's synchronous clear() runs, and would land above R2's replay. The
+    // write mock records every stamp whatever clear() does, so counting marker
+    // writes pins that R1 never stamps (the async-parse test below shows why).
     const pane = makePane('shell');
     pane._wsClosed = true;
     const first = deferred<ReturnType<typeof jsonResponse>>();
@@ -825,6 +828,7 @@ describe('SplitTerminalPane scroll-to-top history pull', () => {
     const writes = pane.terminal.write.mock.calls.map((c) => c[0]);
     expect(writes.at(-1)).toSatisfy(isMarker);
     expect(writes.lastIndexOf('second')).toBe(writes.length - 2);
+    expect(writes.filter(isMarker)).toHaveLength(1);
   });
 
   it('the pull gives the request the long budget and the body read the short one', async () => {
@@ -921,6 +925,64 @@ describe('SplitTerminalPane scroll-to-top history pull', () => {
     expect(writes).toContain('refreshed');
     expect(isMarker(writes.at(-1))).toBe(true);
     expect(writes.lastIndexOf('refreshed')).toBeLessThan(writes.length - 1);
+    // The pull left the owed marker to the refresh rather than stamping it too.
+    expect(writes.filter(isMarker)).toHaveLength(1);
+  });
+
+  it.each([
+    [
+      'back-to-back refreshes',
+      async (pane: PaneUnderTest) => {
+        pane._wsClosed = true;
+        fetchMock.mockResolvedValueOnce(jsonResponse('first')).mockResolvedValueOnce(jsonResponse('second'));
+        pane._refreshBuffer();
+        pane._refreshBuffer(); // coalesced into the trailing re-run
+      },
+    ],
+    [
+      'a pull with a queued refresh and a close mid-pull',
+      async (pane: PaneUnderTest) => {
+        const held = headersOnly();
+        fetchMock.mockResolvedValueOnce(held.response).mockResolvedValueOnce(jsonResponse('second'));
+        void pane._pullHistory();
+        await settle(); // the response landed: the queue is open
+        pane._refreshBuffer(); // coalesced into the trailing re-run
+        pane._onSocketClosed();
+        held.release(rowsOf(30)); // no replay
+      },
+    ],
+  ])('with xterm parsing writes on a later tick, %s leave one marker on screen, last', async (_label, drive) => {
+    // Real xterm queues write() and parses it on a later tick (WriteBuffer's
+    // setTimeout), while clear() rewrites the buffer at once. The default fake
+    // applies writes synchronously and so cannot show a marker overtaken by a
+    // trailing refresh's clear(): parsed after it, that marker sat above the
+    // refresh's replay as a second, stale copy.
+    const pane = makePane('shell');
+    const screen: string[] = [];
+    const pending: Array<{ data: string; done?: () => void }> = [];
+    pane.terminal.write = vi.fn((data: string, done?: () => void) => {
+      if (pending.length === 0) {
+        setTimeout(() => {
+          for (const entry of pending.splice(0)) {
+            if (entry.data === '\x1bc') screen.length = 0;
+            else if (entry.data) screen.push(entry.data);
+            entry.done?.();
+          }
+        }, 0);
+      }
+      pending.push({ data, done });
+    });
+    pane.terminal.clear = vi.fn(() => {
+      screen.length = 0;
+    });
+
+    await drive(pane);
+    for (let i = 0; i < 5; i++) await settle();
+
+    expect(pane._bufferLoading).toBe(false);
+    expect(screen.filter(isMarker)).toHaveLength(1);
+    expect(screen.at(-1)).toSatisfy(isMarker);
+    expect(screen.indexOf('second')).toBe(screen.length - 2);
   });
 
   it('a refresh on an open socket does not stamp a marker', async () => {