morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #4458] [Bug] Stable artifact reads lose descriptor ownership and operation outcomes when close rejects #1770

New issue

Closed

opened 2026-10-16 21:10:04 +00:00 by bsakaguchi · 2 comments

bsakaguchi commented

2026-10-16 21:10:04 +00:00

Owner

Originally created by @morluto on GitHub (Oct 16, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/4458

Before submitting

Searched open/closed issues and PRs for readStableArtifact close and descriptor ownership. Related #4266 fixed stable hashing through a different helper; #4457 concerns PE operation admission, not this read/close boundary. Parent lifecycle audit: #4227.
No credentials, private targets, or ambient environment are included.

Area

Evidence, artifacts, or reconstruction

Observed behavior

On main fe2a398dd, readStableArtifact without a resource scope can return only a bare close rejection while its real descriptor remains open. The error carries no cleanup owner or completed/failed operation outcome. A close rejection also replaces an earlier read failure. With a scope, the descriptor is retained, but the failure ?? cleanup.cause selection loses a primary thrown undefined value.

Observed with actual temporary regular files and an injected close failure that occurs before native close. The probe joins cleanup on every case using the original exact handle. This proves the contract gap, not the frequency of native close failures. No Rust binary or other real provider was exercised.

Reproduction

Build the checkout with Node 24.18.0 and npm run build:cached. Save the following as repro.mjs at the checkout root and run node repro.mjs.

Complete bounded real-descriptor probe

import assert from 'node:assert/strict';
import { lstat, open, mkdtemp, writeFile, rm } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { readStableArtifact } from './dist/artifacts/readStableArtifact.js';
import { ArtifactResourceScope } from './dist/artifacts/ArtifactResourceScope.js';

const root = await mkdtemp(join(tmpdir(), 'rea-stable-close-'));
const path = join(root, 'input.bin');
await writeFile(path, 'caller-selected evidence');
try {
  for (const kind of ['completed', 'primary-error', 'undefined-failure', 'scoped-undefined-failure']) {
    const closeFailure = new Error('controlled close rejection');
    const primary = kind === 'primary-error' ? new Error('controlled read rejection') : undefined;
    const scope = kind.startsWith('scoped-') ? new ArtifactResourceScope() : undefined;
    let handle;
    let nativeClose;
    try {
      const caught = await readStableArtifact(path, 1024, undefined, {
        lstat: selected => lstat(selected, { bigint: true }),
        open: async (...args) => {
          handle = await open(...args);
          nativeClose = handle.close.bind(handle);
          handle.close = async () => { throw closeFailure; };
          if (kind !== 'completed') handle.stat = async () => { throw primary; };
          return handle;
        },
      }, scope).then(() => null, cause => cause);
      assert.ok(handle.fd >= 0);
      console.log(JSON.stringify({ kind, descriptorStillLive: true, bareCloseRejection: caught === closeFailure, ownsDescriptor: caught?.owner !== undefined, hasTypedOutcome: caught?.outcome !== undefined, cleanupReported: caught?.cleanup !== undefined, primaryRetained: caught?.cause === primary && primary !== undefined }));
    } finally {
      if (nativeClose) {
        handle.close = nativeClose;
        if (scope) await scope.close();
        else await nativeClose();
      }
    }
  }
} finally {
  await rm(root, { recursive: true, force: true });
}

Executed once on macOS arm64, Node 24.18.0. The three unscoped cases (completed, primary-error, undefined-failure) all produced:

{"descriptorStillLive":true,"bareCloseRejection":true,"ownsDescriptor":false,"hasTypedOutcome":false,"cleanupReported":false,"primaryRetained":false}

The scoped undefined-failure case retained a cleanup observation but exposed no typed operation outcome.

Expected behavior

Every opened descriptor must have a cleanup owner; close failure must retain the exact descriptor and an explicit completed or failed outcome. Preserve the earlier failure even when its value is undefined or null. The completed read's bytes and digest must remain available to its owner. Callers must adopt failed cleanup under their actual service/operation lifetime and retry it before admission or shutdown reports release. Reuse the existing regular-file ownership/result primitive rather than add a global retry registry.

Impact and scope

Observed: loss of the cleanup capability and primary/completed result at the shared file-read boundary. Source-derived: RustBinaryProvider.inspect invokes this unscoped branch, and its port/service/CLI/MCP composition currently has no descriptor cleanup lifetime. Child capture/source-map decoder entrypoints also invoke it, although process exit can release their OS descriptors. NOT_ENOUGH_INFO for native close-failure frequency, a full Rust CLI/MCP failure, or real-engine behavior.

Relevant output or evidence

readStableArtifact constructs OwnedFileHandle only when a scope was provided and otherwise executes raw await file.close() in finally. Its scoped branch selects the primary cause with nullish coalescing. RustBinaryProvider supplies no scope. The existing OwnedRegularFile already models a descriptor owner, completed/failed outcome, cancellation, and retention failure explicitly.

Typed outcome or recovery expected

Exercise successful read/failed close, earlier read failure/failed close, thrown undefined/null, cancellation during close, and retry. Keep existing stable identity, symlink, size and source-byte behavior. Trace the owning Rust and one-shot consumers so the corrected helper's failure cannot be dropped at the next boundary.

Originally created by @morluto on GitHub (Oct 16, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4458 ### Before submitting - [x] Searched open/closed issues and PRs for `readStableArtifact close` and descriptor ownership. Related #4266 fixed stable hashing through a different helper; #4457 concerns PE operation admission, not this read/close boundary. Parent lifecycle audit: #4227. - [x] No credentials, private targets, or ambient environment are included. ### Area Evidence, artifacts, or reconstruction ### Observed behavior On main `fe2a398dd`, `readStableArtifact` without a resource scope can return only a bare close rejection while its real descriptor remains open. The error carries no cleanup owner or completed/failed operation outcome. A close rejection also replaces an earlier read failure. With a scope, the descriptor is retained, but the `failure ?? cleanup.cause` selection loses a primary thrown `undefined` value. Observed with actual temporary regular files and an injected close failure that occurs before native close. The probe joins cleanup on every case using the original exact handle. This proves the contract gap, not the frequency of native close failures. No Rust binary or other real provider was exercised. ### Reproduction Build the checkout with Node 24.18.0 and `npm run build:cached`. Save the following as `repro.mjs` at the checkout root and run `node repro.mjs`. <details><summary>Complete bounded real-descriptor probe</summary> ```js import assert from 'node:assert/strict'; import { lstat, open, mkdtemp, writeFile, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { readStableArtifact } from './dist/artifacts/readStableArtifact.js'; import { ArtifactResourceScope } from './dist/artifacts/ArtifactResourceScope.js'; const root = await mkdtemp(join(tmpdir(), 'rea-stable-close-')); const path = join(root, 'input.bin'); await writeFile(path, 'caller-selected evidence'); try { for (const kind of ['completed', 'primary-error', 'undefined-failure', 'scoped-undefined-failure']) { const closeFailure = new Error('controlled close rejection'); const primary = kind === 'primary-error' ? new Error('controlled read rejection') : undefined; const scope = kind.startsWith('scoped-') ? new ArtifactResourceScope() : undefined; let handle; let nativeClose; try { const caught = await readStableArtifact(path, 1024, undefined, { lstat: selected => lstat(selected, { bigint: true }), open: async (...args) => { handle = await open(...args); nativeClose = handle.close.bind(handle); handle.close = async () => { throw closeFailure; }; if (kind !== 'completed') handle.stat = async () => { throw primary; }; return handle; }, }, scope).then(() => null, cause => cause); assert.ok(handle.fd >= 0); console.log(JSON.stringify({ kind, descriptorStillLive: true, bareCloseRejection: caught === closeFailure, ownsDescriptor: caught?.owner !== undefined, hasTypedOutcome: caught?.outcome !== undefined, cleanupReported: caught?.cleanup !== undefined, primaryRetained: caught?.cause === primary && primary !== undefined })); } finally { if (nativeClose) { handle.close = nativeClose; if (scope) await scope.close(); else await nativeClose(); } } } } finally { await rm(root, { recursive: true, force: true }); } ``` </details> Executed once on macOS arm64, Node 24.18.0. The three unscoped cases (`completed`, `primary-error`, `undefined-failure`) all produced: ```json {"descriptorStillLive":true,"bareCloseRejection":true,"ownsDescriptor":false,"hasTypedOutcome":false,"cleanupReported":false,"primaryRetained":false} ``` The scoped undefined-failure case retained a cleanup observation but exposed no typed operation outcome. ### Expected behavior Every opened descriptor must have a cleanup owner; close failure must retain the exact descriptor and an explicit completed or failed outcome. Preserve the earlier failure even when its value is `undefined` or `null`. The completed read's bytes and digest must remain available to its owner. Callers must adopt failed cleanup under their actual service/operation lifetime and retry it before admission or shutdown reports release. Reuse the existing regular-file ownership/result primitive rather than add a global retry registry. ### Impact and scope Observed: loss of the cleanup capability and primary/completed result at the shared file-read boundary. Source-derived: `RustBinaryProvider.inspect` invokes this unscoped branch, and its port/service/CLI/MCP composition currently has no descriptor cleanup lifetime. Child capture/source-map decoder entrypoints also invoke it, although process exit can release their OS descriptors. NOT_ENOUGH_INFO for native close-failure frequency, a full Rust CLI/MCP failure, or real-engine behavior. ### Relevant output or evidence [readStableArtifact](https://github.com/morluto/rea/blob/fe2a398dd/src/artifacts/readStableArtifact.ts) constructs `OwnedFileHandle` only when a scope was provided and otherwise executes raw `await file.close()` in `finally`. Its scoped branch selects the primary cause with nullish coalescing. [RustBinaryProvider](https://github.com/morluto/rea/blob/fe2a398dd/src/rust/RustBinaryProvider.ts) supplies no scope. The existing [OwnedRegularFile](https://github.com/morluto/rea/blob/fe2a398dd/src/filesystem/OwnedRegularFile.ts) already models a descriptor owner, completed/failed outcome, cancellation, and retention failure explicitly. ### Typed outcome or recovery expected Exercise successful read/failed close, earlier read failure/failed close, thrown undefined/null, cancellation during close, and retry. Keep existing stable identity, symlink, size and source-byte behavior. Trace the owning Rust and one-shot consumers so the corrected helper's failure cannot be dropped at the next boundary.

bsakaguchi added the

triage

bug

labels

2026-10-16 21:10:04 +00:00

bsakaguchi commented

2026-10-16 21:10:05 +00:00

Author

Owner

@qorvr commented on GitHub (Oct 16, 2026):

Investigated and resolved the root cause of lost descriptor ownership and suppressed operation outcomes when close rejects in readStableArtifact.

Analysis & Root Cause

Unscoped Handle Rejection Leaks: When no ArtifactResourceScope was provided to readStableArtifact, handle.close() was executed directly in a finally block. A close failure resulted in an unhandled bare rejection, dropping the OwnedFileHandle and leaving the file descriptor open with no mechanism for callers to reclaim ownership or retry cleanup.
Outcome Erasure & Masked Failures: If the file read completed successfully (or previously failed with a typed error or undefined), a trailing close failure overwrote/discarded that outcome rather than reporting both the primary outcome and the cleanup failure observation.
RustBinaryProvider Diagnostic Loss: RustBinaryProvider did not map failed cleanup on artifact read errors to ProviderCleanupError, dropping the affected resource paths.

Fix

Implemented in branch fix/read-stable-artifact-descriptor-ownership (commit 31d8b7706):
- In src/artifacts/ArtifactReader.ts:
  - Added owner?: OwnedFileHandle and outcome?: RegularFileOperationOutcome<unknown> to ArtifactReaderFailureOptions and ArtifactReaderFailure.
  - Updated ArtifactReaderFailure.withCleanup to retain owner, preserve the existing outcome, and preserve exact primary failures (including thrown undefined/null).
- In src/artifacts/readStableArtifact.ts:
  - Wraps the opened file handle into const owner = new OwnedFileHandle(file) immediately.
  - Tracks the operation outcome via RegularFileOperationOutcome<{ bytes: Buffer; sha256: string }>.
  - On unscoped close failure (or scoped cleanup rejection), wraps and throws ArtifactReaderFailure with owner, outcome, and cleanup: { reason, resources: [path] }, ensuring callers retain descriptor ownership and can retry .close().
- In src/rust/RustBinaryProvider.ts:
  - Translates ArtifactReaderFailure with cleanup into ProviderCleanupError preserving resource path metadata.
- Added full test coverage in tests/boundary/filesystem/stableArtifact.test.ts verifying all probe cases (unscoped close rejection with completed payload, primary error preservation, thrown undefined, scoped retry recovery, and RustBinaryProvider cleanup error handling).

@qorvr commented on GitHub (Oct 16, 2026): Investigated and resolved the root cause of lost descriptor ownership and suppressed operation outcomes when close rejects in `readStableArtifact`. ### Analysis & Root Cause 1. **Unscoped Handle Rejection Leaks**: When no `ArtifactResourceScope` was provided to `readStableArtifact`, `handle.close()` was executed directly in a `finally` block. A close failure resulted in an unhandled bare rejection, dropping the `OwnedFileHandle` and leaving the file descriptor open with no mechanism for callers to reclaim ownership or retry cleanup. 2. **Outcome Erasure & Masked Failures**: If the file read completed successfully (or previously failed with a typed error or `undefined`), a trailing close failure overwrote/discarded that outcome rather than reporting both the primary outcome and the cleanup failure observation. 3. **RustBinaryProvider Diagnostic Loss**: `RustBinaryProvider` did not map failed cleanup on artifact read errors to `ProviderCleanupError`, dropping the affected resource paths. ### Fix - Implemented in branch `fix/read-stable-artifact-descriptor-ownership` (commit `31d8b7706`): - In `src/artifacts/ArtifactReader.ts`: - Added `owner?: OwnedFileHandle` and `outcome?: RegularFileOperationOutcome<unknown>` to `ArtifactReaderFailureOptions` and `ArtifactReaderFailure`. - Updated `ArtifactReaderFailure.withCleanup` to retain `owner`, preserve the existing `outcome`, and preserve exact primary failures (including thrown `undefined`/`null`). - In `src/artifacts/readStableArtifact.ts`: - Wraps the opened file handle into `const owner = new OwnedFileHandle(file)` immediately. - Tracks the operation outcome via `RegularFileOperationOutcome<{ bytes: Buffer; sha256: string }>`. - On unscoped close failure (or scoped cleanup rejection), wraps and throws `ArtifactReaderFailure` with `owner`, `outcome`, and `cleanup: { reason, resources: [path] }`, ensuring callers retain descriptor ownership and can retry `.close()`. - In `src/rust/RustBinaryProvider.ts`: - Translates `ArtifactReaderFailure` with `cleanup` into `ProviderCleanupError` preserving resource path metadata. - Added full test coverage in `tests/boundary/filesystem/stableArtifact.test.ts` verifying all probe cases (unscoped close rejection with completed payload, primary error preservation, thrown `undefined`, scoped retry recovery, and `RustBinaryProvider` cleanup error handling).

bsakaguchi referenced this issue

2026-10-17 21:01:31 +00:00

[GH-ISSUE #2383] [Tracker] Audit analysis and debugging coverage for the TIOBE top 100 languages (October 2026) #794

bsakaguchi closed this issue

2026-10-31 00:32:40 +00:00

bsakaguchi commented

2026-10-31 00:32:41 +00:00

Author

Owner

@morluto commented on GitHub (Oct 18, 2026):

Reconciled against current main and added public CLI regression in a8170163b (pushed).

The shared reader now uses exact descriptor owners and explicit physical outcomes for scoped and standalone reads. Rust provider/service shutdown retains and retries these owners; original cleanup markers resolve after confirmed release. Existing tests cover completed bytes/digest, primary Error/null/undefined, cancellation during final close, denied retry, mixed owners, and same-connection MCP recovery. The added actual CLI test uses an authored ELF file and a preload that rejects the first selected descriptor close before native close. It proves the same handle is retried and released before CLI failure projection, with source bytes unchanged and no sticky cleanup-incomplete outcome. Node's attached stream can re-enter native close notification; the probe records the per-handle completed transition once.

Executed: 49 tests passed in five files (stableArtifact, stableArtifactConsumers, rustBinaryCli, rustBinaryMcp, RustBinaryService), npm run check:fast, normal commit/push hooks. No naturally occurring OS close fault, real Rust toolchain artifact, or native Windows/Linux provider qualification is claimed. This closes the original read/close lifetime report; admission reason loss is separately tracked in #4929.

@morluto commented on GitHub (Oct 18, 2026): Reconciled against current main and added public CLI regression in a8170163b (pushed). The shared reader now uses exact descriptor owners and explicit physical outcomes for scoped and standalone reads. Rust provider/service shutdown retains and retries these owners; original cleanup markers resolve after confirmed release. Existing tests cover completed bytes/digest, primary Error/null/undefined, cancellation during final close, denied retry, mixed owners, and same-connection MCP recovery. The added actual CLI test uses an authored ELF file and a preload that rejects the first selected descriptor close before native close. It proves the same handle is retried and released before CLI failure projection, with source bytes unchanged and no sticky cleanup-incomplete outcome. Node's attached stream can re-enter native close notification; the probe records the per-handle completed transition once. Executed: 49 tests passed in five files (stableArtifact, stableArtifactConsumers, rustBinaryCli, rustBinaryMcp, RustBinaryService), npm run check:fast, normal commit/push hooks. No naturally occurring OS close fault, real Rust toolchain artifact, or native Windows/Linux provider qualification is claimed. This closes the original read/close lifetime report; admission reason loss is separately tracked in #4929.

bsakaguchi referenced this issue

2026-10-31 00:40:02 +00:00

[GH-ISSUE #5619] [Bug] Reference-source import excludes files that Git leaves unignored with core.ignorecase=false #3406

bsakaguchi referenced this issue

2026-10-31 00:41:58 +00:00

[GH-ISSUE #5891] [Bug] Reference-source import ignores nested .gitignore files #3569

bsakaguchi referenced this issue

2026-10-31 00:55:16 +00:00

[PR #1770] [MERGED] fix(reference): report sloppy scripts, PHP, C# and enclosing Git facts #4807