From 9dc52725ecbfabe9bba234ea2b9729b70264b8bc Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 18:56:54 +0000 Subject: [PATCH 01/28] try keeping user fd alive longer --- index.d.ts | 1 + src/lib.rs | 24 +++++++++++------------- tests/index.test.ts | 21 +++++++++++++++++++++ wrapper.ts | 20 +++++++++++++++++--- 4 files changed, 50 insertions(+), 16 deletions(-) diff --git a/index.d.ts b/index.d.ts index 77679869..9c663d43 100644 --- a/index.d.ts +++ b/index.d.ts @@ -67,4 +67,5 @@ export declare class Pty { * descriptor. */ takeFd(): c_int; + closeUserFd(): void; } diff --git a/src/lib.rs b/src/lib.rs index a75c33ed..bc504323 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,7 +17,7 @@ use napi::Status::GenericFailure; use napi::{self, Env}; use nix::errno::Errno; use nix::fcntl::{fcntl, FcntlArg, FdFlag, OFlag}; -use nix::libc::{self, c_int, ioctl, FIONREAD, TIOCOUTQ, TIOCSCTTY, TIOCSWINSZ}; +use nix::libc::{self, c_int, ioctl, FIONREAD, TIOCSCTTY, TIOCSWINSZ}; use nix::pty::{openpty, Winsize}; use nix::sys::termios::{self, SetArg}; @@ -31,6 +31,7 @@ mod sandbox; #[allow(dead_code)] struct Pty { controller_fd: Option, + user_fd: Option, /// The pid of the forked process. pub pid: u32, } @@ -107,9 +108,7 @@ fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { loop { // check both input and output queues for both FDs let mut controller_inq: i32 = 0; - let mut controller_outq: i32 = 0; let mut user_inq: i32 = 0; - let mut user_outq: i32 = 0; // safe because we're passing valid file descriptors and properly sized integers unsafe { @@ -120,18 +119,10 @@ fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { // break if we can't read break; } - - // check bytes waiting to be written (TIOCOUTQ) - if ioctl(controller_fd, TIOCOUTQ, &mut controller_outq) == -1 - || ioctl(user_fd, TIOCOUTQ, &mut user_outq) == -1 - { - // break if we can't read - break; - } } // if all queues are empty, we're done - if controller_inq == 0 && controller_outq == 0 && user_inq == 0 && user_outq == 0 { + if controller_inq == 0 && user_inq == 0 { break; } @@ -349,7 +340,6 @@ impl Pty { // try to wait for the controller fd to be fully read poll_pty_fds_until_read(raw_controller_fd, raw_user_fd); - drop(user_fd); match wait_result { Ok(status) => { @@ -379,6 +369,7 @@ impl Pty { Ok(Pty { controller_fd: Some(controller_fd), + user_fd: Some(user_fd), pid, }) } @@ -398,6 +389,13 @@ impl Pty { )) } } + + #[napi] + #[allow(dead_code)] + pub fn close_user_fd(&mut self) -> Result<(), napi::Error> { + self.user_fd.take(); + Ok(()) + } } /// Resize the terminal. diff --git a/tests/index.test.ts b/tests/index.test.ts index ae87f5bb..47481c5b 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -354,6 +354,27 @@ describe('PTY', { repeats: 500 }, () => { expect(buffer.toString().length).toBe(payload.length); }); + test.only('doesnt miss lots of lines from bash', async () => { + const payload = Array.from({ length: 4096 * 5 }, (_, i) => i).join('\n'); + let buffer = Buffer.from(''); + const onExit = vi.fn(); + + const pty = new Pty({ + command: 'bash', + args: ['-c', `echo -n "${payload}"`], + onExit, + }); + + const readStream = pty.read; + readStream.on('data', (data) => { + buffer = Buffer.concat([buffer, data]); + }); + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.toString().trim().replace(/\r/g, '')).toBe(payload.trim()); + }); + testSkipOnDarwin('does not leak files', async () => { const oldFds = getOpenFds(); const promises = []; diff --git a/wrapper.ts b/wrapper.ts index 2ce68c50..72ddbbb3 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -78,7 +78,12 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { + console.log('mockedExit', { error, code }); markExited({ error, code }); + + setImmediate(() => { + this.#pty.closeUserFd(); + }); }; // when pty exits, we should wait until the fd actually ends (end OR error) @@ -105,8 +110,14 @@ export class Pty { realExit(result.error, result.code); }; - this.read.once('end', markReadFinished); - this.read.once('close', handleClose); + this.read.once('end', () => { + console.log('read end'); + markReadFinished(); + }); + this.read.once('close', () => { + console.log('read close'); + handleClose(); + }); // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as // cleaning up other spurious errors) so that the user doesn't need to handle them and be in @@ -135,7 +146,10 @@ export class Pty { throw err; }; - this.read.on('error', handleError); + this.read.on('error', (err) => { + console.error('error:', err); + handleError(err); + }); } close() { From 9fe9f50b2084cfecd8f6a9f28763e79185ad36de Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 19:01:51 +0000 Subject: [PATCH 02/28] allow only just to test --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 1ad2399e..7f5af468 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,7 @@ "build:wrapper": "tsup", "prepublishOnly": "napi prepublish -t npm", "test": "vitest run", - "test:ci": "vitest --reporter=verbose --reporter=github-actions run", + "test:ci": "vitest --reporter=verbose --reporter=github-actions --allowOnly run", "test:hang": "vitest run --reporter=hanging-process", "universal": "napi universal", "version": "napi version", From dc2c4a69b76d1d78303c4d38ad2a1997677ec3ea Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 19:10:35 +0000 Subject: [PATCH 03/28] try timeout?? --- tests/index.test.ts | 6 +++--- wrapper.ts | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 47481c5b..e878cf05 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -56,7 +56,7 @@ function getOpenFds(): FdRecord { return fds; } -describe('PTY', { repeats: 500 }, () => { +describe('PTY', { repeats: 0 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; @@ -355,7 +355,7 @@ describe('PTY', { repeats: 500 }, () => { }); test.only('doesnt miss lots of lines from bash', async () => { - const payload = Array.from({ length: 4096 * 5 }, (_, i) => i).join('\n'); + const payload = Array.from({ length: 5000 }, (_, i) => i).join('\n'); let buffer = Buffer.from(''); const onExit = vi.fn(); @@ -372,7 +372,7 @@ describe('PTY', { repeats: 500 }, () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); - expect(buffer.toString().trim().replace(/\r/g, '')).toBe(payload.trim()); + expect(buffer.toString().trim().replace(/\r/g, '').length).toBe(payload.length); }); testSkipOnDarwin('does not leak files', async () => { diff --git a/wrapper.ts b/wrapper.ts index 72ddbbb3..2efd3cfb 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -81,9 +81,10 @@ export class Pty { console.log('mockedExit', { error, code }); markExited({ error, code }); - setImmediate(() => { + // set immediate to give us one more chance to read any remaining data + setTimeout(() => { this.#pty.closeUserFd(); - }); + }, 100); }; // when pty exits, we should wait until the fd actually ends (end OR error) From 8b9718d3c3c665181ffa95b7af7b350d2d69c88e Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 19:13:22 +0000 Subject: [PATCH 04/28] repeat --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index e878cf05..176f4a44 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -56,7 +56,7 @@ function getOpenFds(): FdRecord { return fds; } -describe('PTY', { repeats: 0 }, () => { +describe('PTY', { repeats: 500 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; From a9f5090337db07eb0795c674d85f196aa1f9db94 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 19:34:59 +0000 Subject: [PATCH 05/28] pause style buffering --- wrapper.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index 2efd3cfb..09c662ff 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -81,10 +81,16 @@ export class Pty { console.log('mockedExit', { error, code }); markExited({ error, code }); - // set immediate to give us one more chance to read any remaining data - setTimeout(() => { + // try to read the last of the data before closing the fd + this.read.pause(); + let chunk: Buffer | null; + while ((chunk = this.read.read()) !== null) { + this.read.emit('data', chunk); + } + + setImmediate(() => { this.#pty.closeUserFd(); - }, 100); + }); }; // when pty exits, we should wait until the fd actually ends (end OR error) From fde37ce6ea4b65b9239f30537f9e191ea07a6381 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 19:41:01 +0000 Subject: [PATCH 06/28] more print debugging --- wrapper.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index 09c662ff..f66fae5d 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -85,6 +85,7 @@ export class Pty { this.read.pause(); let chunk: Buffer | null; while ((chunk = this.read.read()) !== null) { + console.log('boi has data') this.read.emit('data', chunk); } @@ -153,10 +154,7 @@ export class Pty { throw err; }; - this.read.on('error', (err) => { - console.error('error:', err); - handleError(err); - }); + this.read.on('error', handleError); } close() { From 6d42e4a525010a3059be9861168d76fd7743c9d0 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 20:06:56 +0000 Subject: [PATCH 07/28] even more print --- wrapper.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/wrapper.ts b/wrapper.ts index f66fae5d..c2b4fd6b 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -82,6 +82,7 @@ export class Pty { markExited({ error, code }); // try to read the last of the data before closing the fd + console.log('pausing') this.read.pause(); let chunk: Buffer | null; while ((chunk = this.read.read()) !== null) { @@ -90,6 +91,7 @@ export class Pty { } setImmediate(() => { + console.log('closing user fd') this.#pty.closeUserFd(); }); }; From 66db8668be09e314885f3d2b25b6f2bb52976379 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 20:49:40 +0000 Subject: [PATCH 08/28] cursed split appraoch --- index.d.ts | 1 + src/lib.rs | 86 +++++++++++++++++++-------------------------- tests/index.test.ts | 11 ++++-- wrapper.ts | 48 ++++++++++++++++++------- 4 files changed, 83 insertions(+), 63 deletions(-) diff --git a/index.d.ts b/index.d.ts index 9c663d43..e6182f4c 100644 --- a/index.d.ts +++ b/index.d.ts @@ -61,6 +61,7 @@ export declare class Pty { /** The pid of the forked process. */ pid: number; constructor(opts: PtyOptions); + areFdsEmpty(controllerFd: c_int): boolean; /** * Transfers ownership of the file descriptor for the PTY controller. This can only be called * once (it will error the second time). The caller is responsible for closing the file diff --git a/src/lib.rs b/src/lib.rs index bc504323..676903ec 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,10 +7,7 @@ use std::os::fd::{FromRawFd, IntoRawFd, RawFd}; use std::os::unix::process::CommandExt; use std::process::{Command, Stdio}; use std::thread; -use std::time::Duration; -use backoff::backoff::Backoff; -use backoff::ExponentialBackoffBuilder; use napi::bindgen_prelude::JsFunction; use napi::threadsafe_function::{ErrorStrategy, ThreadsafeFunction, ThreadsafeFunctionCallMode}; use napi::Status::GenericFailure; @@ -94,49 +91,6 @@ fn cast_to_napi_error(err: Errno) -> napi::Error { napi::Error::new(GenericFailure, err) } -// if the child process exits before the controller fd is fully read or the user fd is fully -// flushed, we might accidentally end in a case where onExit is called but js hasn't had -// the chance to fully read the controller fd -// let's wait until the controller fd is fully read before we call onExit -fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { - let mut backoff = ExponentialBackoffBuilder::default() - .with_initial_interval(Duration::from_millis(1)) - .with_max_interval(Duration::from_millis(100)) - .with_max_elapsed_time(Some(Duration::from_secs(1))) - .build(); - - loop { - // check both input and output queues for both FDs - let mut controller_inq: i32 = 0; - let mut user_inq: i32 = 0; - - // safe because we're passing valid file descriptors and properly sized integers - unsafe { - // check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) - if ioctl(controller_fd, FIONREAD, &mut controller_inq) == -1 - || ioctl(user_fd, FIONREAD, &mut user_inq) == -1 - { - // break if we can't read - break; - } - } - - // if all queues are empty, we're done - if controller_inq == 0 && user_inq == 0 { - break; - } - - // apply backoff strategy - if let Some(d) = backoff.next_backoff() { - thread::sleep(d); - continue; - } else { - // we have exhausted our attempts - break; - } - } -} - #[napi] impl Pty { #[napi(constructor)] @@ -338,9 +292,6 @@ impl Pty { thread::spawn(move || { let wait_result = child.wait(); - // try to wait for the controller fd to be fully read - poll_pty_fds_until_read(raw_controller_fd, raw_user_fd); - match wait_result { Ok(status) => { if status.success() { @@ -374,6 +325,43 @@ impl Pty { }) } + // if the child process exits before the controller fd is fully read or the user fd is fully + // flushed, we might accidentally end in a case where onExit is called but js hasn't had + // the chance to fully read the controller fd + // let's wait until the controller fd is fully read before we call onExit + #[napi] + #[allow(dead_code)] + pub fn are_fds_empty(&self, controller_fd: c_int) -> Result { + let user_fd = if let Some(fd) = &self.user_fd { + fd.as_raw_fd() + } else { + return Err(napi::Error::new( + napi::Status::GenericFailure, + "fd failed: bad file descriptor (os error 9)", + )); + }; + + // check both input and output queues for both FDs + let mut controller_inq: i32 = 0; + let mut user_inq: i32 = 0; + + // safe because we're passing valid file descriptors and properly sized integers + unsafe { + // check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) + if ioctl(controller_fd, FIONREAD, &mut controller_inq) == -1 + || ioctl(user_fd, FIONREAD, &mut user_inq) == -1 + { + return Err(napi::Error::new( + napi::Status::GenericFailure, + format!("ioctl FIONREAD failed: {}", Error::last_os_error()), + )); + } + } + + // if all queues are empty, we're done + Ok(controller_inq == 0 && user_inq == 0) + } + /// Transfers ownership of the file descriptor for the PTY controller. This can only be called /// once (it will error the second time). The caller is responsible for closing the file /// descriptor. diff --git a/tests/index.test.ts b/tests/index.test.ts index 176f4a44..c29e0227 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -371,8 +371,15 @@ describe('PTY', { repeats: 500 }, () => { }); await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); - expect(onExit).toHaveBeenCalledWith(null, 0); - expect(buffer.toString().trim().replace(/\r/g, '').length).toBe(payload.length); + try { + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.toString().trim().replace(/\r/g, '').length).toBe( + payload.length, + ); + } catch (e) { + console.log('FAIL'); + throw e; + } }); testSkipOnDarwin('does not leak files', async () => { diff --git a/wrapper.ts b/wrapper.ts index c2b4fd6b..eaabfc4a 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -23,6 +23,19 @@ type ExitResult = { code: number; }; +async function retryWithBackoff(fn: T) { + for (let attempt = 0; attempt <= 4; attempt++) { + try { + return fn(); + } catch (error) { + if (attempt === 4) throw error; + + const delay = Math.pow(10, attempt); + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } +} + /** * A very thin wrapper around PTYs and processes. * @@ -77,23 +90,34 @@ export class Pty { let readFinished = new Promise((resolve) => { markReadFinished = resolve; }); - const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { + const mockedExit = async ( + error: NodeJS.ErrnoException | null, + code: number, + ) => { console.log('mockedExit', { error, code }); markExited({ error, code }); - // try to read the last of the data before closing the fd - console.log('pausing') - this.read.pause(); - let chunk: Buffer | null; - while ((chunk = this.read.read()) !== null) { - console.log('boi has data') - this.read.emit('data', chunk); - } + // poll until the fds are empty before we close + await retryWithBackoff(() => { + // this effectively checks FIONREAD for the controller side + const bytesAvailable = this.#socket.readableLength; + console.log('bytesAvailable', bytesAvailable); + if (bytesAvailable > 0) { + throw new Error('still data to read'); + } - setImmediate(() => { - console.log('closing user fd') - this.#pty.closeUserFd(); + // more expensive check second + const fdsEmpty = this.#pty.areFdsEmpty(this.#fd); + console.log('fdsEmpty', fdsEmpty); + if (!fdsEmpty) { + throw new Error('fds not empty yet'); + } }); + + console.log('yay done') + + // try to read the last of the data before closing the fd + this.#pty.closeUserFd(); }; // when pty exits, we should wait until the fd actually ends (end OR error) From 07bac5ab92113c8ae3fa6f7e7e2457568293b8e0 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 21:05:25 +0000 Subject: [PATCH 09/28] try with stability period --- index.d.ts | 1 + src/lib.rs | 37 ------------------------------------ tests/index.test.ts | 2 +- wrapper.ts | 46 +++------------------------------------------ 4 files changed, 5 insertions(+), 81 deletions(-) diff --git a/index.d.ts b/index.d.ts index e6182f4c..cafc741d 100644 --- a/index.d.ts +++ b/index.d.ts @@ -36,6 +36,7 @@ export interface PtyOptions { apparmorProfile?: string; interactive?: boolean; sandbox?: SandboxOptions; + exitOutputStabilityPeriod?: number; onExit: (err: null | Error, exitCode: number) => void; } /** A size struct to pass to resize. */ diff --git a/src/lib.rs b/src/lib.rs index 676903ec..a9a4896d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -325,43 +325,6 @@ impl Pty { }) } - // if the child process exits before the controller fd is fully read or the user fd is fully - // flushed, we might accidentally end in a case where onExit is called but js hasn't had - // the chance to fully read the controller fd - // let's wait until the controller fd is fully read before we call onExit - #[napi] - #[allow(dead_code)] - pub fn are_fds_empty(&self, controller_fd: c_int) -> Result { - let user_fd = if let Some(fd) = &self.user_fd { - fd.as_raw_fd() - } else { - return Err(napi::Error::new( - napi::Status::GenericFailure, - "fd failed: bad file descriptor (os error 9)", - )); - }; - - // check both input and output queues for both FDs - let mut controller_inq: i32 = 0; - let mut user_inq: i32 = 0; - - // safe because we're passing valid file descriptors and properly sized integers - unsafe { - // check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) - if ioctl(controller_fd, FIONREAD, &mut controller_inq) == -1 - || ioctl(user_fd, FIONREAD, &mut user_inq) == -1 - { - return Err(napi::Error::new( - napi::Status::GenericFailure, - format!("ioctl FIONREAD failed: {}", Error::last_os_error()), - )); - } - } - - // if all queues are empty, we're done - Ok(controller_inq == 0 && user_inq == 0) - } - /// Transfers ownership of the file descriptor for the PTY controller. This can only be called /// once (it will error the second time). The caller is responsible for closing the file /// descriptor. diff --git a/tests/index.test.ts b/tests/index.test.ts index c29e0227..b9db1741 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -354,7 +354,7 @@ describe('PTY', { repeats: 500 }, () => { expect(buffer.toString().length).toBe(payload.length); }); - test.only('doesnt miss lots of lines from bash', async () => { + test('doesnt miss lots of lines from bash', async () => { const payload = Array.from({ length: 5000 }, (_, i) => i).join('\n'); let buffer = Buffer.from(''); const onExit = vi.fn(); diff --git a/wrapper.ts b/wrapper.ts index eaabfc4a..50fd52fc 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -23,19 +23,6 @@ type ExitResult = { code: number; }; -async function retryWithBackoff(fn: T) { - for (let attempt = 0; attempt <= 4; attempt++) { - try { - return fn(); - } catch (error) { - if (attempt === 4) throw error; - - const delay = Math.pow(10, attempt); - await new Promise((resolve) => setTimeout(resolve, delay)); - } - } -} - /** * A very thin wrapper around PTYs and processes. * @@ -94,29 +81,8 @@ export class Pty { error: NodeJS.ErrnoException | null, code: number, ) => { - console.log('mockedExit', { error, code }); markExited({ error, code }); - - // poll until the fds are empty before we close - await retryWithBackoff(() => { - // this effectively checks FIONREAD for the controller side - const bytesAvailable = this.#socket.readableLength; - console.log('bytesAvailable', bytesAvailable); - if (bytesAvailable > 0) { - throw new Error('still data to read'); - } - - // more expensive check second - const fdsEmpty = this.#pty.areFdsEmpty(this.#fd); - console.log('fdsEmpty', fdsEmpty); - if (!fdsEmpty) { - throw new Error('fds not empty yet'); - } - }); - - console.log('yay done') - - // try to read the last of the data before closing the fd + await new Promise((r) => setTimeout(r, options.exitOutputStabilityPeriod ?? 50)); this.#pty.closeUserFd(); }; @@ -144,14 +110,8 @@ export class Pty { realExit(result.error, result.code); }; - this.read.once('end', () => { - console.log('read end'); - markReadFinished(); - }); - this.read.once('close', () => { - console.log('read close'); - handleClose(); - }); + this.read.once('end', markReadFinished); + this.read.once('close',handleClose); // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as // cleaning up other spurious errors) so that the user doesn't need to handle them and be in From 0e60642afa7eeac5f7a4f8b80b5d2716694bce60 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 21:11:53 +0000 Subject: [PATCH 10/28] oops fix option --- wrapper.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index 50fd52fc..f094fcb7 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -23,6 +23,10 @@ type ExitResult = { code: number; }; +interface FullPtyOptions extends PtyOptions { + exitOutputStabilityPeriod?: number; +} + /** * A very thin wrapper around PTYs and processes. * @@ -65,7 +69,7 @@ export class Pty { return this.#socket; } - constructor(options: PtyOptions) { + constructor(options: FullPtyOptions) { const realExit = options.onExit; let markExited!: (value: ExitResult) => void; @@ -82,7 +86,9 @@ export class Pty { code: number, ) => { markExited({ error, code }); - await new Promise((r) => setTimeout(r, options.exitOutputStabilityPeriod ?? 50)); + await new Promise((r) => + setTimeout(r, options.exitOutputStabilityPeriod ?? 50), + ); this.#pty.closeUserFd(); }; @@ -111,7 +117,7 @@ export class Pty { }; this.read.once('end', markReadFinished); - this.read.once('close',handleClose); + this.read.once('close', handleClose); // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as // cleaning up other spurious errors) so that the user doesn't need to handle them and be in From 156c4dbbcd8531727a8838bd1c60b9e6dc56874a Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 21:33:08 +0000 Subject: [PATCH 11/28] oops fix the type defs --- index.d.ts | 2 -- src/lib.rs | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/index.d.ts b/index.d.ts index cafc741d..9c663d43 100644 --- a/index.d.ts +++ b/index.d.ts @@ -36,7 +36,6 @@ export interface PtyOptions { apparmorProfile?: string; interactive?: boolean; sandbox?: SandboxOptions; - exitOutputStabilityPeriod?: number; onExit: (err: null | Error, exitCode: number) => void; } /** A size struct to pass to resize. */ @@ -62,7 +61,6 @@ export declare class Pty { /** The pid of the forked process. */ pid: number; constructor(opts: PtyOptions); - areFdsEmpty(controllerFd: c_int): boolean; /** * Transfers ownership of the file descriptor for the PTY controller. This can only be called * once (it will error the second time). The caller is responsible for closing the file diff --git a/src/lib.rs b/src/lib.rs index a9a4896d..283cd691 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,7 +14,7 @@ use napi::Status::GenericFailure; use napi::{self, Env}; use nix::errno::Errno; use nix::fcntl::{fcntl, FcntlArg, FdFlag, OFlag}; -use nix::libc::{self, c_int, ioctl, FIONREAD, TIOCSCTTY, TIOCSWINSZ}; +use nix::libc::{self, c_int, TIOCSCTTY, TIOCSWINSZ}; use nix::pty::{openpty, Winsize}; use nix::sys::termios::{self, SetArg}; From 989f76bd21021ffbe7428ac4cd22e0c99720de89 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 21:50:44 +0000 Subject: [PATCH 12/28] 3.6.0 --- npm/darwin-arm64/package.json | 2 +- npm/darwin-x64/package.json | 2 +- npm/linux-x64-gnu/package.json | 2 +- package-lock.json | 4 ++-- package.json | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/npm/darwin-arm64/package.json b/npm/darwin-arm64/package.json index bf9f0bf5..c72485d4 100644 --- a/npm/darwin-arm64/package.json +++ b/npm/darwin-arm64/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty-darwin-arm64", - "version": "3.5.3", + "version": "3.6.0", "os": [ "darwin" ], diff --git a/npm/darwin-x64/package.json b/npm/darwin-x64/package.json index 8dceb4ea..d0396f5d 100644 --- a/npm/darwin-x64/package.json +++ b/npm/darwin-x64/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty-darwin-x64", - "version": "3.5.3", + "version": "3.6.0", "os": [ "darwin" ], diff --git a/npm/linux-x64-gnu/package.json b/npm/linux-x64-gnu/package.json index a8baa81a..5a4bed3a 100644 --- a/npm/linux-x64-gnu/package.json +++ b/npm/linux-x64-gnu/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty-linux-x64-gnu", - "version": "3.5.3", + "version": "3.6.0", "os": [ "linux" ], diff --git a/package-lock.json b/package-lock.json index 2d1b14f4..f8c7190a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@replit/ruspty", - "version": "3.5.3", + "version": "3.6.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@replit/ruspty", - "version": "3.5.3", + "version": "3.6.0", "license": "MIT", "devDependencies": { "@napi-rs/cli": "^2.18.4", diff --git a/package.json b/package.json index 7f5af468..0fc4e87f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty", - "version": "3.5.3", + "version": "3.6.0", "main": "dist/wrapper.js", "types": "dist/wrapper.d.ts", "author": "Szymon Kaliski ", From 86225029d214e130d8c9166a04e11d4aab8ec254 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 23:08:07 +0000 Subject: [PATCH 13/28] remove try catch --- tests/index.test.ts | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index b9db1741..532f3596 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -371,15 +371,10 @@ describe('PTY', { repeats: 500 }, () => { }); await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); - try { - expect(onExit).toHaveBeenCalledWith(null, 0); - expect(buffer.toString().trim().replace(/\r/g, '').length).toBe( - payload.length, - ); - } catch (e) { - console.log('FAIL'); - throw e; - } + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.toString().trim().replace(/\r/g, '').length).toBe( + payload.length, + ); }); testSkipOnDarwin('does not leak files', async () => { From 86e0add4a756c40b842d9c494c66434673057648 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 25 Sep 2025 23:55:29 -0700 Subject: [PATCH 14/28] try synthetic eof? (#95) --- src/lib.rs | 12 ++++ syntheticEof.ts | 83 ++++++++++++++++++++++++ tests/index.test.ts | 2 +- tests/syntheticEOF.test.ts | 129 +++++++++++++++++++++++++++++++++++++ wrapper.ts | 60 +++++++++-------- 5 files changed, 254 insertions(+), 32 deletions(-) create mode 100644 syntheticEof.ts create mode 100644 tests/syntheticEOF.test.ts diff --git a/src/lib.rs b/src/lib.rs index 283cd691..b6a18dad 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -39,6 +39,8 @@ pub enum Operation { Delete, } +const SYNTHETIC_EOF: &[u8] = b"\x1B]7878\x1B\\"; + /// Sandboxing rules. Deleting / modifying a path with any of the prefixes is forbidden and will /// cause process termination. #[napi(object)] @@ -292,6 +294,16 @@ impl Pty { thread::spawn(move || { let wait_result = child.wait(); + // by this point, child has closed its copy of the user_fd + // lets inject our synthetic EOF OSC into the user_fd + unsafe { + libc::write( + raw_user_fd, + SYNTHETIC_EOF.as_ptr() as *const libc::c_void, + SYNTHETIC_EOF.len(), + ); + } + match wait_result { Ok(status) => { if status.success() { diff --git a/syntheticEof.ts b/syntheticEof.ts new file mode 100644 index 00000000..8b166f44 --- /dev/null +++ b/syntheticEof.ts @@ -0,0 +1,83 @@ +import { Transform } from 'node:stream'; + +// keep in sync with lib.rs::SYNTHETIC_EOF +export const SYNTHETIC_EOF = Buffer.from('\x1B]7878\x1B\\'); + +function getCommonPrefixLength(buffer: Buffer) { + for (let prefixLen = SYNTHETIC_EOF.length; prefixLen >= 1; prefixLen--) { + const suffix = buffer.subarray(buffer.length - prefixLen); + const prefix = SYNTHETIC_EOF.subarray(0, prefixLen); + + if (suffix.equals(prefix)) { + return prefixLen; + } + } + return 0; +} + +export class SyntheticEOFDetector extends Transform { + buffer: Buffer; + maxBufferSize: number; + + constructor(options = {}) { + super(options); + this.buffer = Buffer.alloc(0); + this.maxBufferSize = SYNTHETIC_EOF.length - 1; + } + + _transform(chunk: Buffer, encoding: string, callback: () => void) { + // combine any leftover buffer with new chunk + // and look for synthetic EOF in the combined data + const searchData = Buffer.concat([this.buffer, chunk]); + const eofIndex = searchData.indexOf(SYNTHETIC_EOF); + + // found EOF - emit data before it + if (eofIndex !== -1) { + const beforeEOF = searchData.subarray(0, eofIndex); + const afterEOF = searchData.subarray(eofIndex + SYNTHETIC_EOF.length); + + if (beforeEOF.length > 0) { + this.push(Buffer.from(beforeEOF)); + } + + this.emit('synthetic-eof'); + + // Continue processing remaining data (might have more EOFs) + if (afterEOF.length > 0) { + this._transform(Buffer.from(afterEOF), encoding, callback); + return; + } + + this.buffer = Buffer.alloc(0); + } else { + // no EOF found - emit all the data except for the enough of a buffer + // to potentially match the start of an EOF sequence next time + const commonPrefixLen = getCommonPrefixLength(searchData); + if (commonPrefixLen > 0) { + // has common prefix - buffer the suffix, emit the rest + const emitSize = searchData.length - commonPrefixLen; + + if (emitSize > 0) { + const toEmit = searchData.subarray(0, emitSize); + this.push(Buffer.from(toEmit)); + } + + this.buffer = Buffer.from(searchData.subarray(emitSize)); + } else { + // no common prefix - emit everything, clear buffer + this.push(Buffer.from(searchData)); + this.buffer = Buffer.alloc(0); + } + } + + callback(); + } + + _flush(callback: () => void) { + if (this.buffer.length > 0) { + this.push(this.buffer); + } + + callback(); + } +} diff --git a/tests/index.test.ts b/tests/index.test.ts index 532f3596..e908bf96 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -56,7 +56,7 @@ function getOpenFds(): FdRecord { return fds; } -describe('PTY', { repeats: 500 }, () => { +describe('PTY', { repeats: 0 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; diff --git a/tests/syntheticEOF.test.ts b/tests/syntheticEOF.test.ts new file mode 100644 index 00000000..a1a85dfc --- /dev/null +++ b/tests/syntheticEOF.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { SyntheticEOFDetector, SYNTHETIC_EOF } from '../syntheticEof'; + +describe('sequence', () => { + it('should have correct EOF sequence', () => { + expect(SYNTHETIC_EOF).toEqual( + Buffer.from([0x1b, 0x5d, 0x37, 0x38, 0x37, 0x38, 0x1b, 0x5c]), + ); + expect(SYNTHETIC_EOF.length).toBe(8); + }); +}); + +describe('SyntheticEOFDetector', () => { + let detector: SyntheticEOFDetector; + let onData: (data: Buffer) => void; + let onEOF: () => void; + let output: Buffer; + + beforeEach(() => { + detector = new SyntheticEOFDetector(); + output = Buffer.alloc(0); + onData = vi.fn((data: Buffer) => (output = Buffer.concat([output, data]))); + onEOF = vi.fn(); + + detector.on('data', onData); + detector.on('synthetic-eof', onEOF); + }); + + it('should handle EOF at the end of stream', async () => { + detector.write('Before EOF'); + detector.write(SYNTHETIC_EOF); + detector.end(); + + expect(output.toString()).toBe('Before EOF'); + expect(onEOF).toHaveBeenCalledTimes(1); + }); + + it('should handle EOF split across chunks', async () => { + detector.write('Data1'); + detector.write('\x1B]78'); // Partial EOF + detector.write('78\x1B\\'); // Complete EOF + detector.write('Data2'); + detector.end(); + + expect(output.toString()).toBe('Data1Data2'); + expect(onEOF).toHaveBeenCalledTimes(1); + }); + + it('should pass through data when no EOF is present', async () => { + detector.write('Just normal data'); + detector.write(' with no EOF'); + detector.end(); + + expect(output.toString()).toBe('Just normal data with no EOF'); + expect(onEOF).not.toHaveBeenCalled(); + }); + + it('should not trigger on partial EOF at end', async () => { + detector.write('Data'); + detector.write('\x1B]78'); // Incomplete EOF + detector.end(); + + expect(output.toString()).toBe('Data\x1B]78'); + expect(onEOF).not.toHaveBeenCalled(); + }); + + it('should handle EOF split in multiple ways', async () => { + // Split after escape + detector.write('\x1B'); + detector.write(']7878\x1B\\'); + detector.write('data1'); + + // Split in middle + detector.write('\x1B]78'); + detector.write('78\x1B\\'); + detector.write('data2'); + detector.end(); + + expect(output.toString()).toBe('data1data2'); + expect(onEOF).toHaveBeenCalledTimes(2); + }); + + it('should not hold up data that isnt a prefix of EOF', async () => { + detector.write('Data that is definitely not an EOF prefix'); + + expect(output.toString()).toBe('Data that is definitely not an EOF prefix'); + expect(onEOF).not.toHaveBeenCalled(); + + detector.end(); + expect(onEOF).not.toHaveBeenCalled(); + }); + + it('should emit events in correct order', async () => { + const detector = new SyntheticEOFDetector(); + const events: Array< + | { + type: 'eof'; + } + | { + type: 'data'; + data: string; + } + > = []; + + detector.on('data', (chunk) => { + events.push({ type: 'data', data: chunk.toString() }); + }); + detector.on('synthetic-eof', () => { + events.push({ type: 'eof' }); + }); + + const finished = new Promise((resolve) => { + detector.on('end', resolve); + }); + + detector.write('before'); + detector.write(SYNTHETIC_EOF); + detector.write('after'); + detector.end(); + + await finished; + + expect(events).toEqual([ + { type: 'data', data: 'before' }, + { type: 'eof' }, + { type: 'data', data: 'after' }, + ]); + }); +}); diff --git a/wrapper.ts b/wrapper.ts index f094fcb7..6f0abd9e 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -1,4 +1,4 @@ -import type { Readable, Writable } from 'node:stream'; +import { Transform, type Readable, type Writable } from 'node:stream'; import { ReadStream } from 'node:tty'; import { Pty as RawPty, @@ -15,6 +15,7 @@ import { type SandboxRule, type SandboxOptions, } from './index.js'; +import { SyntheticEOFDetector } from './syntheticEof.js'; export { Operation, type SandboxRule, type SandboxOptions, type PtyOptions }; @@ -23,10 +24,6 @@ type ExitResult = { code: number; }; -interface FullPtyOptions extends PtyOptions { - exitOutputStabilityPeriod?: number; -} - /** * A very thin wrapper around PTYs and processes. * @@ -60,16 +57,10 @@ export class Pty { #fdClosed: boolean = false; #socket: ReadStream; + read: Readable; + write: Writable; - get read(): Readable { - return this.#socket; - } - - get write(): Writable { - return this.#socket; - } - - constructor(options: FullPtyOptions) { + constructor(options: PtyOptions) { const realExit = options.onExit; let markExited!: (value: ExitResult) => void; @@ -81,22 +72,15 @@ export class Pty { let readFinished = new Promise((resolve) => { markReadFinished = resolve; }); - const mockedExit = async ( - error: NodeJS.ErrnoException | null, - code: number, - ) => { - markExited({ error, code }); - await new Promise((r) => - setTimeout(r, options.exitOutputStabilityPeriod ?? 50), - ); - this.#pty.closeUserFd(); - }; // when pty exits, we should wait until the fd actually ends (end OR error) // before closing the pty // we use a mocked exit function to capture the exit result // and then call the real exit function after the fd is fully read - this.#pty = new RawPty({ ...options, onExit: mockedExit }); + this.#pty = new RawPty({ + ...options, + onExit: (error, code) => markExited({ error, code }), + }); // Transfer ownership of the FD to us. this.#fd = this.#pty.takeFd(); @@ -116,13 +100,11 @@ export class Pty { realExit(result.error, result.code); }; - this.read.once('end', markReadFinished); - this.read.once('close', handleClose); - // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as // cleaning up other spurious errors) so that the user doesn't need to handle them and be in // blissful peace. const handleError = (err: NodeJS.ErrnoException) => { + console.log('handling error', err); if (err.code) { const code = err.code; if (code === 'EINTR' || code === 'EAGAIN') { @@ -134,10 +116,10 @@ export class Pty { // EIO only happens when the child dies. It is therefore our only true signal that there // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. - this.read.off('error', handleError); + this.#socket.off('error', handleError); // emit 'end' to signal no more data // this will trigger our 'end' handler which marks readFinished - this.read.emit('end'); + this.#socket.emit('end'); return; } } @@ -146,7 +128,23 @@ export class Pty { throw err; }; - this.read.on('error', handleError); + this.read = this.#socket.pipe(new SyntheticEOFDetector()); + this.write = this.#socket; + + this.#socket.on('error', handleError); + this.#socket.once('end', () => { + console.log('socket end'); + markReadFinished(); + }); + this.#socket.once('close', () => { + console.log('socket close'); + handleClose(); + }); + this.read.once('synthetic-eof', () => { + console.log('synthetic eof'); + this.#pty.closeUserFd(); + }); + this.read.on('data', () => console.log('data')); } close() { From 0328fce44166c1f124a7bb808364f7a431887199 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 26 Sep 2025 07:09:46 +0000 Subject: [PATCH 15/28] formatting and things --- index.d.ts | 8 ++++++-- src/lib.rs | 6 ++++-- wrapper.ts | 21 +++++---------------- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/index.d.ts b/index.d.ts index 9c663d43..187efd27 100644 --- a/index.d.ts +++ b/index.d.ts @@ -66,6 +66,10 @@ export declare class Pty { * once (it will error the second time). The caller is responsible for closing the file * descriptor. */ - takeFd(): c_int; - closeUserFd(): void; + takeControllerFd(): c_int; + /** + * Closes the owned file descriptor for the PTY controller. The Nodejs side must call this + * when it is done with the file descriptor to avoid leaking FDs. + */ + dropUserFd(): void; } diff --git a/src/lib.rs b/src/lib.rs index b6a18dad..fad40e56 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -342,7 +342,7 @@ impl Pty { /// descriptor. #[napi] #[allow(dead_code)] - pub fn take_fd(&mut self) -> Result { + pub fn take_controller_fd(&mut self) -> Result { if let Some(fd) = self.controller_fd.take() { Ok(fd.into_raw_fd()) } else { @@ -353,9 +353,11 @@ impl Pty { } } + /// Closes the owned file descriptor for the PTY controller. The Nodejs side must call this + /// when it is done with the file descriptor to avoid leaking FDs. #[napi] #[allow(dead_code)] - pub fn close_user_fd(&mut self) -> Result<(), napi::Error> { + pub fn drop_user_fd(&mut self) -> Result<(), napi::Error> { self.user_fd.take(); Ok(()) } diff --git a/wrapper.ts b/wrapper.ts index 6f0abd9e..daeb4e01 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -1,4 +1,4 @@ -import { Transform, type Readable, type Writable } from 'node:stream'; +import { type Readable, type Writable } from 'node:stream'; import { ReadStream } from 'node:tty'; import { Pty as RawPty, @@ -82,7 +82,7 @@ export class Pty { onExit: (error, code) => markExited({ error, code }), }); // Transfer ownership of the FD to us. - this.#fd = this.#pty.takeFd(); + this.#fd = this.#pty.takeControllerFd(); this.#socket = new ReadStream(this.#fd); @@ -104,7 +104,6 @@ export class Pty { // cleaning up other spurious errors) so that the user doesn't need to handle them and be in // blissful peace. const handleError = (err: NodeJS.ErrnoException) => { - console.log('handling error', err); if (err.code) { const code = err.code; if (code === 'EINTR' || code === 'EAGAIN') { @@ -132,19 +131,9 @@ export class Pty { this.write = this.#socket; this.#socket.on('error', handleError); - this.#socket.once('end', () => { - console.log('socket end'); - markReadFinished(); - }); - this.#socket.once('close', () => { - console.log('socket close'); - handleClose(); - }); - this.read.once('synthetic-eof', () => { - console.log('synthetic eof'); - this.#pty.closeUserFd(); - }); - this.read.on('data', () => console.log('data')); + this.#socket.once('end', markReadFinished); + this.#socket.once('close', handleClose); + this.read.once('synthetic-eof', () => this.#pty.dropUserFd()); } close() { From 1990998c9d22774cd7fd7fa9bbf8fa53224e99a7 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 26 Sep 2025 07:12:21 +0000 Subject: [PATCH 16/28] rebump to 500x --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index e908bf96..532f3596 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -56,7 +56,7 @@ function getOpenFds(): FdRecord { return fds; } -describe('PTY', { repeats: 0 }, () => { +describe('PTY', { repeats: 500 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; From 6ff1c28139e4f37398736faae6b92edd44e7dd4f Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 26 Sep 2025 07:32:59 +0000 Subject: [PATCH 17/28] wait for style --- tests/index.test.ts | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 532f3596..7d55bb10 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -77,9 +77,9 @@ describe('PTY', { repeats: 500 }, () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); expect(buffer.trim()).toBe(message); - expect(getOpenFds()).toStrictEqual(oldFds); expect(pty.write.writable).toBe(false); expect(pty.read.readable).toBe(false); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('captures an exit code', async () => { @@ -132,7 +132,7 @@ describe('PTY', { repeats: 500 }, () => { const expectedResult = 'hello cat\r\nhello cat\r\n'; expect(result.trim()).toStrictEqual(expectedResult.trim()); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('can be started in non-interactive fashion', async () => { @@ -157,7 +157,7 @@ describe('PTY', { repeats: 500 }, () => { let result = buffer.toString(); const expectedResult = '\r\n'; expect(result.trim()).toStrictEqual(expectedResult.trim()); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('can be resized', async () => { @@ -211,7 +211,7 @@ describe('PTY', { repeats: 500 }, () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); expect(state).toBe('done'); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('respects working directory', async () => { @@ -234,7 +234,7 @@ describe('PTY', { repeats: 500 }, () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); expect(buffer.trim()).toBe(cwd); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('respects env', async () => { @@ -260,7 +260,7 @@ describe('PTY', { repeats: 500 }, () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); expect(buffer.trim()).toBe(message); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test("resize after exit shouldn't throw", async () => { @@ -330,7 +330,7 @@ describe('PTY', { repeats: 500 }, () => { ).toBe(i); } - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('doesnt miss large output from fast commands', async () => { @@ -417,7 +417,7 @@ describe('PTY', { repeats: 500 }, () => { } await Promise.all(promises); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('can run concurrent shells', async () => { @@ -474,7 +474,7 @@ describe('PTY', { repeats: 500 }, () => { expect(result).toStrictEqual(expectedResult); } - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test("doesn't break when executing non-existing binary", async () => { @@ -487,7 +487,7 @@ describe('PTY', { repeats: 500 }, () => { }); }).rejects.toThrow('No such file or directory'); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('cannot be written to after closing', async () => { @@ -515,7 +515,7 @@ describe('PTY', { repeats: 500 }, () => { } }); await vi.waitFor(() => receivedError); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); test('cannot resize when out of range', async () => { @@ -554,7 +554,7 @@ describe('PTY', { repeats: 500 }, () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, -1); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); }); @@ -745,7 +745,7 @@ describe('cgroup opts', async () => { // Verify that the process was placed in the correct cgroup by // checking its output contains our unique slice name expect(buffer).toContain(cgroupState.sliceName); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); testOnlyOnDarwin('cgroup is not supported on darwin', async () => { @@ -822,7 +822,7 @@ describe('sandbox opts', { repeats: 10 }, async () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); expect(buffer).toContain('hello'); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); testSkipOnDarwin('basic protection against git-yeetage', async () => { @@ -867,7 +867,7 @@ describe('sandbox opts', { repeats: 10 }, async () => { expect(buffer.trimEnd()).toBe( `Tried to delete a forbidden path: ${gitPath}`, ); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); testSkipOnDarwin('can exclude prefixes', async () => { @@ -913,7 +913,7 @@ describe('sandbox opts', { repeats: 10 }, async () => { await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); expect(buffer.trimEnd()).toBe(''); - expect(getOpenFds()).toStrictEqual(oldFds); + await vi.waitFor(() => expect(getOpenFds()).toStrictEqual(oldFds)); }); }); From dbfe16c5b917665057ab05d7953685430a0b34ec Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 26 Sep 2025 16:46:19 +0000 Subject: [PATCH 18/28] comments... --- wrapper.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index daeb4e01..4bf33060 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -81,9 +81,7 @@ export class Pty { ...options, onExit: (error, code) => markExited({ error, code }), }); - // Transfer ownership of the FD to us. this.#fd = this.#pty.takeControllerFd(); - this.#socket = new ReadStream(this.#fd); // catch end events @@ -127,13 +125,24 @@ export class Pty { throw err; }; + // we need this synthetic eof detector as the pty stream has no way + // of distinguishing the program existing vs the data being fully read + // this is injected on the rust side after the .wait on the child process + // returns + // more details: https://github.com/replit/ruspty/pull/93 this.read = this.#socket.pipe(new SyntheticEOFDetector()); this.write = this.#socket; this.#socket.on('error', handleError); this.#socket.once('end', markReadFinished); this.#socket.once('close', handleClose); - this.read.once('synthetic-eof', () => this.#pty.dropUserFd()); + this.read.once('synthetic-eof', async () => { + // even if the program accidentally emits our synthetic eof + // we dont yank the user fd away from them until the program actually exits + // (and drops its copy of the user fd) + await exitResult; + this.#pty.dropUserFd(); + }); } close() { From 8aca9693fb0b6956c242ee22384674a8f8edfc4c Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Sun, 28 Sep 2025 11:20:18 -0700 Subject: [PATCH 19/28] review comments --- index.d.ts | 1 + index.js | 2 ++ src/lib.rs | 23 +++++++------ syntheticEof.ts | 70 ++++++++++++++++++++------------------ tests/syntheticEOF.test.ts | 24 ++++++++----- wrapper.ts | 25 ++++++++++---- 6 files changed, 85 insertions(+), 60 deletions(-) diff --git a/index.d.ts b/index.d.ts index 187efd27..0142bcd6 100644 --- a/index.d.ts +++ b/index.d.ts @@ -45,6 +45,7 @@ export interface Size { } export const MAX_U16_VALUE: number; export const MIN_U16_VALUE: number; +export declare function getSyntheticEofSequence(): Buffer; /** Resize the terminal. */ export declare function ptyResize(fd: number, size: Size): void; /** diff --git a/index.js b/index.js index 5b9f1d8b..64824e59 100644 --- a/index.js +++ b/index.js @@ -326,6 +326,7 @@ const { Operation, MAX_U16_VALUE, MIN_U16_VALUE, + getSyntheticEofSequence, ptyResize, setCloseOnExec, getCloseOnExec, @@ -335,6 +336,7 @@ module.exports.Pty = Pty; module.exports.Operation = Operation; module.exports.MAX_U16_VALUE = MAX_U16_VALUE; module.exports.MIN_U16_VALUE = MIN_U16_VALUE; +module.exports.getSyntheticEofSequence = getSyntheticEofSequence; module.exports.ptyResize = ptyResize; module.exports.setCloseOnExec = setCloseOnExec; module.exports.getCloseOnExec = getCloseOnExec; diff --git a/src/lib.rs b/src/lib.rs index fad40e56..014bb3b1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,14 +1,14 @@ use std::collections::HashMap; -use std::fs::{write, File}; use std::io::ErrorKind; use std::io::{Error, Write}; +use std::mem; use std::os::fd::{AsRawFd, OwnedFd}; use std::os::fd::{FromRawFd, IntoRawFd, RawFd}; use std::os::unix::process::CommandExt; use std::process::{Command, Stdio}; use std::thread; -use napi::bindgen_prelude::JsFunction; +use napi::bindgen_prelude::{Buffer, JsFunction}; use napi::threadsafe_function::{ErrorStrategy, ThreadsafeFunction, ThreadsafeFunctionCallMode}; use napi::Status::GenericFailure; use napi::{self, Env}; @@ -39,8 +39,6 @@ pub enum Operation { Delete, } -const SYNTHETIC_EOF: &[u8] = b"\x1B]7878\x1B\\"; - /// Sandboxing rules. Deleting / modifying a path with any of the prefixes is forbidden and will /// cause process termination. #[napi(object)] @@ -89,6 +87,13 @@ pub const MAX_U16_VALUE: u16 = u16::MAX; #[napi] pub const MIN_U16_VALUE: u16 = u16::MIN; +const SYNTHETIC_EOF: &[u8] = b"\x1B]7878\x1B\\"; + +#[napi] +pub fn get_synthetic_eof_sequence() -> Buffer { + SYNTHETIC_EOF.into() +} + fn cast_to_napi_error(err: Errno) -> napi::Error { napi::Error::new(GenericFailure, err) } @@ -296,13 +301,9 @@ impl Pty { // by this point, child has closed its copy of the user_fd // lets inject our synthetic EOF OSC into the user_fd - unsafe { - libc::write( - raw_user_fd, - SYNTHETIC_EOF.as_ptr() as *const libc::c_void, - SYNTHETIC_EOF.len(), - ); - } + let mut file = unsafe { std::fs::File::from_raw_fd(raw_user_fd) }; + let _ = file.write_all(&SYNTHETIC_EOF); // ignore, we have a timeout on the nodejs side to handle if this write fails + mem::forget(file); // forget the file to avoid dropping it match wait_result { Ok(status) => { diff --git a/syntheticEof.ts b/syntheticEof.ts index 8b166f44..581b4127 100644 --- a/syntheticEof.ts +++ b/syntheticEof.ts @@ -1,71 +1,73 @@ import { Transform } from 'node:stream'; +import { getSyntheticEofSequence } from './index.js'; // keep in sync with lib.rs::SYNTHETIC_EOF -export const SYNTHETIC_EOF = Buffer.from('\x1B]7878\x1B\\'); - -function getCommonPrefixLength(buffer: Buffer) { - for (let prefixLen = SYNTHETIC_EOF.length; prefixLen >= 1; prefixLen--) { - const suffix = buffer.subarray(buffer.length - prefixLen); - const prefix = SYNTHETIC_EOF.subarray(0, prefixLen); +export const SYNTHETIC_EOF = getSyntheticEofSequence(); +export const EOF_EVENT = 'synthetic-eof'; + +// get the longest suffix of buffer that is a prefix of SYNTHETIC_EOF +function getBufferEndPrefixLength(buffer: Buffer) { + const maxLen = Math.min(buffer.length, SYNTHETIC_EOF.length); + for (let len = maxLen; len > 0; len--) { + let match = true; + for (let i = 0; i < len; i++) { + if (buffer[buffer.length - len + i] !== SYNTHETIC_EOF[i]) { + match = false; + break; + } + } - if (suffix.equals(prefix)) { - return prefixLen; + if (match) { + return len; } } + return 0; } export class SyntheticEOFDetector extends Transform { buffer: Buffer; - maxBufferSize: number; constructor(options = {}) { super(options); this.buffer = Buffer.alloc(0); - this.maxBufferSize = SYNTHETIC_EOF.length - 1; } - _transform(chunk: Buffer, encoding: string, callback: () => void) { - // combine any leftover buffer with new chunk - // and look for synthetic EOF in the combined data + _transform(chunk: Buffer, _encoding: string, callback: () => void) { const searchData = Buffer.concat([this.buffer, chunk]); const eofIndex = searchData.indexOf(SYNTHETIC_EOF); - // found EOF - emit data before it if (eofIndex !== -1) { - const beforeEOF = searchData.subarray(0, eofIndex); - const afterEOF = searchData.subarray(eofIndex + SYNTHETIC_EOF.length); - - if (beforeEOF.length > 0) { - this.push(Buffer.from(beforeEOF)); + // found EOF - emit everything before it + if (eofIndex > 0) { + this.push(searchData.subarray(0, eofIndex)); } - this.emit('synthetic-eof'); + this.emit(EOF_EVENT); - // Continue processing remaining data (might have more EOFs) + // emit everything after EOF (if any) and clear buffer + const afterEOF = searchData.subarray(eofIndex + SYNTHETIC_EOF.length); if (afterEOF.length > 0) { - this._transform(Buffer.from(afterEOF), encoding, callback); - return; + this.push(afterEOF); } this.buffer = Buffer.alloc(0); } else { - // no EOF found - emit all the data except for the enough of a buffer - // to potentially match the start of an EOF sequence next time - const commonPrefixLen = getCommonPrefixLength(searchData); + // no EOF - buffer potential partial match at end + + // get the longest suffix of buffer that is a prefix of SYNTHETIC_EOF + // and emit everything before it + // this is done for the case which the eof happened to be split across multiple chunks + const commonPrefixLen = getBufferEndPrefixLength(searchData); + if (commonPrefixLen > 0) { - // has common prefix - buffer the suffix, emit the rest const emitSize = searchData.length - commonPrefixLen; - if (emitSize > 0) { - const toEmit = searchData.subarray(0, emitSize); - this.push(Buffer.from(toEmit)); + this.push(searchData.subarray(0, emitSize)); } - - this.buffer = Buffer.from(searchData.subarray(emitSize)); + this.buffer = searchData.subarray(emitSize); } else { - // no common prefix - emit everything, clear buffer - this.push(Buffer.from(searchData)); + this.push(searchData); this.buffer = Buffer.alloc(0); } } diff --git a/tests/syntheticEOF.test.ts b/tests/syntheticEOF.test.ts index a1a85dfc..df5837c1 100644 --- a/tests/syntheticEOF.test.ts +++ b/tests/syntheticEOF.test.ts @@ -1,5 +1,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { SyntheticEOFDetector, SYNTHETIC_EOF } from '../syntheticEof'; +import { + SyntheticEOFDetector, + SYNTHETIC_EOF, + EOF_EVENT, +} from '../syntheticEof'; describe('sequence', () => { it('should have correct EOF sequence', () => { @@ -23,7 +27,7 @@ describe('SyntheticEOFDetector', () => { onEOF = vi.fn(); detector.on('data', onData); - detector.on('synthetic-eof', onEOF); + detector.on(EOF_EVENT, onEOF); }); it('should handle EOF at the end of stream', async () => { @@ -64,20 +68,24 @@ describe('SyntheticEOFDetector', () => { expect(onEOF).not.toHaveBeenCalled(); }); - it('should handle EOF split in multiple ways', async () => { - // Split after escape + it('should handle EOF split after escape', async () => { detector.write('\x1B'); detector.write(']7878\x1B\\'); detector.write('data1'); + detector.end(); - // Split in middle + expect(output.toString()).toBe('data1'); + expect(onEOF).toHaveBeenCalledTimes(1); + }); + + it('should handle EOF split in the middle', async () => { detector.write('\x1B]78'); detector.write('78\x1B\\'); detector.write('data2'); detector.end(); - expect(output.toString()).toBe('data1data2'); - expect(onEOF).toHaveBeenCalledTimes(2); + expect(output.toString()).toBe('data2'); + expect(onEOF).toHaveBeenCalledTimes(1); }); it('should not hold up data that isnt a prefix of EOF', async () => { @@ -105,7 +113,7 @@ describe('SyntheticEOFDetector', () => { detector.on('data', (chunk) => { events.push({ type: 'data', data: chunk.toString() }); }); - detector.on('synthetic-eof', () => { + detector.on(EOF_EVENT, () => { events.push({ type: 'eof' }); }); diff --git a/wrapper.ts b/wrapper.ts index 4bf33060..59a4656f 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -15,7 +15,7 @@ import { type SandboxRule, type SandboxOptions, } from './index.js'; -import { SyntheticEOFDetector } from './syntheticEof.js'; +import { EOF_EVENT, SyntheticEOFDetector } from './syntheticEof.js'; export { Operation, type SandboxRule, type SandboxOptions, type PtyOptions }; @@ -54,7 +54,8 @@ export class Pty { #fd: number; #handledClose: boolean = false; - #fdClosed: boolean = false; + #socketClosed: boolean = false; + #userFdDropped: boolean = false; #socket: ReadStream; read: Readable; @@ -86,11 +87,11 @@ export class Pty { // catch end events const handleClose = async () => { - if (this.#fdClosed) { + if (this.#socketClosed) { return; } - this.#fdClosed = true; + this.#socketClosed = true; // must wait for fd close and exit result before calling real exit await readFinished; @@ -126,7 +127,7 @@ export class Pty { }; // we need this synthetic eof detector as the pty stream has no way - // of distinguishing the program existing vs the data being fully read + // of distinguishing the program exiting vs the data being fully read // this is injected on the rust side after the .wait on the child process // returns // more details: https://github.com/replit/ruspty/pull/93 @@ -136,11 +137,17 @@ export class Pty { this.#socket.on('error', handleError); this.#socket.once('end', markReadFinished); this.#socket.once('close', handleClose); - this.read.once('synthetic-eof', async () => { + this.read.once(EOF_EVENT, async () => { // even if the program accidentally emits our synthetic eof // we dont yank the user fd away from them until the program actually exits // (and drops its copy of the user fd) await exitResult; + + if (this.#userFdDropped) { + return; + } + + this.#userFdDropped = true; this.#pty.dropUserFd(); }); } @@ -151,10 +158,14 @@ export class Pty { // end instead of destroy so that the user can read the last bits of data // and allow graceful close event to mark the fd as ended this.#socket.end(); + + if (!this.#userFdDropped) { + this.#pty.dropUserFd(); + } } resize(size: Size) { - if (this.#handledClose || this.#fdClosed) { + if (this.#handledClose || this.#socketClosed) { return; } From 1adb8bd2eb273a1dc8cdc6f2e2d65d6daa334a9a Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Sun, 28 Sep 2025 11:26:22 -0700 Subject: [PATCH 20/28] readd rust import, fix drop on wrapper side --- src/lib.rs | 1 + wrapper.ts | 31 +++++++++++++++++++++++++------ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 014bb3b1..09367e5f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::fs::{write, File}; use std::io::ErrorKind; use std::io::{Error, Write}; use std::mem; diff --git a/wrapper.ts b/wrapper.ts index 59a4656f..fb1d0e15 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -56,6 +56,7 @@ export class Pty { #handledClose: boolean = false; #socketClosed: boolean = false; #userFdDropped: boolean = false; + #fdDropTimeout: ReturnType | null = null; #socket: ReadStream; read: Readable; @@ -80,7 +81,15 @@ export class Pty { // and then call the real exit function after the fd is fully read this.#pty = new RawPty({ ...options, - onExit: (error, code) => markExited({ error, code }), + onExit: (error, code) => { + // give nodejs a max of 1s to read the fd before + // dropping the fd to avoid leaking it + this.#fdDropTimeout = setTimeout(() => { + this.dropUserFd(); + }, 1000); + + markExited({ error, code }); + }, }); this.#fd = this.#pty.takeControllerFd(); this.#socket = new ReadStream(this.#fd); @@ -148,20 +157,30 @@ export class Pty { } this.#userFdDropped = true; - this.#pty.dropUserFd(); + this.dropUserFd(); }); } + private dropUserFd() { + if (this.#userFdDropped) { + return; + } + + if (this.#fdDropTimeout) { + clearTimeout(this.#fdDropTimeout); + } + + this.#userFdDropped = true; + this.#pty.dropUserFd(); + } + close() { this.#handledClose = true; // end instead of destroy so that the user can read the last bits of data // and allow graceful close event to mark the fd as ended this.#socket.end(); - - if (!this.#userFdDropped) { - this.#pty.dropUserFd(); - } + this.dropUserFd(); } resize(size: Size) { From d3476d564877cf69467a29dd7e13c53cbb0a16af Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 17:44:07 +0000 Subject: [PATCH 21/28] fix drop oops --- src/lib.rs | 3 ++- tests/index.test.ts | 4 ++-- wrapper.ts | 6 ------ 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 09367e5f..9b986e0c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -302,8 +302,9 @@ impl Pty { // by this point, child has closed its copy of the user_fd // lets inject our synthetic EOF OSC into the user_fd + // its ok to ignore the result here as we have a timeout on the nodejs side to handle if this write fails let mut file = unsafe { std::fs::File::from_raw_fd(raw_user_fd) }; - let _ = file.write_all(&SYNTHETIC_EOF); // ignore, we have a timeout on the nodejs side to handle if this write fails + let _ = file.write_all(SYNTHETIC_EOF); // ignore, we have a timeout on the nodejs side to handle if this write fails mem::forget(file); // forget the file to avoid dropping it match wait_result { diff --git a/tests/index.test.ts b/tests/index.test.ts index 7d55bb10..185a8b0a 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -56,8 +56,8 @@ function getOpenFds(): FdRecord { return fds; } -describe('PTY', { repeats: 500 }, () => { - test('spawns and exits', async () => { +describe('PTY', { repeats: 0 }, () => { + test.only('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; let buffer = ''; diff --git a/wrapper.ts b/wrapper.ts index fb1d0e15..8598e125 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -151,12 +151,6 @@ export class Pty { // we dont yank the user fd away from them until the program actually exits // (and drops its copy of the user fd) await exitResult; - - if (this.#userFdDropped) { - return; - } - - this.#userFdDropped = true; this.dropUserFd(); }); } From 4427693388eb51fd114a8bcfce18b986a031d140 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 17:47:25 +0000 Subject: [PATCH 22/28] fix file parallelism --- vitest.config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/vitest.config.ts b/vitest.config.ts index 9a4c46f9..8aaae5d0 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,5 +1,6 @@ export default { test: { exclude: ['node_modules', 'dist', '.direnv'], + fileParallelism: false }, }; From c07e3478cfebeff6091e66f6c1ebfdca51da04e9 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 17:47:40 +0000 Subject: [PATCH 23/28] fix vitest --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 185a8b0a..0ce53a06 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -57,7 +57,7 @@ function getOpenFds(): FdRecord { } describe('PTY', { repeats: 0 }, () => { - test.only('spawns and exits', async () => { + test('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; let buffer = ''; From 92ca323346315bb3bada640baed1a8d377f1d82d Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 18:00:01 +0000 Subject: [PATCH 24/28] test with threads --- tests/index.test.ts | 2 +- vitest.config.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 0ce53a06..185a8b0a 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -57,7 +57,7 @@ function getOpenFds(): FdRecord { } describe('PTY', { repeats: 0 }, () => { - test('spawns and exits', async () => { + test.only('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; let buffer = ''; diff --git a/vitest.config.ts b/vitest.config.ts index 8aaae5d0..3d192a61 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,6 +1,7 @@ export default { test: { exclude: ['node_modules', 'dist', '.direnv'], - fileParallelism: false + fileParallelism: false, + pool: 'threads', }, }; From 1fb83fb0f58145d2d66d493454c486e0ad274624 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 18:03:02 +0000 Subject: [PATCH 25/28] forks instead maybe?? --- vitest.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vitest.config.ts b/vitest.config.ts index 3d192a61..d0ba3102 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -2,6 +2,6 @@ export default { test: { exclude: ['node_modules', 'dist', '.direnv'], fileParallelism: false, - pool: 'threads', + pool: 'forks', }, }; From 2a34263815b3e009a82aa122be8cd58f50123ce2 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 18:05:41 +0000 Subject: [PATCH 26/28] rebump repeads --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 185a8b0a..5b91826e 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -56,7 +56,7 @@ function getOpenFds(): FdRecord { return fds; } -describe('PTY', { repeats: 0 }, () => { +describe('PTY', { repeats: 500 }, () => { test.only('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; From 776afc9613740a073049e1477edf0d97b0cf782e Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 18:59:35 +0000 Subject: [PATCH 27/28] oops remove only --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 5b91826e..7d55bb10 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -57,7 +57,7 @@ function getOpenFds(): FdRecord { } describe('PTY', { repeats: 500 }, () => { - test.only('spawns and exits', async () => { + test('spawns and exits', async () => { const oldFds = getOpenFds(); const message = 'hello from a pty'; let buffer = ''; From c65af58feaec85a10f1f7fb1c3635d42a17565ad Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 29 Sep 2025 19:12:59 +0000 Subject: [PATCH 28/28] write_syn_eof_to_fd helper instead of unsafe + forget --- src/lib.rs | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9b986e0c..8397c505 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; use std::fs::{write, File}; use std::io::ErrorKind; use std::io::{Error, Write}; -use std::mem; use std::os::fd::{AsRawFd, OwnedFd}; use std::os::fd::{FromRawFd, IntoRawFd, RawFd}; use std::os::unix::process::CommandExt; @@ -303,9 +302,7 @@ impl Pty { // by this point, child has closed its copy of the user_fd // lets inject our synthetic EOF OSC into the user_fd // its ok to ignore the result here as we have a timeout on the nodejs side to handle if this write fails - let mut file = unsafe { std::fs::File::from_raw_fd(raw_user_fd) }; - let _ = file.write_all(SYNTHETIC_EOF); // ignore, we have a timeout on the nodejs side to handle if this write fails - mem::forget(file); // forget the file to avoid dropping it + let _ = write_syn_eof_to_fd(raw_user_fd); match wait_result { Ok(status) => { @@ -458,3 +455,35 @@ fn set_nonblocking(fd: i32) -> Result<(), napi::Error> { } Ok(()) } + +fn write_syn_eof_to_fd(fd: libc::c_int) -> std::io::Result<()> { + let mut remaining = SYNTHETIC_EOF; + while !remaining.is_empty() { + match unsafe { + libc::write( + fd, + remaining.as_ptr() as *const libc::c_void, + remaining.len(), + ) + } { + -1 => { + let err = std::io::Error::last_os_error(); + if err.kind() == std::io::ErrorKind::Interrupted { + continue; + } + + return Err(err); + } + 0 => { + return Err(std::io::Error::new( + std::io::ErrorKind::WriteZero, + "write returned 0", + )); + } + n => { + remaining = &remaining[n as usize..]; + } + } + } + Ok(()) +}