fix(opencode): preserve primary failures through complete lock cleanup

2026-09-29 21:15:16 +02:00 · 2026-09-27 22:41:08 -04:00
parent 429384f80c
commit e9911d6668
2 changed files with 212 additions and 8 deletions
@@ -56,18 +56,31 @@ function assertOwnedLock(owned) {
  }
 }

+function annotateCleanupFailure(primary, property, value) {
+  try {
+    // Own data properties avoid invoking caller getters/setters. Frozen values,
+    // primitives and rejecting proxy traps simply retain no extra diagnostic.
+    Object.defineProperty(primary, property, { value, configurable: true, enumerable: true, writable: true });
+  } catch {
+    // Diagnostics must never replace the exact primary thrown value.
+  }
+}
+
 function releaseOwned(ownedLocks) {
-  let primaryError;
+  const failures = [];
  for (const owned of [...ownedLocks].reverse()) {
    try {
      assertOwnedLock(owned);
      owned.release();
    } catch (error) {
-      if (!primaryError) primaryError = error;
-      else (primaryError.releaseErrors ||= []).push(error);
+      failures.push(error);
    }
  }
-  if (primaryError) throw primaryError;
+  // Finish every safe release before touching any caller-owned error object.
+  if (failures.length > 0) {
+    if (failures.length > 1) annotateCleanupFailure(failures[0], 'releaseErrors', failures.slice(1));
+    throw failures[0];
+  }
 }

 function acquireOpenCodeInstallLocks(roots, existingLease) {
@@ -97,7 +110,7 @@ function acquireOpenCodeInstallLocks(roots, existingLease) {
    }
  } catch (error) {
    try { releaseOwned(ownedLocks); }
-    catch (releaseError) { error.releaseError = releaseError; }
+    catch (releaseError) { annotateCleanupFailure(error, 'releaseError', releaseError); }
    throw error;
  }
  const lease = Object.freeze({});
@@ -115,13 +128,17 @@ function acquireOpenCodeInstallLocks(roots, existingLease) {
 function withOpenCodeInstallLocks(roots, callback, existingLease) {
  if (typeof callback !== 'function') throw new TypeError('OpenCode install lock callback must be a function.');
  const holder = acquireOpenCodeInstallLocks(roots, existingLease);
+  let didThrow = false;
  let primaryError;
  let result;
  try { result = callback(holder.lease); }
-  catch (error) { primaryError = error; }
+  catch (error) { didThrow = true; primaryError = error; }
  try { holder.release(); }
-  catch (error) { if (primaryError) primaryError.releaseError = error; else primaryError = error; }
-  if (primaryError) throw primaryError;
+  catch (error) {
+    if (didThrow) annotateCleanupFailure(primaryError, 'releaseError', error);
+    else { didThrow = true; primaryError = error; }
+  }
+  if (didThrow) throw primaryError;
  return result;
 }

@@ -233,5 +233,192 @@ test('engine ownership metadata is immutable and derived from the acquired descr
  } finally { release(); }
  assert.strictEqual(fs.existsSync(`${settingsPath}.ecc.lock`), false);
 });
+
+function captureThrown(callback) {
+  try { return { didThrow: false, result: callback() }; }
+  catch (value) { return { didThrow: true, value }; }
+}
+
+const falsyThrownValues = [undefined, null, false, 0, '', NaN];
+for (const [index, primary] of falsyThrownValues.entries()) {
+  for (const cleanupFails of [false, true]) {
+    test(`falsy callback value ${index} survives ${cleanupFails ? 'failed' : 'healthy'} cleanup`, root => {
+      let lease;
+      const caught = captureThrown(() => withOpenCodeInstallLocks([root], active => {
+        lease = active;
+        if (cleanupFails) {
+          fs.renameSync(lockPath(root), `${lockPath(root)}.owned`);
+          fs.writeFileSync(lockPath(root), 'replacement');
+        }
+        throw primary;
+      }));
+      assert.strictEqual(caught.didThrow, true, 'A thrown falsy value must not become success');
+      assert.ok(Object.is(caught.value, primary), 'Preserve the exact thrown value, including NaN');
+      assert.throws(() => acquireOpenCodeInstallLocks([root], lease), /inactive/);
+      if (cleanupFails) {
+        assert.strictEqual(fs.readFileSync(lockPath(root), 'utf8'), 'replacement');
+        assert.ok(fs.existsSync(`${lockPath(root)}.owned`));
+      } else assert.strictEqual(fs.existsSync(lockPath(root)), false);
+    });
+  }
+}
+
+for (const kind of ['frozen', 'nonextensible', 'nonwritable', 'accessor', 'proxy', 'primitive']) {
+  test(`${kind} callback primary survives cleanup failure without invoking accessors`, root => {
+    const a = path.join(root, 'a');
+    const b = path.join(root, 'b');
+    const marker = new Error('preexisting release diagnostic');
+    let accesses = 0;
+    let definitions = 0;
+    let primary = new Error('primary');
+    if (kind === 'frozen') Object.freeze(primary);
+    if (kind === 'nonextensible') Object.preventExtensions(primary);
+    if (kind === 'nonwritable') Object.defineProperty(primary, 'releaseError', { value: marker });
+    if (kind === 'accessor') Object.defineProperty(primary, 'releaseError', {
+      configurable: true,
+      get() { accesses++; throw new Error('getter must not run'); },
+      set() { accesses++; throw new Error('setter must not run'); }
+    });
+    if (kind === 'proxy') primary = new Proxy(primary, { defineProperty() {
+      definitions++;
+      assert.strictEqual(fs.existsSync(lockPath(a)), false, 'All safe cleanup precedes annotation');
+      throw new Error('annotation rejected');
+    } });
+    if (kind === 'primitive') primary = 'literal primary';
+    let lease;
+    const caught = captureThrown(() => withOpenCodeInstallLocks([a, b], active => {
+      lease = active;
+      fs.renameSync(lockPath(b), `${lockPath(b)}.owned`);
+      fs.writeFileSync(lockPath(b), 'replacement');
+      throw primary;
+    }));
+    assert.strictEqual(caught.didThrow, true);
+    assert.ok(Object.is(caught.value, primary));
+    assert.strictEqual(accesses, 0);
+    if (kind === 'proxy') assert.strictEqual(definitions, 1);
+    if (kind === 'nonwritable') assert.strictEqual(primary.releaseError, marker);
+    if (kind === 'accessor') {
+      const descriptor = Object.getOwnPropertyDescriptor(primary, 'releaseError');
+      assert.ok('value' in descriptor, 'Diagnostic should be an own data property');
+      assert.match(descriptor.value.message, /changed/);
+    }
+    assert.strictEqual(fs.existsSync(lockPath(a)), false);
+    assert.strictEqual(fs.readFileSync(lockPath(b), 'utf8'), 'replacement');
+    assert.ok(fs.existsSync(`${lockPath(b)}.owned`));
+    assert.throws(() => acquireOpenCodeInstallLocks([a], lease), /inactive/);
+  });
+}
+
+for (const kind of ['mutable', 'frozen', 'frozen array', 'non-array', 'readonly', 'accessor', 'proxy']) {
+  test(`three-root cleanup preserves ${kind} first failure and finishes reverse cleanup`, root => {
+    const roots = ['a', 'b', 'c'].map(name => path.join(root, name));
+    const [a, b, c] = roots;
+    const originalRename = fs.renameSync;
+    const secondary = new Error('second cleanup failure');
+    const priorDiagnostic = new Error('prior diagnostic');
+    const originalArray = Object.freeze([priorDiagnostic]);
+    let accesses = 0;
+    let primary = new Error('first cleanup failure');
+    if (kind === 'frozen') Object.freeze(primary);
+    if (kind === 'frozen array') primary.releaseErrors = originalArray;
+    if (kind === 'non-array') primary.releaseErrors = { push() { accesses++; throw new Error('caller push must not run'); } };
+    if (kind === 'readonly') Object.defineProperty(primary, 'releaseErrors', { value: originalArray });
+    if (kind === 'accessor') Object.defineProperty(primary, 'releaseErrors', {
+      get() { accesses++; throw new Error('caller getter must not run'); },
+      set() { accesses++; throw new Error('caller setter must not run'); }
+    });
+    if (kind === 'proxy') primary = new Proxy(primary, { defineProperty() {
+      assert.strictEqual(fs.existsSync(lockPath(a)), false, 'Finish cleanup before a diagnostic trap');
+      throw new Error('diagnostic trap');
+    } });
+    const holder = acquireOpenCodeInstallLocks(roots);
+    const attempts = [];
+    fs.renameSync = (from, to) => {
+      if (roots.some(candidate => lockPath(candidate) === from)) attempts.push(from);
+      if (from === lockPath(c)) throw primary;
+      if (from === lockPath(b)) throw secondary;
+      return originalRename(from, to);
+    };
+    let caught;
+    try { caught = captureThrown(() => holder.release()); }
+    finally { fs.renameSync = originalRename; }
+    assert.strictEqual(caught.didThrow, true);
+    assert.strictEqual(caught.value, primary);
+    assert.deepStrictEqual(attempts, [lockPath(c), lockPath(b), lockPath(a)]);
+    assert.strictEqual(fs.existsSync(lockPath(a)), false);
+    assert.ok(fs.existsSync(lockPath(b)) && fs.existsSync(lockPath(c)), 'Failed releases must not be claimed removed');
+    assert.strictEqual(accesses, 0);
+    assert.deepStrictEqual(originalArray, [priorDiagnostic], 'Never mutate a caller-owned diagnostics array');
+    if (['mutable', 'frozen array', 'non-array'].includes(kind)) {
+      assert.deepStrictEqual(primary.releaseErrors, [secondary]);
+      assert.notStrictEqual(primary.releaseErrors, originalArray);
+    }
+    if (kind === 'readonly') assert.strictEqual(primary.releaseErrors, originalArray);
+    assert.throws(() => acquireOpenCodeInstallLocks([a], holder.lease), /inactive/);
+  });
+}
+
+for (const [index, primary] of falsyThrownValues.entries()) {
+  test(`falsy first cleanup value ${index} is thrown after all remaining roots are attempted`, root => {
+    const roots = ['a', 'b', 'c'].map(name => path.join(root, name));
+    const [a, b, c] = roots;
+    const originalRename = fs.renameSync;
+    const secondary = new Error('second cleanup failure');
+    const attempts = [];
+    let lease;
+    let caught;
+    fs.renameSync = (from, to) => {
+      if (roots.some(candidate => lockPath(candidate) === from)) attempts.push(from);
+      if (from === lockPath(c)) throw primary;
+      if (from === lockPath(b)) throw secondary;
+      return originalRename(from, to);
+    };
+    try { caught = captureThrown(() => withOpenCodeInstallLocks(roots, active => { lease = active; return 'success'; })); }
+    finally { fs.renameSync = originalRename; }
+    assert.strictEqual(caught.didThrow, true);
+    assert.ok(Object.is(caught.value, primary));
+    assert.deepStrictEqual(attempts, [lockPath(c), lockPath(b), lockPath(a)]);
+    assert.strictEqual(fs.existsSync(lockPath(a)), false);
+    assert.ok(fs.existsSync(lockPath(b)) && fs.existsSync(lockPath(c)));
+    assert.throws(() => acquireOpenCodeInstallLocks([a], lease), /inactive/);
+  });
+}
+
+test('frozen acquisition failure survives rollback while replaced ownership is preserved', root => {
+  const roots = ['a', 'b', 'c'].map(name => path.join(root, name));
+  const [a, b, c] = roots;
+  const primary = Object.freeze(new Error('one-shot acquisition validation failure'));
+  const originalLink = fs.linkSync;
+  const originalStat = fs.lstatSync;
+  const originalRename = fs.renameSync;
+  let published = false;
+  let faults = 0;
+  let callbacks = 0;
+  const checked = [];
+  fs.linkSync = (from, to) => { originalLink(from, to); if (to === lockPath(c)) published = true; };
+  fs.lstatSync = (...args) => {
+    if (published && args[0] === c && faults === 0) {
+      faults++;
+      originalRename(lockPath(b), `${lockPath(b)}.owned`);
+      fs.writeFileSync(lockPath(b), 'replacement during rollback');
+      throw primary;
+    }
+    if (faults > 0 && roots.includes(args[0]) && args[1]?.bigint) checked.push(args[0]);
+    return originalStat(...args);
+  };
+  let caught;
+  try { caught = captureThrown(() => withOpenCodeInstallLocks(roots, () => { callbacks++; })); }
+  finally { fs.linkSync = originalLink; fs.lstatSync = originalStat; }
+  assert.strictEqual(caught.didThrow, true);
+  assert.strictEqual(caught.value, primary);
+  assert.strictEqual(faults, 1);
+  assert.strictEqual(callbacks, 0);
+  assert.deepStrictEqual(checked, [c, b, a]);
+  assert.strictEqual(fs.existsSync(lockPath(a)), false);
+  assert.strictEqual(fs.existsSync(lockPath(c)), false);
+  assert.strictEqual(fs.readFileSync(lockPath(b), 'utf8'), 'replacement during rollback');
+  assert.ok(fs.existsSync(`${lockPath(b)}.owned`));
+});
+
 console.log(`Results: Passed: ${passed}, Failed: ${failed}`);
 process.exitCode = failed ? 1 : 0;