Files
orca/src/shared/fish-query-reply-child-stdin.node-pty.test.ts
NeilandBrennan c92f394cde fix(pty): delete the reply-withholding scheduler (#15578)
* fix(pty): answer a terminal colour query in its own turn

Root-cause follow-up to #15559, which stopped a CPR overtaking a deferred
colour reply but left the deferral itself in place.

Orca answers terminal queries by writing to the PTY master, which a line
discipline in ECHO copies straight back out as junk on a cooked prompt
(#12112). The guard was to withhold the write until an `stty` subprocess
proved ECHO clear — and forking is what forced the decision to be async.
Any deferral, however short, lets a reply written later in the same turn
overtake this one, so the async probe was the bug's root cause.

Read the bit synchronously instead. Linux and the BSDs redirect a
master's mode ioctls to the slave, so a `tcgetattr` on the master fd
node-pty already owns answers for the slave with no fork: measured 0.26us
against 2403us for the subprocess. With a verdict available inline, a
querying program that already cleared ECHO — every raw-mode prober,
including the colour probe behind the `gh auth login` report — is
answered in its own turn and can never be reordered.

The deferral stays for the genuinely cooked case, and the ordering
guarantee stays underneath it: hosts whose node-pty predates this patch
get no sync probe and fall back to the deferred path, which mixed
client/host versions make a live production path.

Reply routing is all-or-nothing: a payload needing neither containment
nor ordering stays on the host's own path, so a CPR answered during shell
startup cannot pass the daemon's post-ready flush gate and splice into
the buffered startup command.

Native side is fail-safe: a kernel that did not redirect would answer
from the master's own termios, whose ECHO defaults set, so the degraded
verdict is "echoing" — never a false "quiet". The JS half ships in the
pnpm patch while the binding needs a source build, so
ORCA_REQUIRE_NODE_PTY_ECHO_STATE=1 makes CI fail rather than silently
skip when it is handed an upstream prebuild.

Co-authored-by: Brennan <brennanb2025@users.noreply.github.com>

* fix(pty): keep the flush ordered under synchronous re-entry

Three defects found in external review of the reply-ordering work.

node-pty delivers onData inside the master write, so a query can be
answered while the queue is mid-flush. `flushPendingWrites` spliced the
array off before writing, so that reply saw an empty queue, took the
same-turn path, and landed ahead of entries the loop had not written yet
— reproduced as 01, 99, 02, 03. It now shifts one entry at a time so a
re-entrant reply queues behind the rest, bounded by the length at entry
so a re-entrant push cannot spin the loop.

An overflow flush can re-enter as far as teardown. `answer` did not
re-check `closed` afterwards, so it queued behind a closed delivery,
returned true, and the reply was never written and never reported.

The payload router's ownership comment overstated its guarantee. The
`any` semantics are deliberate — returning false after a constituent was
already written would have the caller re-write the whole payload and
duplicate it into the child's stdin — so the residual mixed-failure drop
is now documented rather than implied away.

* fix(pty): delete the reply-withholding scheduler

Orca answered a terminal query by withholding the write until a probe
proved the slave's ECHO bit was clear. That was the wrong mechanism, and
it is now gone: replies are written in the caller's turn and their echo
is contained on the output side, where it always was.

Withholding never removed an echo. The wait was bounded and always ended
in a write, so the output-side projections were doing the work the whole
time — including the readline rewrite, which happens with the tty already
raw and which therefore no reading of the ECHO bit can predict. What
withholding did add was an asynchronous write path, and that is what let
one reply overtake another and land in the next program's stdin (#15559),
what produced a re-entrancy inversion inside its own flush, and what four
rounds of regressions have lived in.

The last thing it covered was the verbatim echo of a `stty -echoctl` tty.
That shape is now projected directly. It starts with ESC, so it is
matched only when complete and never held as a partial: holding it would
take a bare trailing ESC from the query parser and an expired hold would
release it raw, so a query torn at its own ESC would never be answered.
Complete-match-only is what makes the shape safe to project at all.

Measured on a real pty: a cooked-mode master write is both echoed AND
delivered — ECHO copies the bytes without consuming them from the slave's
input queue, so a program arming raw mode with TCSANOW/TCSADRAIN (libuv's
setRawMode, hence every Node agent) still reads them. Only a TCSAFLUSH
switcher discards it, which it does on every terminal, none of which
gates a reply on termios state.

Deletes the pending-write queue, the async stty probe, the poll budget
and probe rate limit, the deadline-driven flush, and the answer/
answerInOrder split. Replies now leave in call order by construction.
No packaging, native or CI surface is touched.

* test(pty): restore stty-probe coverage and pin the duplicate-query retry

Archaeology on how withholding got here, and what its tests were really
protecting.

Deleting the ECHO probe took four tests with it that were not about the
probe at all: they cover createSttyProbe, which the shell-readiness
line-editor probe still uses — in-flight sharing, the per-platform stty
flag, and transient-versus-permanent failure latching. Restored against
the line-editor probe, which is now their only caller.

Also pins the property that answers the one case an immediate write
cannot serve. A program that queries while cooked and then arms raw mode
with TCSAFLUSH discards the reply with the rest of its input queue.
Nothing can prevent that from the terminal side, and no terminal tries.
What matters is that such a program re-queries after its own timeout: the
ingress declines to answer an already-answered slot but forwards the
duplicate downstream, so the renderer's emulator answers the retry, by
which point the program is raw. The retry path is the recovery, not
withholding.

* ci(pty): keep the fish real-PTY test in the shell-contracts lane only

Reverting pr.yml to main dropped the exclusion for the fish query-reply
test, which this branch keeps, so it would have run in the sharded lane
as well. Restores it to the shell-contracts include list and the shard
exclude list, and drops the parallelism expectations for the deleted
cooked-querier suite and the echo-state env guard.

---------

Co-authored-by: Brennan <brennanb2025@users.noreply.github.com>
2026-08-20 02:15:42 -07:00

226 lines
8.2 KiB
TypeScript
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
/**
* Real-fish regression for #13892: a terminal query reply Orca held back is overtaken
* by the DA1 answer written later in the same turn, so fish's read sentinel hands the
* tty to the child while the OSC 11 reply is still queued — and the CHILD READS IT.
*
* What is real here: node-pty running fish, `PtyStartupIngress`, `PtyStartupReplyDelivery`
* and both echo probes, plus the host's own write gate
* (`answerLiveQueryReply` — copied from local-pty-provider.write, which
* is what decides whether a reply is deferred at all). Only the renderer is modelled: it
* answers queries strictly in the order they appear in the stream, so any inversion the
* child sees was produced by the delivery split and nothing else.
*
* The assertion is about a CHILD PROCESS'S STDIN, not the screen: a rendered-output check
* passes while the bytes are still being eaten by the next `npx` / `brew` confirm prompt.
* The child reads a full LINE because a leaked reply carries no newline, so a
* once('data') child would report it alone whenever it happened to land in its own read.
*/
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
import { tmpdir } from 'node:os'
import path from 'node:path'
import { afterEach, describe, expect, it } from 'vitest'
import { fishRequirementViolation, resolveFishBinary } from './fish-binary-requirement'
import { PtyStartupIngress } from './pty-startup-ingress'
// Why fish 4: the DA1-sentinel handoff this measures lives in the 4.0 Rust tty_handoff.
const FISH = resolveFishBinary(4)
const itWithFish = FISH.available ? it : it.skip
const PROMPT_MARK = 'ORCA13892> '
/* oxlint-disable no-control-regex -- terminal query grammars are control sequences */
/** Anchored at an ESC, first match wins; reply values match xterm.js's. */
const QUERY_GRAMMARS = [
{
re: /^\x1b\]1[012];\?(\x07|\x1b\\)/,
reply: (m: RegExpExecArray) => `\x1b]11;rgb:1e1e/1e1e/1e1e${m[1]}`
},
{ re: /^\x1b\[\?6n/, reply: () => '\x1b[?1;1;1R' },
{ re: /^\x1b\[6n/, reply: () => '\x1b[1;1R' },
{ re: /^\x1b\[\?996n/, reply: () => '\x1b[?997;1n' },
{ re: /^\x1b\[>0?c/, reply: () => '\x1b[>0;276;0c' },
{ re: /^\x1b\[0?c/, reply: () => '\x1b[?1;2c' },
{ re: /^\x1b\[>0?q/, reply: () => '\x1bP>|Orca\x1b\\' },
{ re: /^\x1b\[\?u/, reply: () => '\x1b[?0u' }
] as const
/** Still accumulating: no CSI final byte and no OSC/DCS terminator yet. */
const PARTIAL_QUERY_RE =
/^(?:\x1b|\x1b\[[?>=]?[0-9;]*|\x1b\][0-9]*(?:;[^\x07\x1b]*)?\x1b?|\x1bP[^\x1b]*\x1b?)$/
/* oxlint-enable no-control-regex */
const sleep = (ms: number): Promise<void> => new Promise((resolve) => setTimeout(resolve, ms))
async function waitUntil(predicate: () => boolean, timeoutMs: number): Promise<boolean> {
const deadline = Date.now() + timeoutMs
while (Date.now() < deadline) {
if (predicate()) {
return true
}
await sleep(10)
}
return false
}
describe('a held query reply never reaches the next child process (#13892)', () => {
let configHome: string | null = null
// Always runs, so the CI lane cannot report green with the regression below skipped.
it('has the fish this suite needs when CI requires one', () => {
expect(fishRequirementViolation(FISH)).toBeNull()
})
// Same contract for the other half of the setup: a prebuilt node-pty has no echoState,
// so the fix under test would be off and the regression below would run vacuously.
afterEach(() => {
if (configHome) {
rmSync(configHome, { recursive: true, force: true })
configHome = null
}
})
itWithFish(
'answers OSC 11 in the query turn so the reply cannot land in the child’s stdin',
async () => {
configHome = mkdtempSync(path.join(tmpdir(), 'orca-fish-13892-'))
mkdirSync(path.join(configHome, 'fish'), { recursive: true })
writeFileSync(
path.join(configHome, 'fish/config.fish'),
[
'set -g fish_greeting ""',
`function fish_prompt; printf '${PROMPT_MARK}'; end`,
'function fish_right_prompt; end',
''
].join('\n')
)
const childScript = path.join(configHome, 'read-stdin.mjs')
writeFileSync(
childScript,
"let buffered = ''\n" +
"process.stdin.on('data', (d) => {\n" +
" buffered += d.toString('utf8')\n" +
" if (!buffered.includes('\\n')) return\n" +
" process.stdout.write('CHILD-READ:' + JSON.stringify(buffered) + '\\n')\n" +
' process.exit(0)\n' +
'})\n'
)
const nodePty = await import('node-pty')
const term = nodePty.spawn(FISH.path as string, ['-l', '-i'], {
name: 'xterm-256color',
cols: 120,
rows: 30,
cwd: configHome,
env: {
PATH: process.env.PATH ?? '/usr/bin:/bin',
HOME: configHome,
TERM: 'xterm-256color',
COLORTERM: 'truecolor',
LANG: 'en_US.UTF-8',
XDG_CONFIG_HOME: configHome,
XDG_DATA_HOME: path.join(configHome, 'data'),
ORCA_NODE_BIN: process.execPath,
ORCA_CHILD_SCRIPT: childScript
}
})
let rendered = ''
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
write: (data) => term.write(data),
onEmission: (emission) => {
rendered += emission.data
answerQueriesInOrder(emission.data)
}
})
// The host gate: cooked-echo-risk replies are written by the ingress with their
// echo shapes armed; DA1/CPR stay on the host's own path, in call order.
const hostWrite = (data: string): void => {
if (ingress.answerLiveQueryReply(data)) {
return
}
term.write(data)
}
let tail = ''
let oscQueryCount = 0
function answerQueriesInOrder(chunk: string): void {
let buffer = tail + chunk
tail = ''
let index = 0
while (index < buffer.length) {
const at = buffer.indexOf('\x1b', index)
if (at === -1) {
return
}
const rest = buffer.slice(at)
const grammar = QUERY_GRAMMARS.map((candidate) => ({
candidate,
match: candidate.re.exec(rest)
})).find((entry) => entry.match)
if (grammar?.match) {
if (grammar.candidate === QUERY_GRAMMARS[0]) {
oscQueryCount += 1
}
hostWrite(grammar.candidate.reply(grammar.match))
index = at + grammar.match[0].length
continue
}
if (PARTIAL_QUERY_RE.test(rest)) {
tail = rest
return
}
index = at + 1
}
}
let exited = false
term.onExit(() => {
exited = true
})
term.onData((data) => ingress.accept(data))
try {
expect(await waitUntil(() => rendered.includes(PROMPT_MARK), 15_000)).toBe(true)
await sleep(500)
const oscQueriesBeforeHandoff = oscQueryCount
// Type-ahead is the deterministic shape: queue the child's command while an
// external command still owns the tty, so fish repaints its prompt (re-querying
// OSC 11) and hands the tty over in the same breath.
term.write('sleep 0.4\r')
await sleep(150)
term.write('"$ORCA_NODE_BIN" "$ORCA_CHILD_SCRIPT"\r')
await sleep(1_500)
expect(oscQueryCount).toBeGreaterThan(oscQueriesBeforeHandoff)
const renderedBeforeChildInput = rendered.length
term.write('hello\r')
expect(
await waitUntil(
() => rendered.slice(renderedBeforeChildInput).includes('CHILD-READ:'),
10_000
)
).toBe(true)
const childRead =
rendered.slice(renderedBeforeChildInput).match(/CHILD-READ:[^\r\n]*/)?.[0] ?? ''
// The merge-blocking assertion: the child's first LINE is what the user typed,
// with no escape byte in front of it. Pre-fix this reads
// `\u001b]11;rgb:1e1e/1e1e/1e1e\u001b\\hello\n`.
expect(childRead).toBe('CHILD-READ:"hello\\n"')
} finally {
term.write('exit\r')
await waitUntil(() => exited, 3_000)
try {
term.kill()
} catch {
// already gone
}
}
},
45_000
)
})