From f909e778f8d05dfd990be28d383de6c2f9ca48e4 Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:18:40 +0530 Subject: [PATCH 1/2] Fish integration: fault-injection test for an interrupted marker write (#443) #443's evidence so far was timing: a CI loop and an strace that saw two real interrupted marker writes retried. This test makes write() fail with EINTR a set number of times through a preloaded shim (Linux, Fish 3.7 on CI), so nothing depends on when SIGWINCH lands. It also characterises the residual: Fish 3 drops its own interrupted output; Fish 4 retries. The marker function moves to an exported constant so the test runs the bootstrap's own text. Expected to fail on CI before the next commit: two interruptions in a row, and a write that never succeeds (the retry prints "write: Interrupted system call"). --- src/shell/adapters/FishAdapter.ts | 17 ++++-- tests/fishMarkerWrite.test.ts | 93 +++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 6 deletions(-) create mode 100644 tests/fishMarkerWrite.test.ts diff --git a/src/shell/adapters/FishAdapter.ts b/src/shell/adapters/FishAdapter.ts index 37332210..f947aaa4 100644 --- a/src/shell/adapters/FishAdapter.ts +++ b/src/shell/adapters/FishAdapter.ts @@ -29,18 +29,23 @@ 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, 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. + */ +export const FISH_MARK_FUNCTION = `function __nmsh_mark + printf $argv 2>/dev/null; or printf $argv +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 diff --git a/tests/fishMarkerWrite.test.ts b/tests/fishMarkerWrite.test.ts new file mode 100644 index 00000000..a241e093 --- /dev/null +++ b/tests/fishMarkerWrite.test.ts @@ -0,0 +1,93 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import {spawnSync} from 'node:child_process'; +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 write() fail with exactly that error, a + * set number of times, for a write that contains a given text; nothing depends on when a signal lands. + * + * Linux only (LD_PRELOAD), with a C compiler and Fish; CI's Ubuntu runner has Fish 3.7.0. + */ +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 +#include +#include +#include +#include +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 && 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; the first `fails` writes containing `match` fail with EINTR. */ +function run(script: string, match: string, fails: number) { + const result = spawnSync(fish!, ['--no-config', '-c', `${FISH_MARK_FUNCTION}\n${script}`], {encoding: 'utf8', timeout: 20_000, + env: {...process.env, LD_PRELOAD: build(), QA_EINTR_MATCH: match, QA_EINTR_FAILS: String(fails)}}); + assert.equal(result.error, undefined, 'fish ran and ended'); + return {stdout: result.stdout, stderr: result.stderr}; +} +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 write in Fish fails as #443\'s trace did', {skip}, () => { + const {stdout, stderr} = 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(stdout.includes('QA-OUTPUT'), false, 'Fish 3 drops the interrupted write'); + assert.match(stderr, /Interrupted system call/u); + } else assert.match(stdout, /QA-OUTPUT/u, 'Fish 4 retries an interrupted write'); +}); + +test('a marker whose write is interrupted once arrives exactly once', {skip}, () => { + const {stdout, stderr} = run(MARK, ']777;nmsh;QA;', 1); + assert.equal(count(stdout, MARKER), 1); + assert.match(stdout, /QA-AFTER/u); + assert.doesNotMatch(stderr, /Interrupted/u, 'no error reaches the person'); +}); + +test('a marker whose write is interrupted twice in a row still arrives exactly once (resizes come in bursts)', {skip}, () => { + const {stdout, stderr} = run(MARK, ']777;nmsh;QA;', 2); + assert.equal(count(stdout, MARKER), 1); + assert.match(stdout, /QA-AFTER/u); + assert.doesNotMatch(stderr, /Interrupted/u, 'no error reaches the person'); +}); + +test('a marker that cannot be written is given up after a bounded number of attempts: no error text, no hang', {skip}, () => { + const {stdout, stderr} = run(MARK, ']777;nmsh;QA;', 50); + assert.equal(count(stdout, MARKER), major < 4 ? 0 : 1); + assert.match(stdout, /QA-AFTER/u, 'the command goes on'); + assert.doesNotMatch(stderr, /Interrupted/u, 'no error reaches the person'); +}); From eef10fd09977afcc2f895953c112685bbf277fc3 Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:50:20 +0530 Subject: [PATCH 2/2] Fish integration: on Fish 3, markers go through cat, so an interrupted write never loses one (#443) The fault-injection test (f909e778) failed on CI's Ubuntu Fish 3.7.0 as expected for two interruptions in a row, and showed more: even when #445's single retry saved the marker, Fish 3 printed "write: Interrupted system call" into the transcript. Fish 3 reports a failed builtin write from its own output layer on its own stderr, so the builtin's 2>/dev/null never silenced it (#443's strace shows the same). On Fish 3 a marker is now printf'd into a pipe (a few bytes, never blocked, so never interrupted) and written to the terminal by cat, which ignores SIGWINCH and retries an interrupted write. The marker travels on stdin, never in arguments. Fish 4 retries by itself and keeps the builtin. The cost is one short cat per marker, on Fish 3 only. The test now runs Fish in a real terminal and fails only terminal writes, as the real signal does (a pipe write of a few bytes never waits). A characterisation test runs #445's helper under the same faults: on Fish 3 it shows the error and loses a marker interrupted twice. Command output from Fish's own builtins is still lost on Fish 3 (the shim test documents it); NMSh cannot retry a person's command for them. --- src/shell/adapters/FishAdapter.ts | 19 ++++++-- tests/fishMarkerWrite.test.ts | 77 ++++++++++++++++++------------- 2 files changed, 59 insertions(+), 37 deletions(-) diff --git a/src/shell/adapters/FishAdapter.ts b/src/shell/adapters/FishAdapter.ts index f947aaa4..7ace0b91 100644 --- a/src/shell/adapters/FishAdapter.ts +++ b/src/shell/adapters/FishAdapter.ts @@ -30,12 +30,21 @@ const FISH_BUILTINS = new Set(['and', 'begin', 'bg', 'bind', 'block', 'break', ' 'time', 'true', 'type', 'ulimit', 'wait', 'while', 'abbr', 'argparse']); /** - * 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. + * 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 = `function __nmsh_mark - printf $argv 2>/dev/null; or printf $argv +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 { diff --git a/tests/fishMarkerWrite.test.ts b/tests/fishMarkerWrite.test.ts index a241e093..5bf9c09e 100644 --- a/tests/fishMarkerWrite.test.ts +++ b/tests/fishMarkerWrite.test.ts @@ -1,6 +1,7 @@ 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'; @@ -9,10 +10,11 @@ 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 write() fail with exactly that error, a - * set number of times, for a write that contains a given text; nothing depends on when a signal lands. + * 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. + * 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); @@ -32,7 +34,7 @@ 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 && memmem(buffer, length, match, strlen(match))) { remaining -= 1; errno = EINTR; return -1; } + 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); } `; @@ -50,44 +52,55 @@ function build(): string { } test.after(() => { if (root) rmSync(root, {recursive: true, force: true}); }); -/** Run a Fish script with NMSh's marker function; the first `fails` writes containing `match` fail with EINTR. */ -function run(script: string, match: string, fails: number) { - const result = spawnSync(fish!, ['--no-config', '-c', `${FISH_MARK_FUNCTION}\n${script}`], {encoding: 'utf8', timeout: 20_000, - env: {...process.env, LD_PRELOAD: build(), QA_EINTR_MATCH: match, QA_EINTR_FAILS: String(fails)}}); - assert.equal(result.error, undefined, 'fish ran and ended'); - return {stdout: result.stdout, stderr: result.stderr}; +/** + * 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 { + 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}); + 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 write in Fish fails as #443\'s trace did', {skip}, () => { - const {stdout, stderr} = run('echo QA-OUTPUT', 'QA-OUTPUT', 1); +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(stdout.includes('QA-OUTPUT'), false, 'Fish 3 drops the interrupted write'); - assert.match(stderr, /Interrupted system call/u); - } else assert.match(stdout, /QA-OUTPUT/u, 'Fish 4 retries an interrupted write'); + 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'); }); -test('a marker whose write is interrupted once arrives exactly once', {skip}, () => { - const {stdout, stderr} = run(MARK, ']777;nmsh;QA;', 1); - assert.equal(count(stdout, MARKER), 1); - assert.match(stdout, /QA-AFTER/u); - assert.doesNotMatch(stderr, /Interrupted/u, 'no error reaches the person'); -}); +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'); + }); +} -test('a marker whose write is interrupted twice in a row still arrives exactly once (resizes come in bursts)', {skip}, () => { - const {stdout, stderr} = run(MARK, ']777;nmsh;QA;', 2); - assert.equal(count(stdout, MARKER), 1); - assert.match(stdout, /QA-AFTER/u); - assert.doesNotMatch(stderr, /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('a marker that cannot be written is given up after a bounded number of attempts: no error text, no hang', {skip}, () => { - const {stdout, stderr} = run(MARK, ']777;nmsh;QA;', 50); - assert.equal(count(stdout, MARKER), major < 4 ? 0 : 1); - assert.match(stdout, /QA-AFTER/u, 'the command goes on'); - assert.doesNotMatch(stderr, /Interrupted/u, 'no error reaches the person'); +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'); });