Fish integration: deterministic EINTR test; markers survive repeated interrupted writes (#443)

src/shell/adapters/FishAdapter.ts

@@ -29,18 +29,32 @@ const FISH_BUILTINS = new Set(['and', 'begin', 'bg', 'bind', 'block', 'break', '
   'math', 'not', 'or', 'path', 'printf', 'pwd', 'random', 'read', 'realpath', 'return', 'set', 'set_color', 'source', 'status', 'string', 'switch', 'test',
   'time', 'true', 'type', 'ulimit', 'wait', 'while', 'abbr', 'argparse']);
 
+/**
+ * One of NMSh's markers. Fish 3 does not retry a terminal write that a signal interrupts (a window-size change while
+ * the write waits): it prints "write: Interrupted system call" on its own stderr and the bytes are gone, and a lost
+ * marker leaves NMSh wrong about whether a command runs (#443). Retrying in Fish still printed that error, and a burst
+ * of resizes could interrupt every retry. On Fish 3 the marker goes through `cat` instead: printf only fills a pipe (a
+ * few bytes, never blocked), and cat, which ignores the window-size signal and retries an interrupted write, writes
+ * it to the terminal. The marker travels on stdin, never in a process's arguments. Fish 4 retries by itself.
+ */
+export const FISH_MARK_FUNCTION = `if string match -qr '^[0-3]\\.' -- $version; and command -q cat
+  function __nmsh_mark
+    printf $argv | command cat
+  end
+else
+  function __nmsh_mark
+    printf $argv 2>/dev/null; or printf $argv
+  end
+end`;
+
 function bootstrap({token, knowledgePath, bridgeEnvPath}: LaunchContext, originalTerm: string | undefined): string {
   const sync = bridgeEnvPath ? '\n    nmsh_bridge_sync' : '';
   const marker = (body: string) => `__nmsh_mark '\\e]777;nmsh;${token};${body}\\a'`;
   return `# NMSh managed fish session bootstrap (private, per session)
 ${originalTerm ? `set -gx TERM ${fishQuote(originalTerm)}` : ''}
 set -g fish_greeting
-# One of NMSh's markers, written again once if the write fails. Fish 3 does not retry a terminal write that a signal
-# interrupts (a window-size change while the write waits), and a lost marker leaves NMSh wrong about whether a command
-# runs (#443). An interrupted write normally writes nothing (as in #443's trace), so the marker arrives once.
-function __nmsh_mark
-  printf $argv 2>/dev/null; or printf $argv
-end
+# NMSh's markers survive an interrupted write (#443).
+${FISH_MARK_FUNCTION}
 # Fish >= 4 only; NMSh shows its own suggestions.
 set -g fish_autosuggestion_enabled 0
 function fish_prompt

tests/fishMarkerWrite.test.ts

@@ -0,0 +1,106 @@
+import test from 'node:test';
+import assert from 'node:assert/strict';
+import {spawnSync} from 'node:child_process';
+import pty from 'node-pty';
+import {existsSync, mkdtempSync, rmSync, writeFileSync} from 'node:fs';
+import {tmpdir} from 'node:os';
+import {join} from 'node:path';
+import {FISH_MARK_FUNCTION} from '../src/shell/adapters/FishAdapter.js';
+
+/**
+ * #443, by fault injection instead of timing. Fish 3 does not retry a terminal write that a signal interrupts: the
+ * kernel returns EINTR (ERESTARTSYS without SA_RESTART, as #443's strace showed for SIGWINCH), Fish prints "write:
+ * Interrupted system call" and the bytes are gone. Here a preloaded shim makes a terminal write() fail with exactly that
+ * error, a set number of times, when it contains a given text; nothing depends on when a signal lands. Only terminal
+ * writes fail, as with the real signal: a few bytes into a pipe never wait, so nothing interrupts them.
+ *
+ * Linux only (LD_PRELOAD), with a C compiler and Fish; CI's Ubuntu runner has Fish 3.7.0. Fish runs in a real terminal.
+ */
+const fish = ['/usr/bin/fish', '/usr/local/bin/fish', '/opt/homebrew/bin/fish'].find(existsSync);
+const cc = ['/usr/bin/cc', '/usr/bin/gcc', '/usr/bin/clang'].find(existsSync);
+const major = fish ? Number(/version (\d+)/u.exec(spawnSync(fish, ['--version'], {encoding: 'utf8'}).stdout ?? '')?.[1]) : NaN;
+const skip = process.platform !== 'linux' ? 'needs LD_PRELOAD (Linux)' : !fish ? 'fish not installed' : !cc ? 'no C compiler' : false;
+
+const SHIM = String.raw`
+#define _GNU_SOURCE
+#include <dlfcn.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+static ssize_t (*real_write)(int, const void *, size_t);
+static int remaining = -1;
+ssize_t write(int fd, const void *buffer, size_t length) {
+  if (!real_write) real_write = (ssize_t (*)(int, const void *, size_t))dlsym(RTLD_NEXT, "write");
+  if (remaining < 0) { const char *fails = getenv("QA_EINTR_FAILS"); remaining = fails ? atoi(fails) : 0; }
+  const char *match = getenv("QA_EINTR_MATCH");
+  if (remaining > 0 && match && *match && isatty(fd) && memmem(buffer, length, match, strlen(match))) { remaining -= 1; errno = EINTR; return -1; }
+  return real_write(fd, buffer, length);
+}
+`;
+
+let root: string | undefined;
+let shim: string | undefined;
+function build(): string {
+  if (shim) return shim;
+  root = mkdtempSync(join(process.env.TMPDIR ?? tmpdir(), 'nmsh-eintr-'));
+  writeFileSync(join(root, 'shim.c'), SHIM);
+  shim = join(root, 'shim.so');
+  const built = spawnSync(cc!, ['-shared', '-fPIC', '-o', shim, join(root, 'shim.c'), '-ldl'], {encoding: 'utf8'});
+  assert.equal(built.status, 0, built.stderr);
+  return shim;
+}
+test.after(() => { if (root) rmSync(root, {recursive: true, force: true}); });
+
+/**
+ * Run a Fish script with NMSh's marker function in a terminal; in every process, the first `fails` terminal writes
+ * containing `match` fail with EINTR. Resolves with everything the terminal received, once the script says it ended.
+ */
+function run(script: string, match: string, fails: number, mark = FISH_MARK_FUNCTION): Promise<string> {
+  const shim = build();
+  return new Promise((resolve, reject) => {
+    const child = pty.spawn(fish!, ['--no-config', '-c', `${mark}\n${script}\necho QA-END`], {cols: 200, rows: 20,
+      env: {...process.env, LD_PRELOAD: shim, QA_EINTR_MATCH: match, QA_EINTR_FAILS: String(fails)} as Record<string, string>});
+    let output = '';
+    const timer = setTimeout(() => { child.kill(); reject(new Error(`fish did not finish: ${JSON.stringify(output)}`)); }, 20_000);
+    child.onData(data => {
+      output += data;
+      if (output.includes('QA-END')) { clearTimeout(timer); child.kill(); resolve(output); }
+    });
+  });
+}
+const count = (text: string, part: string) => text.split(part).length - 1;
+const MARK = `__nmsh_mark '\\e]777;nmsh;QA;mark\\a'; echo QA-AFTER`;
+const MARKER = '\u001b]777;nmsh;QA;mark\u0007';
+
+test('the shim is faithful: an interrupted terminal write in Fish fails as #443\'s trace did', {skip}, async () => {
+  const output = await run('echo QA-OUTPUT', 'QA-OUTPUT', 1);
+  if (major < 4) {
+    // Fish 3's own output is lost, with the error #443 showed. NMSh cannot retry a person's command for them: the
+    // documented limit (Linux, Fish 3), whose cause is NMSh resizing the terminal while a command runs.
+    assert.equal(output.includes('QA-OUTPUT\r\n'), false, 'Fish 3 drops the interrupted write');
+    assert.match(output, /Interrupted system call/u);
+  } else assert.match(output, /QA-OUTPUT/u, 'Fish 4 retries an interrupted write');
+});
+
+for (const fails of [1, 2, 50]) {
+  test(`a marker whose terminal write is interrupted ${fails === 50 ? 'again and again' : fails === 1 ? 'once' : 'twice in a row (resizes come in bursts)'} arrives exactly once, with no error shown`, {skip}, async () => {
+    const output = await run(MARK, ']777;nmsh;QA;', fails);
+    assert.equal(count(output, MARKER), 1);
+    assert.match(output, /QA-AFTER/u, 'the command goes on');
+    assert.doesNotMatch(output, /Interrupted/u, 'no error reaches the person');
+  });
+}
+
+/** #445's helper (v0.22 candidate 68c1cbb7): one retry with the builtin. */
+const RETRY_ONCE = `function __nmsh_mark
+  printf $argv 2>/dev/null; or printf $argv
+end`;
+
+test('the harness tells them apart: on Fish 3, #445\'s single retry shows the error and loses a marker interrupted twice', {skip: skip || (major >= 4 ? 'Fish 4 retries by itself' : false)}, async () => {
+  const once = await run(MARK, ']777;nmsh;QA;', 1, RETRY_ONCE);
+  assert.equal(count(once, MARKER), 1, 'one retry is enough for one interruption');
+  assert.match(once, /Interrupted system call/u, 'but Fish 3 still prints the error into the transcript');
+  const twice = await run(MARK, ']777;nmsh;QA;', 2, RETRY_ONCE);
+  assert.equal(count(twice, MARKER), 0, 'and two interruptions in a row lose the marker');
+});