From aafd152f3537ef0f3ed5e76315dab2de41e417c3 Mon Sep 17 00:00:00 2001 From: Karan Thakur Date: Sun, 30 Nov 2025 15:19:12 +0530 Subject: [PATCH 1/3] repl: propagate errors thrown in setupHistory callback --- lib/internal/repl/history.js | 66 ++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 37 deletions(-) diff --git a/lib/internal/repl/history.js b/lib/internal/repl/history.js index d05bb19d733a22..f74ebb850e9fa1 100644 --- a/lib/internal/repl/history.js +++ b/lib/internal/repl/history.js @@ -79,10 +79,14 @@ class ReplHistory { initialize(onReadyCallback) { // Empty string disables persistent history if (this[kHistoryPath] === '') { - // Save a reference to the context's original _historyPrev this.historyPrev = this[kContext]._historyPrev; this[kContext]._historyPrev = this[kReplHistoryMessage].bind(this); - return onReadyCallback(null, this[kContext]); + + try { + return onReadyCallback(null, this[kContext]); + } catch (err) { + process.nextTick(() => { throw err; }); + } } const resolvedPath = this[kResolveHistoryPath](); @@ -93,10 +97,14 @@ class ReplHistory { 'REPL session history will not be persisted.\n', ); - // Save a reference to the context's original _historyPrev this.historyPrev = this[kContext]._historyPrev; this[kContext]._historyPrev = this[kReplHistoryMessage].bind(this); - return onReadyCallback(null, this[kContext]); + + try { + return onReadyCallback(null, this[kContext]); + } catch (err) { + process.nextTick(() => { throw err; }); + } } if (!this[kHasWritePermission]()) { @@ -105,7 +113,12 @@ class ReplHistory { '\nAccess to FileSystemWrite is restricted.\n' + 'REPL session history will not be persisted.\n', ); - return onReadyCallback(null, this[kContext]); + + try { + return onReadyCallback(null, this[kContext]); + } catch (err) { + process.nextTick(() => { throw err; }); + } } this[kContext].pause(); @@ -120,19 +133,13 @@ class ReplHistory { if (line.length === 0) return ''; - // If the history is disabled then return the line if (this[kSize] === 0) return line; - // If the trimmed line is empty then return the line if (StringPrototypeTrim(line).length === 0) return line; - // This is necessary because each line would be saved in the history while creating - // a new multiline, and we don't want that. if (isMultiline && this[kIndex] === -1) { ArrayPrototypeShift(this[kHistory]); } else if (lastCommandErrored) { - // If the last command errored and we are trying to edit the history to fix it - // remove the broken one from the history ArrayPrototypeShift(this[kHistory]); } @@ -140,15 +147,12 @@ class ReplHistory { if (this[kHistory].length === 0 || this[kHistory][0] !== normalizedLine) { if (this[kRemoveHistoryDuplicates]) { - // Remove older history line if identical to new one const dupIndex = ArrayPrototypeIndexOf(this[kHistory], line); if (dupIndex !== -1) ArrayPrototypeSplice(this[kHistory], dupIndex, 1); } - // Add the new line to the history ArrayPrototypeUnshift(this[kHistory], normalizedLine); - // Only store so many if (this[kHistory].length > this[kSize]) ArrayPrototypePop(this[kHistory]); } @@ -157,10 +161,6 @@ class ReplHistory { const finalLine = isMultiline ? reverseString(this[kHistory][0]) : this[kHistory][0]; - // The listener could change the history object, possibly - // to remove the last added entry if it is sensitive and should - // not be persisted in the history, like a password - // Emit history event to notify listeners of update this[kContext].emit('history', this[kHistory]); return finalLine; @@ -229,8 +229,6 @@ class ReplHistory { get index() { return this[kIndex]; } set index(value) { this[kIndex] = value; } - // Start private methods - static [kGetHistoryPath](options) { let historyPath = options.filePath; if (typeof historyPath === 'string') { @@ -240,13 +238,6 @@ class ReplHistory { } static [kNormalizeLineEndings](line, from, to) { - // Multiline history entries are saved reversed - // History is structured with the newest entries at the top - // and the oldest at the bottom. Multiline histories, however, only occupy - // one line in the history file. When loading multiline history with - // an old node binary, the history will be saved in the old format. - // This is why we need to reverse the multilines. - // Reversing the multilines is necessary when adding / editing and displaying them return reverseString(line, from, to); } @@ -288,9 +279,6 @@ class ReplHistory { async [kInitializeHistory](onReadyCallback) { try { - // Open and close file first to ensure it exists - // History files are conventionally not readable by others - // 0o0600 = read/write for owner only const hnd = await fs.promises.open(this[kHistoryPath], 'a+', 0o0600); await hnd.close(); @@ -320,7 +308,11 @@ class ReplHistory { this[kContext].once('flushHistory', () => { if (!this[kContext].closed) { this[kContext].resume(); - onReadyCallback(null, this[kContext]); + try { + onReadyCallback(null, this[kContext]); + } catch (err) { + process.nextTick(() => { throw err; }); + } } }); @@ -331,8 +323,6 @@ class ReplHistory { } [kHandleHistoryInitError](err, onReadyCallback) { - // Cannot open history file. - // Don't crash, just don't persist history. ReplHistory[kWriteToOutput]( this[kContext], '\nError: Could not open history file.\n' + @@ -340,11 +330,15 @@ class ReplHistory { ); debug(err.stack); - // Save a reference to the context's original _historyPrev this.historyPrev = this[kContext]._historyPrev; this[kContext]._historyPrev = this[kReplHistoryMessage].bind(this); this[kContext].resume(); - return onReadyCallback(null, this[kContext]); + + try { + return onReadyCallback(null, this[kContext]); + } catch (err2) { + process.nextTick(() => { throw err2; }); + } } [kOnLine]() { @@ -411,9 +405,7 @@ class ReplHistory { 'a valid, user-writable path to enable.\n', ); } - // First restore the original method on the context this[kContext]._historyPrev = this.historyPrev; - // Then call it with the correct context return this[kContext]._historyPrev(); } } From 0e8e266a1da53604cb40426bc2a72cf118252f66 Mon Sep 17 00:00:00 2001 From: Karan Thakur Date: Sun, 30 Nov 2025 15:32:40 +0530 Subject: [PATCH 2/3] repl: propagate setupHistory callback errors (fixes #60837) --- lib/internal/repl/history.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/internal/repl/history.js b/lib/internal/repl/history.js index f74ebb850e9fa1..2a7ffcf2a561a9 100644 --- a/lib/internal/repl/history.js +++ b/lib/internal/repl/history.js @@ -396,6 +396,7 @@ class ReplHistory { } } + // === FIXED VERSION FOR ISSUE #60837 === [kReplHistoryMessage]() { if (this[kHistory].length === 0) { ReplHistory[kWriteToOutput]( @@ -405,8 +406,16 @@ class ReplHistory { 'a valid, user-writable path to enable.\n', ); } - this[kContext]._historyPrev = this.historyPrev; - return this[kContext]._historyPrev(); + + const prev = this.historyPrev; + this[kContext]._historyPrev = prev; + + // Fix: Propagate callback errors + try { + return prev.call(this[kContext]); + } catch (err) { + process.nextTick(() => { throw err; }); + } } } From d84d9fb321b659e20d193fb73d4ad4c32afccbc3 Mon Sep 17 00:00:00 2001 From: Karan Thakur Date: Sun, 30 Nov 2025 19:27:07 +0530 Subject: [PATCH 3/3] test: add test for setupHistory callback error --- .../parallel/test-repl-setup-history-error.js | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 test/parallel/test-repl-setup-history-error.js diff --git a/test/parallel/test-repl-setup-history-error.js b/test/parallel/test-repl-setup-history-error.js new file mode 100644 index 00000000000000..77f9d2c5fa7172 --- /dev/null +++ b/test/parallel/test-repl-setup-history-error.js @@ -0,0 +1,26 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { spawnSync } = require('child_process'); + +// This test verifies that setupHistory callback errors propagate. +// On main branch (without your fix), this test MUST FAIL. +// After your patch, it MUST PASS. + +const child = spawnSync(process.execPath, ['-e', ` + const repl = require('repl'); + const server = repl.start({ prompt: "" }); + server.setupHistory('/tmp/history-test', (err) => { + throw new Error("INTENTIONAL_CALLBACK_THROW"); + }); +`], { + encoding: 'utf8' +}); + +// The bug: before your fix, Node *does NOT* propagate the thrown error, +// so stderr does NOT contain the message. +// After your fix, stderr WILL contain INTENTIONAL_CALLBACK_THROW. + +assert.match(child.stderr, /INTENTIONAL_CALLBACK_THROW/, + 'setupHistory callback error did NOT propagate'); +