From d69eaecf25bc2955f59f4c7ad519f4e640321fd9 Mon Sep 17 00:00:00 2001 From: Yann Roberto <1922498827@qq.com> Date: Tue, 6 Oct 2026 00:36:11 +0800 Subject: [PATCH] fix(state-store): keep cleanup diagnostics off caller errors --- scripts/lib/control-pane/work-item-worker.js | 4 +- scripts/lib/state-store/file-lock.js | 25 ++++--- scripts/lib/state-store/index.js | 10 +-- tests/lib/state-store-persistence.test.js | 77 ++++++++++++++++++-- 4 files changed, 94 insertions(+), 22 deletions(-) diff --git a/scripts/lib/control-pane/work-item-worker.js b/scripts/lib/control-pane/work-item-worker.js index 0518a3734..495b00073 100644 --- a/scripts/lib/control-pane/work-item-worker.js +++ b/scripts/lib/control-pane/work-item-worker.js @@ -2,7 +2,7 @@ const { parentPort, workerData } = require('worker_threads'); const { createStateStore } = require('../state-store'); -const { attachCleanupError } = require('../state-store/file-lock'); +const { recordCleanupError } = require('../state-store/file-lock'); const { claimWorkItem, moveWorkItem } = require('./work-item-mutations'); /** Complete one board mutation away from the HTTP event loop, then close. */ @@ -18,7 +18,7 @@ async function mutate() { catch (error) { failed = true; primary = error; } try { store.close(); } catch (error) { - if (failed) attachCleanupError(primary, 'closeError', error); + if (failed) recordCleanupError(primary, 'closeError', error); else { failed = true; primary = error; } } if (failed) throw primary; diff --git a/scripts/lib/state-store/file-lock.js b/scripts/lib/state-store/file-lock.js index 5e35fa5f9..6629126a6 100644 --- a/scripts/lib/state-store/file-lock.js +++ b/scripts/lib/state-store/file-lock.js @@ -7,12 +7,19 @@ const { performance } = require('perf_hooks'); const WAIT_BUFFER = new Int32Array(new SharedArrayBuffer(4)); const DEFAULT_TIMEOUT_MS = 5000; -// Cleanup must never replace an arbitrary primary throw (including frozen errors, -// primitives, or objects with throwing property traps). Diagnostics are best effort. -function attachCleanupError(primary, name, secondary) { +const CLEANUP_ERRORS = new WeakMap(); +const EMPTY_CLEANUP_ERRORS = Object.freeze({}); + +// Preserve the original throw's identity and properties, including frozen errors +// and proxies. Primitive throws cannot own diagnostics and are left unchanged. +function recordCleanupError(primary, name, secondary) { if (primary === null || (typeof primary !== 'object' && typeof primary !== 'function')) return; - try { Object.defineProperty(primary, name, { value: secondary, configurable: true }); } - catch (_error) { /* Preserve the primary when it cannot accept diagnostics. */ } + CLEANUP_ERRORS.set(primary, Object.freeze({ ...getCleanupErrors(primary), [name]: secondary })); +} + +// Frozen snapshots are local to this module instance; they do not cross workers. +function getCleanupErrors(primary) { + return CLEANUP_ERRORS.get(primary) || EMPTY_CLEANUP_ERRORS; } function hasCode(error, code) { @@ -53,7 +60,7 @@ function releaseOwnedLock(lockPath, descriptor, identity) { fs.closeSync(descriptor); // Exactly one attempt, even after a release failure. } catch (error) { closeFailed = true; - if (failed) attachCleanupError(primary, 'closeError', error); + if (failed) recordCleanupError(primary, 'closeError', error); else { failed = true; primary = error; } } if (pendingDelete && !closeFailed) { @@ -100,7 +107,7 @@ function acquireLock(dbPath, timeoutMs) { if (identity) releaseOwnedLock(lockPath, descriptor, identity); else fs.closeSync(descriptor); } catch (releaseError) { - attachCleanupError(error, 'releaseError', releaseError); + recordCleanupError(error, 'releaseError', releaseError); } throw error; } @@ -127,10 +134,10 @@ function withStateStoreLock(dbPath, callback, { timeoutMs = DEFAULT_TIMEOUT_MS } release(); } catch (releaseError) { if (!failed) throw releaseError; - attachCleanupError(primaryError, 'releaseError', releaseError); + recordCleanupError(primaryError, 'releaseError', releaseError); } if (failed) throw primaryError; return result; } -module.exports = { withStateStoreLock, attachCleanupError }; +module.exports = { withStateStoreLock, recordCleanupError, getCleanupErrors }; diff --git a/scripts/lib/state-store/index.js b/scripts/lib/state-store/index.js index 93ceed4ca..8c0573a6a 100644 --- a/scripts/lib/state-store/index.js +++ b/scripts/lib/state-store/index.js @@ -5,7 +5,7 @@ const crypto = require('crypto'); const os = require('os'); const path = require('path'); const initSqlJs = require('sql.js'); -const { withStateStoreLock, attachCleanupError } = require('./file-lock'); +const { withStateStoreLock, recordCleanupError } = require('./file-lock'); const { applyMigrations, getAppliedMigrations } = require('./migrations'); const { createQueryApi } = require('./queries'); @@ -229,7 +229,7 @@ function wrapSqlJsDatabase(SQL, dbPath) { } catch (error) { // The replacement is not adopted until both initialization and the old // handle's close succeed. Never reuse an uncertain previous handle. - try { latest.close(); } catch (closeError) { attachCleanupError(error, 'closeError', closeError); } + try { latest.close(); } catch (closeError) { recordCleanupError(error, 'closeError', closeError); } throw error; } rawDb = latest; @@ -324,9 +324,9 @@ function wrapSqlJsDatabase(SQL, dbPath) { } catch (rollbackError) { // Never reuse an uncertain transaction, including an in-memory store. closed = true; - try { rawDb.close(); } catch (closeError) { attachCleanupError(error, 'closeError', closeError); } + try { rawDb.close(); } catch (closeError) { recordCleanupError(error, 'closeError', closeError); } rawDb = null; - attachCleanupError(error, 'rollbackError', rollbackError); + recordCleanupError(error, 'rollbackError', rollbackError); } dirty = previouslyDirty; throw error; @@ -398,7 +398,7 @@ async function createStateStore(options = {}) { try { appliedMigrations = db.withSnapshot(() => applyMigrations(db)); } catch (error) { - try { db.close(); } catch (closeError) { attachCleanupError(error, 'closeError', closeError); } + try { db.close(); } catch (closeError) { recordCleanupError(error, 'closeError', closeError); } throw error; } const queryApi = createQueryApi(db); diff --git a/tests/lib/state-store-persistence.test.js b/tests/lib/state-store-persistence.test.js index c2bc046a0..38798b307 100644 --- a/tests/lib/state-store-persistence.test.js +++ b/tests/lib/state-store-persistence.test.js @@ -18,7 +18,7 @@ function loadPrivate(filename, overrides = {}, source = fs.readFileSync(filename return module.exports; } const lockModule = loadPrivate(LOCK, { fs }); -const { withStateStoreLock } = lockModule; +const { withStateStoreLock, recordCleanupError, getCleanupErrors } = lockModule; // sql.js is only initialized by the original integration cases, never the pure mode. const { createStateStore } = loadPrivate(STORE, { fs, './file-lock': lockModule }); @@ -48,7 +48,8 @@ function lockFixture(overrides = {}, platform = 'linux', performance = require(' } const lock = loadPrivate(LOCK, { fs: facade, os: { hostname: () => 'fixture' }, perf_hooks: { performance } }, undefined, { platform, pid: 7 }); - return { ...state, calls, run: (callback, timeoutMs = 0) => lock.withStateStoreLock('synthetic.db', callback, { timeoutMs }) }; + return { ...state, calls, getCleanupErrors: lock.getCleanupErrors, + run: (callback, timeoutMs = 0) => lock.withStateStoreLock('synthetic.db', callback, { timeoutMs }) }; } async function runSynthetic(test) { @@ -154,7 +155,7 @@ async function runSynthetic(test) { const result = thrownBy(() => fixture.run(() => {})); assert.ok(result.failed); assert.strictEqual(result.error, denied); - assert.strictEqual(result.error.closeError, cleanup); + assert.strictEqual(fixture.getCleanupErrors(result.error).closeError, cleanup); assert.deepStrictEqual(fixture.calls.slice(3), ['lstat', 'close']); }); await test('initial missing lock is loss after descriptor closure', () => { @@ -172,8 +173,11 @@ async function runSynthetic(test) { assert.strictEqual(result.error, denied); assert.deepStrictEqual(fixture.calls.slice(3), ['lstat', 'unlink', 'close']); }); + const revoked = Proxy.revocable({}, {}); + revoked.revoke(); const primaries = [null, undefined, false, 0, '', 'primitive', Object.freeze(new Error('frozen')), - Object.defineProperty(new Error('setter'), 'releaseError', { set() { throw new Error('setter must not mask'); } })]; + Object.defineProperty(new Error('setter'), 'releaseError', { set() { throw new Error('setter must not mask'); } }), + revoked.proxy]; for (const [index, primary] of primaries.entries()) { for (const stage of ['callback', 'metadata', 'identity']) { await test(`arbitrary primary survives lock cleanup (${index}, ${stage})`, () => { @@ -187,6 +191,10 @@ async function runSynthetic(test) { const result = thrownBy(() => fixture.run(() => { if (stage === 'callback') throw primary; assert.fail('callback must not run'); })); assert.ok(result.failed); assert.strictEqual(result.error, primary); + const diagnostics = fixture.getCleanupErrors(primary); + if (primary !== null && typeof primary === 'object') assert.strictEqual(diagnostics.releaseError, cleanup); + else assert.deepStrictEqual(diagnostics, {}); + assert.ok(Object.isFrozen(diagnostics)); }); } for (const stage of ['migration', 'rollback']) { @@ -197,8 +205,10 @@ async function runSynthetic(test) { run(sql) { if (sql === 'ROLLBACK') throw secondary; } close() { closes++; throw secondary; } } }; + const cleanupLock = loadPrivate(LOCK); const api = loadPrivate(STORE, { 'sql.js': async () => fakeSQL, + './file-lock': cleanupLock, './migrations': { applyMigrations() { if (stage === 'migration') throw primary; return []; } }, './queries': { createQueryApi: () => ({}) }, }); @@ -211,9 +221,64 @@ async function runSynthetic(test) { assert.ok(failed); assert.strictEqual(caught, primary); assert.strictEqual(closes, 1); + const diagnostics = cleanupLock.getCleanupErrors(primary); + if (primary !== null && typeof primary === 'object') { + assert.strictEqual(diagnostics.closeError, secondary); + if (stage === 'rollback') assert.strictEqual(diagnostics.rollbackError, secondary); + } else assert.deepStrictEqual(diagnostics, {}); + assert.ok(Object.isFrozen(diagnostics)); }); } } + await test('cleanup diagnostics preserve caller descriptors without invoking accessors', () => { + let accesses = 0; + const primary = Object.defineProperty(new Error('operation failed'), 'releaseError', { + configurable: true, + enumerable: true, + get() { accesses++; throw new Error('getter must not run'); }, + set() { accesses++; throw new Error('setter must not run'); }, + }); + const before = Object.getOwnPropertyDescriptors(primary); + const cleanup = new Error('unlink failed'); + const fixture = lockFixture({ unlinkSync() { throw cleanup; } }); + const result = thrownBy(() => fixture.run(() => { throw primary; })); + assert.ok(result.failed); + assert.strictEqual(result.error, primary); + assert.deepStrictEqual(Object.getOwnPropertyDescriptors(primary), before); + assert.strictEqual(accesses, 0); + assert.strictEqual(fixture.getCleanupErrors(primary).releaseError, cleanup); + }); + await test('successive cleanup diagnostics are immutable snapshots isolated by primary', () => { + const primary = Object.freeze(new Error('primary')); + const other = () => {}; + const closeError = new Error('close failed'); + const rollbackError = new Error('rollback failed'); + const untouched = getCleanupErrors(primary); + assert.deepStrictEqual(untouched, {}); + recordCleanupError(primary, 'closeError', closeError); + const first = getCleanupErrors(primary); + recordCleanupError(primary, 'rollbackError', rollbackError); + const second = getCleanupErrors(primary); + assert.notStrictEqual(first, second); + assert.deepStrictEqual(untouched, {}); + assert.deepStrictEqual(first, { closeError }); + assert.deepStrictEqual(second, { closeError, rollbackError }); + assert.ok(!Object.isFrozen(closeError)); + assert.ok(!Object.isFrozen(rollbackError)); + assert.ok([untouched, first, second].every(Object.isFrozen)); + assert.throws(() => { first.closeError = rollbackError; }, TypeError); + recordCleanupError(primary, 'closeError', rollbackError); + const third = getCleanupErrors(primary); + assert.notStrictEqual(third, second); + assert.deepStrictEqual(second, { closeError, rollbackError }); + assert.deepStrictEqual(third, { closeError: rollbackError, rollbackError }); + assert.ok(Object.isFrozen(third)); + assert.deepStrictEqual(getCleanupErrors(other), {}); + recordCleanupError(other, 'releaseError', rollbackError); + assert.deepStrictEqual(getCleanupErrors(other), { releaseError: rollbackError }); + assert.strictEqual(getCleanupErrors(primary), third); + assert.deepStrictEqual(Object.keys(primary), []); + }); for (const stage of ['initialize', 'previous-close']) { for (const [index, primary] of [null, undefined, Object.freeze(new Error('reload primary'))].entries()) { await test(`reload closes its unadopted handle and preserves ${stage} failure (${index})`, () => { @@ -503,7 +568,7 @@ async function run() { assert.throws(() => store._database.transaction(() => { store._database.exec('ROLLBACK'); throw failure; - })(), error => error === failure && error.rollbackError instanceof Error); + })(), error => error === failure && getCleanupErrors(error).rollbackError instanceof Error); assert.throws(() => store.listWorkItems(), /closed/); store.close(); }); @@ -545,7 +610,7 @@ async function run() { }; try { assert.throws(() => withStateStoreLock(dbPath, () => { throw primary; }), - error => error === primary && error.releaseError === cleanup); + error => error === primary && getCleanupErrors(error).releaseError === cleanup); } finally { fs.unlinkSync = unlink; unlink(lock);