Skip to content

Commit e085620

Browse files
authored
fix(core): resume() drops keystrokes due to deferred listener registration (#858)
## Problem `resume()` calls `stdin.resume()` then defers listener registration into a `setImmediate`: ```ts this.stdin.resume() this.addExitListeners() setImmediate(() => { while (this.stdin.read() !== null) {} this.stdin.on("data", this.stdinListener) }) ``` The stream starts flowing immediately, but `stdinListener` isn't attached until the next macrotask. Any keystrokes arriving in that gap are silently lost — which is exactly the window where a user types after `fg`. `setupInput()` has a milder variant of the same bug: `resume()` before `on("data")`. The drain loop was introduced in #331 to discard stale input from child process suspension. The placement made it ineffective: `read()` returns null in flowing mode because data goes through events, not pull. Stale bytes were only "discarded" by accident — emitted as `data` events with no listener present. **Repro:** in the test suite, `mockInput.pressKey()` right after `renderer.resume()` is silently dropped. The existing tests masked this by yielding a macrotask (`await new Promise(r => setImmediate(r))`) before asserting. ## Fix Reorder `resume()` to: drain → register listener → resume stream: ```ts while (this.stdin.read() !== null) {} this.stdin.on("data", this.stdinListener) this.stdin.resume() ``` The ordering is load-bearing: 1. **Drain first** — `read()` pulls from the internal buffer in paused mode, discarding stale input (#331's scenario). 2. **Register listener second** — must come after the drain. Adding a `data` listener can auto-resume a paused Readable, flushing stale bytes through the listener before the drain runs. 3. **Resume last** — starts flowing mode. New data hits the registered listener with zero gap. `setupInput()` just swaps the two lines (no drain needed at init). ## Tests **Modified:** removed three `setImmediate` yield workarounds (2 in `renderer.control`, 1 in `renderer.input`). The tests now assert synchronous delivery — they'd fail if the fix were wrong. **New (4):** - "keystrokes received immediately after resume() without yielding" — core invariant. - "keystrokes survive multiple rapid suspend/resume cycles" — 5 cycles, no accumulation. - "input buffered during suspension is drained on resume" — `push()` bytes into stdin during suspension, verifies they're discarded. Directly tests the #331 scenario. - "suspend/resume does not leak stdin listeners" — 10 cycles, `listenerCount("data")` stays constant. ## Background I hit this in 0.1.88 while building a TUI on OpenTUI and patched the bundled JS. When 0.1.90 shipped, `setupInput()` had its `setImmediate` removed but the ordering was still wrong, and `resume()` still defers via `setImmediate`. I've been running a patched build since. The `setImmediate` + drain pattern was introduced in #770 (building on #331). The intent was sound — this fix keeps the drain but moves it to where `read()` actually works.
1 parent d7ce851 commit e085620

File tree

3 files changed

+74
-14
lines changed

3 files changed

+74
-14
lines changed

packages/core/src/renderer.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1311,8 +1311,8 @@ export class CliRenderer extends EventEmitter implements RenderContext {
13111311
this.stdin.setRawMode(true)
13121312
}
13131313

1314-
this.stdin.resume()
13151314
this.stdin.on("data", this.stdinListener)
1315+
this.stdin.resume()
13161316
}
13171317

13181318
private dispatchMouseEvent(
@@ -1862,15 +1862,15 @@ export class CliRenderer extends EventEmitter implements RenderContext {
18621862
this.stdin.setRawMode(true)
18631863
}
18641864

1865+
// Drain any input buffered during suspension before registering the
1866+
// listener. Adding a "data" listener can auto-resume a Readable, so the
1867+
// drain must come first while the stream is still paused and read()
1868+
// pulls from the internal buffer rather than being a flowing-mode no-op.
1869+
while (this.stdin.read() !== null) {}
1870+
this.stdin.on("data", this.stdinListener)
18651871
this.stdin.resume()
18661872
this.addExitListeners()
18671873

1868-
setImmediate(() => {
1869-
// Consume any existing stdin data to avoid processing stale input
1870-
while (this.stdin.read() !== null) {}
1871-
this.stdin.on("data", this.stdinListener)
1872-
})
1873-
18741874
this.lib.resumeRenderer(this.rendererPtr)
18751875

18761876
if (this._suspendedMouseEnabled) {

packages/core/src/tests/renderer.control.test.ts

Lines changed: 67 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ test("multiple suspend/resume cycles work correctly", () => {
277277
expect(renderer.controlState).toBe(RendererControlState.EXPLICIT_PAUSED)
278278
})
279279

280-
test("keyboard input is suspended when renderer is suspended", async () => {
280+
test("keyboard input is suspended when renderer is suspended", () => {
281281
renderer.start()
282282

283283
let keyEventReceived = false
@@ -295,8 +295,6 @@ test("keyboard input is suspended when renderer is suspended", async () => {
295295
mockInput.pressKey("b")
296296
expect(keyEventReceived).toBe(false)
297297
renderer.resume()
298-
// Wait for renderer to consume stale input and re-register listeners
299-
await new Promise((r) => setImmediate(r))
300298
mockInput.pressKey("c")
301299
expect(keyEventReceived).toBe(true)
302300
renderer.keyInput.off("keypress", onKeypress)
@@ -335,7 +333,7 @@ test("mouse input is suspended when renderer is suspended", async () => {
335333
renderer.root.remove(testRenderable.id)
336334
})
337335

338-
test("paste input is suspended when renderer is suspended", async () => {
336+
test("paste input is suspended when renderer is suspended", () => {
339337
renderer.start()
340338

341339
let pasteEventReceived = false
@@ -354,11 +352,74 @@ test("paste input is suspended when renderer is suspended", async () => {
354352
expect(pasteEventReceived).toBe(false)
355353

356354
renderer.resume()
357-
// Wait for renderer to consume stale input and re-register listeners
358-
await new Promise((r) => setImmediate(r))
359355

360356
mockInput.pasteBracketedText("pasted text 3")
361357
expect(pasteEventReceived).toBe(true)
362358

363359
renderer.keyInput.off("paste", onPaste)
364360
})
361+
362+
test("keystrokes received immediately after resume() without yielding", () => {
363+
renderer.start()
364+
365+
const received: string[] = []
366+
const onKeypress = (e: { name: string }) => received.push(e.name)
367+
renderer.keyInput.on("keypress", onKeypress)
368+
369+
renderer.suspend()
370+
renderer.resume()
371+
mockInput.pressKey("a")
372+
mockInput.pressKey("b")
373+
374+
expect(received).toEqual(["a", "b"])
375+
renderer.keyInput.off("keypress", onKeypress)
376+
})
377+
378+
test("keystrokes survive multiple rapid suspend/resume cycles", () => {
379+
renderer.start()
380+
381+
const received: string[] = []
382+
const onKeypress = (e: { name: string }) => received.push(e.name)
383+
renderer.keyInput.on("keypress", onKeypress)
384+
385+
for (let i = 0; i < 5; i++) {
386+
renderer.suspend()
387+
renderer.resume()
388+
}
389+
mockInput.pressKey("a")
390+
391+
expect(received).toEqual(["a"])
392+
renderer.keyInput.off("keypress", onKeypress)
393+
})
394+
395+
test("input buffered during suspension is drained on resume", () => {
396+
renderer.start()
397+
398+
const received: string[] = []
399+
const onKeypress = (e: { name: string }) => received.push(e.name)
400+
renderer.keyInput.on("keypress", onKeypress)
401+
402+
renderer.suspend()
403+
// Simulate stale input accumulating in stdin's internal buffer during
404+
// suspension (e.g. from a child process or kernel line buffer).
405+
// push() writes to the Readable's internal buffer without emitting.
406+
renderer.stdin.push(Buffer.from("x"))
407+
renderer.resume()
408+
mockInput.pressKey("a")
409+
410+
// "x" should have been drained — only "a" received
411+
expect(received).toEqual(["a"])
412+
renderer.keyInput.off("keypress", onKeypress)
413+
})
414+
415+
test("suspend/resume does not leak stdin listeners", () => {
416+
renderer.start()
417+
const baseline = renderer.stdin.listenerCount("data")
418+
419+
for (let i = 0; i < 10; i++) {
420+
renderer.suspend()
421+
renderer.resume()
422+
}
423+
424+
expect(renderer.stdin.listenerCount("data")).toBe(baseline)
425+
})

packages/core/src/tests/renderer.input.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2104,7 +2104,6 @@ describe("stdin routing", () => {
21042104

21052105
renderer.suspend()
21062106
renderer.resume()
2107-
await new Promise((resolve) => setImmediate(resolve))
21082107

21092108
renderer.stdin.emit("data", Buffer.from("x"))
21102109
advanceClock(clock)

0 commit comments

Comments
 (0)