Repository navigation
Conversation
meta-cla
Bot
added
the
CLA Signed
Metro review: Changes suggestedRisk: low · Commit: The PR runs the status update loop inline with synchronous cleanup, marks fire-and-forget rejections as handled, and resets state after a failed status write. All four findings from the previous review are fixed, but the failure handling still has two gaps that contradict the description and changelog: a failed write during the Findings
Verified
Not run: Automated review by Claude. Advisory only: it never approves or blocks, and a maintainer makes the call. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
87fcdbe to
77e8372
Compare
|
Addressed the review findings:
Verified: |
deecf49 to
505c4bb
Compare
vzaidman
changed the title
803a5f3 to
3a037ba
Compare
vzaidman
changed the title
3a037ba to
9ecabbc
Compare
| // An update is fire-and-forget: mark its rejection as handled, so | ||
| // a failed update doesn't crash the process when no one awaits it. | ||
| // Waiters still see the rejection through #updatePromise. | ||
| void this.#updatePromise.catch(() => {}); |
There was a problem hiding this comment.
should fix · A failed write during the beforeExit flush still crashes the process
The PR marks #updatePromise and #lastWritePromise as handled, but the beforeExit listener in the constructor (Terminal.js:108) still calls void this.flush(). flush() returns a new promise that rejects when the write fails, and nothing handles that one.
This contradicts the description's claim that a failed update no longer crashes the process when no one awaits it. The exit flush is the one internal caller of flush() and has no waiter of its own.
Failure case: a plain new Terminal(stream), with stream.write throwing on the pending write:
t.status('a'); // written
t.status('b'); // pending behind the unref'd status delay
s.failNext = 1; // the next write throws
// the event loop drains, beforeExit flushes, the write throwsThe process exits with code 1 on an unhandled rejection, even though no code ever awaited the update.
Fix: handle the rejection in the listener: this.flush().catch(() => {});. Add a test that runs this in a child process or invokes the listener directly.
Reproduced: node __review__/terminal-repro.js, scenario beforeExitFlush, on PR head: exit=1, exit handler code=1 writes=["a\n"], with the stderr showing the thrown write failed. Scenario noWaiter (failure with the update's own promise only) exits 0, so the gap is specific to the flush path. · id b39da856
| // A write is fire-and-forget: mark its rejection as handled, so a | ||
| // failed write doesn't crash the process when no one awaits it. | ||
| // Waiters still see the rejection through #lastWritePromise. | ||
| void this.#lastWritePromise.catch(() => {}); |
There was a problem hiding this comment.
should fix · A failed log() write keeps the stale state this PR fixes for status writes
The reset added to #writeStatus() only covers writes that function issues itself. log() calls #write() directly, then starts an update that returns early when the status looks already written, so the new catch never runs. The changelog says recovery happens without erasing earlier output or rejecting with a stale error, but both still happen on this path:
- Stale rejection: with
stream.writethrowing once,t.log('x')leaves#lastWritePromiserejected. Every laterflush()rejects with the oldwrite failed, even though the stream has recovered, until some other write replaces the promise. - Erases earlier output (TTY): with
foologged andstatusdrawn,log('bar')erases the status successfully, then itsline + statusredraw throws.log()still sets#writtenStatus = writtenStatus, so the status is recorded as drawn although it was just erased. The nextstatus('status2')emits cursor-up and clear again, which moves over the line above and erasesfoo.
Fix: settle failed writes in one place. For example, have log() track its write promise and, if it rejects and is still the latest write, reset #writtenStatus and clear #lastWritePromise, as #writeStatus() does. Add tests for a failed log() next to the new status tests.
Reproduced: node __review__/terminal-repro.js, scenario logStale: flush 1, 2 and 3 all report rejected: write failed. node __review__/terminal-log-repro.js: the failed log's only successful write is \r\x1b[1A\x1b[J, then status2 writes ["\r\x1b[1A\x1b[J","status2\n"], a second cursor-up with no status on screen. · id 4523774c
Run each status update in #scheduleStatusUpdate() itself, with no helper: each iteration assigns #updatePromise inside the try block to the current #writeStatus() promise and awaits it, looping until the written status is the latest one. The loop exit check and the cleanup (beforeExit listener removal, #updatePromise reset, flush-request reset) run in one synchronous step, with no microtask gap between them, so a status() call just after an update finishes can't slip between them and be lost. A rejected #writeStatus() no longer leaves the listener attached or a rejected promise in #updatePromise, which made every later #scheduleStatusUpdate() early-return and permanently stopped status updates. waitForUpdates() loops on #updatePromise until it clears, so it waits for the whole update, not just the current write. Status updates, writes, and the beforeExit flush are fire-and-forget, so mark their rejections as handled: a failed update no longer crashes the process when no one awaits it, while waitForUpdates() and flush() still see the rejection. #writeStatus() marks the status as written before awaiting the write's completion. If that write fails, the status was never drawn, but #writtenStatus still claims it was. The next update then erases a status that isn't on the screen, which in a TTY moves the cursor above the drawn output and erases earlier logs, and a retry of the same status writes nothing because it looks already written. Reset #writtenStatus and clear #lastWritePromise on a failed write, unless another write has replaced the awaited promise or #writtenStatus has changed since, as a log() in between redraws the status itself. Without the clear, flush() and waitForUpdates() keep rejecting with the stale error. A failed log() write leaves the same stale state: log() now applies the same reset to a redrawn status and clears the stale write promise. Test plan: New tests 'does not lose a status set just after an update finishes' (fails on the previous .finally() version, passes on base and with this change), 'does not crash on a failed status write with no waiter', 'does not crash on a failed write during the beforeExit flush', 'recovers after a failed status write', 'recovery after a failed status write does not erase earlier logs', 'retries the same status after a failed status write', 'flush does not reject with a stale error after a failed status write', 'flush does not reject with a stale error after a failed log write', and 'a failed log redraw does not erase earlier output on recovery' fail without the fix and pass with it. yarn jest packages/metro-core/src/__tests__/Terminal-test.js, yarn eslint, yarn prettier, yarn flow check.
9ecabbc to
31e08d5
Compare
|
Addressed the latest review findings:
Verified: |
Run each status update in
#scheduleStatusUpdate()itself, with no helper: each iteration assigns#updatePromiseinside thetryblock to the current#writeStatus()promise and awaits it, looping until the written status is the latest one. The loop exit check and the cleanup (thebeforeExitlistener removal, the#updatePromisereset, the flush-request reset) run in one synchronous step, with no microtask gap between them, so astatus()call just after an update finishes can't slip between them and be lost. A rejected#writeStatus()no longer leaves the listener attached or a rejected promise in#updatePromise. Every later#scheduleStatusUpdate()then early-returned at the#updatePromise != nullguard, permanently stopping status updates and leavingbeforeExitwired to flush a broken terminal.waitForUpdates()loops on#updatePromiseuntil it clears, so it waits for the whole update, not just the current write.Status updates, writes, and the
beforeExitflush are fire-and-forget, so their rejections are marked as handled: a failed update no longer crashes the process when no one awaits it, whilewaitForUpdates()andflush()still see the rejection.#writeStatus()marks the status as written before awaiting the write's completion. If that write fails, the status was never drawn, but#writtenStatusstill claims it was. The next update then erases a status that isn't on the screen, which in a TTY moves the cursor above the drawn output and erases earlier logs, and a retry of the same status writes nothing because it looks already written. Reset#writtenStatusand clear#lastWritePromiseon a failed write, unless another write has replaced the awaited promise or#writtenStatushas changed since, as alog()in between redraws the status itself. Without the clear,flush()andwaitForUpdates()keep rejecting with the stale error. A failedlog()write leaves the same stale state:log()now applies the same reset to a redrawn status and clears the stale write promise.Changelog: [Fix] Terminal status updates can resume after a failed write, without losing a status set just after an update finishes, erasing earlier output, or rejecting with a stale error
Test plan:
does not lose a status set just after an update finishes(fails on the previous.finally()version, passes on base and with this change),does not crash on a failed status write with no waiter,does not crash on a failed write during the beforeExit flush,recovers after a failed status write,recovery after a failed status write does not erase earlier logs,retries the same status after a failed status write,flush does not reject with a stale error after a failed status write,flush does not reject with a stale error after a failed log write, anda failed log redraw does not erase earlier output on recoveryfail without the fix and pass with it.yarn jest packages/metro-core/src/__tests__/Terminal-test.jsyarn eslint packages/metro-core/src/Terminal.js packages/metro-core/src/__tests__/Terminal-test.jsyarn prettier --checkon both filesyarn flow check