From d904edc6156bc056f39efc0d8be967995a10bc10 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Tue, 19 May 2026 09:44:53 +0900 Subject: [PATCH] test(lib): make concurrent-write test actually concurrent + use regex matcher for assert.throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two round-1 review findings in `tests/lib/session-bridge.test.js`, both about test correctness rather than the underlying fix: 1. **greptile P1 + coderabbitai Major + cubic P2 (all three): concurrent-write test ran sequentially.** The test spawned two child processes with two consecutive `spawnSync` calls. Because `spawnSync` blocks until the child exits, the second writer started *after* the first finished — the two writers never overlapped, so the rename race the fix targets was never actually exercised. The test would have passed with the old broken `${target}.tmp` suffix. Fix: introduce a one-off "race runner" helper that runs inside its own subprocess and uses async `spawn` to start both writers simultaneously. The runner waits for both to exit (the event loop is local to the runner subprocess, so this stays compatible with the synchronous test harness used elsewhere in this file) and reports both exit codes plus stderrs on stdout. The test then calls the runner via `spawnSync` and parses the result. Both writer children now overlap for the duration of their 200 `writeBridgeAtomic` calls each, which is enough wall time to reliably trigger the rename race against the pre-fix code. Verified: with the fixed `${target}.${pid}.${nonce}.tmp` suffix, the test passes; with the old fixed `${target}.tmp` suffix reintroduced, it fails as expected (one writer hits ENOENT on roughly half its rename calls). 2. **greptile P2 + cubic P3: `assert.throws` used a string as the second argument.** Node deprecated passing a string as the second argument to `assert.throws` years ago: the string is silently treated as the assertion failure message (what to print when the function does *not* throw) rather than as an error matcher. The check passed for any thrown error, not just the rename failure. Fix: pass a regex matcher as the second arg and keep the explanatory text as the third. The regex matches `EISDIR`, `EPERM`, `ENOTDIR`, or `ENOENT` because `renameSync` of a regular tmp file onto an existing directory raises different codes on Linux / macOS / BSD — making the matcher portable across CI runners. Test count unchanged at 14; `npm test` green; `npm run lint` clean. The two helper files (`tests/__tmp_bridge_writer.js`, `tests/__tmp_bridge_race_runner.js`) are written and unlinked inside the test's try/finally so they never persist beyond the test run. --- tests/lib/session-bridge.test.js | 57 +++++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/tests/lib/session-bridge.test.js b/tests/lib/session-bridge.test.js index 6e57f8fc..632c2fdc 100644 --- a/tests/lib/session-bridge.test.js +++ b/tests/lib/session-bridge.test.js @@ -151,10 +151,19 @@ function runTests() { if ( test('concurrent writeBridgeAtomic does not throw ENOENT or corrupt the bridge file', () => { + // Spawn two child processes that BOTH stay alive at the same time + // and call writeBridgeAtomic in a tight loop. `spawnSync` would + // run them sequentially (blocking on each), which would never + // exercise the race the fix targets. Instead a sync runner script + // launches both as async `spawn` children inside its own process, + // waits for both to exit, and reports their statuses on stdout — + // and the test calls *that* runner via `spawnSync`. The runner is + // the only place that needs the event loop. const { spawnSync } = require('child_process'); const path = require('path'); const testId = `test-bridge-race-${Date.now()}-${process.pid}`; const writerPath = path.join(__dirname, '..', '__tmp_bridge_writer.js'); + const runnerPath = path.join(__dirname, '..', '__tmp_bridge_race_runner.js'); const bridgeLib = path.join(__dirname, '..', '..', 'scripts', 'lib', 'session-bridge'); fs.writeFileSync( writerPath, @@ -167,13 +176,38 @@ function runTests() { ].join('\n'), 'utf8' ); + fs.writeFileSync( + runnerPath, + [ + "'use strict';", + "const { spawn } = require('child_process');", + "const [, , writerPath, sid] = process.argv;", + "const c1 = spawn(process.execPath, [writerPath, sid, 'A'], { stdio: ['ignore','pipe','pipe'] });", + "const c2 = spawn(process.execPath, [writerPath, sid, 'B'], { stdio: ['ignore','pipe','pipe'] });", + "const exits = {};", + "const stderrs = { A: '', B: '' };", + "c1.stderr.on('data', chunk => { stderrs.A += chunk.toString(); });", + "c2.stderr.on('data', chunk => { stderrs.B += chunk.toString(); });", + "let done = 0;", + "function onExit(tag) { return function(code) { exits[tag] = code; if (++done === 2) finish(); }; }", + "c1.on('exit', onExit('A'));", + "c2.on('exit', onExit('B'));", + "function finish() {", + " process.stdout.write(JSON.stringify({ exits, stderrs }));", + " process.exit(0);", + "}", + ].join('\n'), + 'utf8' + ); try { - const r1 = spawnSync('node', [writerPath, testId, 'A'], { encoding: 'utf8' }); - const r2 = spawnSync('node', [writerPath, testId, 'B'], { encoding: 'utf8' }); - assert.strictEqual(r1.status, 0, - `writer A should exit 0 (no ENOENT), got ${r1.status}: ${r1.stderr}`); - assert.strictEqual(r2.status, 0, - `writer B should exit 0 (no ENOENT), got ${r2.status}: ${r2.stderr}`); + const result = spawnSync('node', [runnerPath, writerPath, testId], { encoding: 'utf8' }); + assert.strictEqual(result.status, 0, + `race runner should exit 0, got ${result.status}: ${result.stderr}`); + const parsed = JSON.parse(result.stdout); + assert.strictEqual(parsed.exits.A, 0, + `writer A should exit 0 (no ENOENT), got ${parsed.exits.A}: ${parsed.stderrs.A}`); + assert.strictEqual(parsed.exits.B, 0, + `writer B should exit 0 (no ENOENT), got ${parsed.exits.B}: ${parsed.stderrs.B}`); // Final file must be parseable JSON and belong to one of the writers. const final = readBridge(testId); assert.ok(final && typeof final === 'object', @@ -183,6 +217,7 @@ function runTests() { } finally { try { fs.unlinkSync(getBridgePath(testId)); } catch { /* ignore */ } try { fs.unlinkSync(writerPath); } catch { /* ignore */ } + try { fs.unlinkSync(runnerPath); } catch { /* ignore */ } } }) ) @@ -202,8 +237,14 @@ function runTests() { // Plant a directory at the target path so renameSync (target.tmp → target) fails. fs.mkdirSync(target); try { - assert.throws(() => writeBridgeAtomic(testId, { x: 1 }), - 'expected rename failure to surface'); + assert.throws( + () => writeBridgeAtomic(testId, { x: 1 }), + // renameSync of a regular file onto an existing directory throws + // EISDIR on Linux, EPERM on macOS, ENOTDIR on some BSDs. Accept + // any of those so the test stays portable across CI runners. + /EISDIR|EPERM|ENOTDIR|ENOENT/, + 'expected rename failure to surface' + ); // Count any leaked tmp files. The pid+nonce suffix is unique per // call, so we look for any matching pattern under os.tmpdir(). const prefix = path.basename(target) + '.' + process.pid + '.';