morluto/rea

Fork 0

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

[GH-ISSUE #6033] [Bug] Output-tree rollback retry stats an already removed file through its closed handle #3681

New issue

Closed

opened 2026-10-31 00:43:00 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 00:43:00 +00:00

Owner

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

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Evidence, artifacts, or reconstruction

Observed behavior

After a failed output write and a partially successful rollback, retrying cleanup fails with EBADF / fstat / file closed, even after the original filesystem obstruction is removed.

A real filesystem test streamed one byte while expecting two. The first rollback removed the partial file, then failed to remove the root with EACCES because its parent was not writable. Restoring the parent's permissions did not recover the same tree: retry attempted to stat the removed file's closed handle and left the empty output root with cleanup status incomplete. The otherwise identical successful-write control completed its second rollback.

Reproduction

Run as a non-root account on a POSIX filesystem that enforces directory permissions. Set REA_VERIFY_ROOT to an existing empty disposable directory. With locked dependencies installed, save the test below as tests/boundary/filesystem/outputTreeRetry.test.ts, then run:

node node_modules/vitest/vitest.mjs run tests/boundary/filesystem/outputTreeRetry.test.ts --maxWorkers=1 --no-file-parallelism

This is the output-tree portion of an executed focused test, with unrelated imports and cases removed. It ran twice, on Node 26.12.0 and the pinned Node 24.18.0. Both runs failed the failed-write retry assertion and passed the completed-write control. File creation, descriptor closure, permission denial, unlink, and rmdir are real operations; none is mocked.

import { createHash } from "node:crypto";
import { access, chmod, mkdtemp, readdir, rm } from "node:fs/promises";
import { join } from "node:path";
import { Readable } from "node:stream";
import { expect, it } from "vitest";
import { ArtifactReaderFailure } from "../../../src/artifacts/ArtifactReader.js";
import { SafeOutputTree } from "../../../src/artifacts/SafeOutputTree.js";
const scratch = process.env.REA_VERIFY_ROOT!;
it.each([false, true])("rollback resumes after root removal denial with failed write=%s", async (failedWrite) => {
  const parent = await mkdtemp(join(scratch, "tree-"));
  const output = join(parent, "output"); const tree = await SafeOutputTree.create(output);
  const bytes = Buffer.from("x");
  try {
    const write = tree.write("partial.txt", Readable.from([bytes]), {
      bytes: failedWrite ? 2 : 1, sha256: createHash("sha256").update(bytes).digest("hex"),
    });
    if (failedWrite) await expect(write).rejects.toMatchObject({ reason: "integrity" }); else await write;
    await chmod(parent, 0o500);
    const first = await tree.rollback().catch((cause: unknown) => cause);
    expect(first).toBeInstanceOf(ArtifactReaderFailure);
    expect((first as Error).cause).toMatchObject({ code: "EACCES", syscall: "rmdir" });
    expect(await readdir(output)).toEqual([]);
    await chmod(parent, 0o700);
    const second = await tree.rollback().catch((cause: unknown) => cause);
    console.log(JSON.stringify({ case: "rollback", failedWrite,
      firstCauseCode: (first as any).cause?.code, fileRemovedByFirst: true,
      retryStatus: (second as any).status ?? null, retryCauseCode: (second as any).cause?.code ?? null,
      retryMessage: second instanceof Error ? second.message : null,
      rootRemains: await access(output).then(() => true, () => false), cleanup: tree.cleanup }));
    expect(second).toEqual({ status: "complete", residualPaths: [] });
  } finally {
    await chmod(parent, 0o700); await tree.closeHandles(); await rm(parent, { recursive: true, force: true });
  }
});

The finally block restores permissions, closes remaining handles and removes the fixture. Remove the temporary test afterward.

Expected behavior

After the obstruction is removed, the retained owner should continue from its confirmed cleanup progress and return { status: "complete", residualPaths: [] }. Files already unlinked must not be re-admitted using closed descriptors. The lifecycle contract requires retrying unresolved children while retaining successful progress; the cleanup regression guidance specifically requires restoring the denied resource and verifying the owning retry.

Impact and scope

P2: a transient removal failure becomes a persistent cleanup failure for a tree containing a failed write. In the reproduction only an empty operation-owned root remains; no user-data loss or security boundary crossing was observed. Manual removal of that known empty residual and abandoning the affected scope is possible, but retrying the same owner is broken.

Source inspection shows ArtifactResourceScope retains output-tree owners and retries rollback before admitting new work, so this can keep the affected scope blocked after the original permissions are restored. The scope/public CLI/MCP behavior was not executed here. Runtime coverage is macOS with a size-mismatch write; cancellation, Linux, Windows and other obstruction timings were not tested. Natural incidence is unknown.

REA version

rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e.

Runtime and platform

macOS 27.0.1 arm64; Node.js v26.12.0 and repository-pinned v24.18.0; locked Vitest 5.0.3; non-root account and actual local filesystem permissions.

Target and artifact context

One synthetic regular file in an operation-created output tree. A size-mismatch write leaves its descriptor pending; the output parent is temporarily changed to mode 0500 and restored to 0700.

Interface

Other

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Executed observations on both Node versions:

Failed write: ArtifactReaderFailure, reason=integrity
First rollback: cause.code=EACCES, cause.syscall=rmdir
Output directory after first rollback: []
Parent permissions restored: 0700
Second rollback: ArtifactReaderFailure, reason=integrity
Second rollback cause: code=EBADF, syscall=fstat, message="file closed"
Second rollback message: "Extraction output cleanup could not be verified: file closed"
Root still exists; cleanup={status:"incomplete", residualPaths:["output"]}
Completed-write control: second rollback={status:"complete", residualPaths:[]}; root absent

This was an exported production filesystem boundary test. Source-derived public reachability: artifact materialization creates and writes this tree, failed extraction cleanup retains its output-tree owner, and scope cleanup calls that same tree’s rollback.

Typed outcome or recovery expected

Keep the first permission failure attributed to the output root. Once the obstruction is removed, resolve the same retained cleanup owner and report no residual paths. Preserve the initiating write-integrity diagnostic independently of cleanup.

Analysis or suspected cause

A failed write keeps its destination in openFiles. rollback admits pending files and closes their handles before removal. closeHandles removes owners from openHandles but does not clear openFiles; only the successful-write closeFileHandle path clears that map. removal deletes the ownedFiles identity immediately after unlink and can then fail at root rmdir. On retry, pending-file admission sees the stale openFiles record without an ownedFiles identity and calls readFileIdentity on the closed handle. This matches the observed fstat EBADF.

Additional context

Fetched upstream main 6d356b84344659085f2b912857b5e27fe0160c9b and compared SafeOutputTree and SafeOutputTreeCleanup: unchanged from the executed baseline. The newer revision was not executed.

#5898 concerns syncing nested directories before successful publication, not rollback state. #5956 concerns preserving export cancellation classification after rollback failure; its owner retry succeeds. #4483 and PR #4493 concern readback failure/cancellation precedence, #4928 concerns stale error markers after successful owner release, and #137 concerns mutable cleanup getters. PR #5672 changes source error lifetime and cleanup composition but does not repair openFiles admission after partial removal. No matching issue or PR was found. Last known good revision and natural failure frequency: NOT_ENOUGH_INFO.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/6033 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area Evidence, artifacts, or reconstruction ### Observed behavior After a failed output write and a partially successful rollback, retrying cleanup fails with `EBADF` / `fstat` / `file closed`, even after the original filesystem obstruction is removed. A real filesystem test streamed one byte while expecting two. The first rollback removed the partial file, then failed to remove the root with `EACCES` because its parent was not writable. Restoring the parent's permissions did not recover the same tree: retry attempted to stat the removed file's closed handle and left the empty output root with cleanup status `incomplete`. The otherwise identical successful-write control completed its second rollback. ### Reproduction Run as a non-root account on a POSIX filesystem that enforces directory permissions. Set `REA_VERIFY_ROOT` to an existing empty disposable directory. With locked dependencies installed, save the test below as `tests/boundary/filesystem/outputTreeRetry.test.ts`, then run: ```sh node node_modules/vitest/vitest.mjs run tests/boundary/filesystem/outputTreeRetry.test.ts --maxWorkers=1 --no-file-parallelism ``` This is the output-tree portion of an executed focused test, with unrelated imports and cases removed. It ran twice, on Node 26.12.0 and the pinned Node 24.18.0. Both runs failed the failed-write retry assertion and passed the completed-write control. File creation, descriptor closure, permission denial, unlink, and rmdir are real operations; none is mocked. ```ts import { createHash } from "node:crypto"; import { access, chmod, mkdtemp, readdir, rm } from "node:fs/promises"; import { join } from "node:path"; import { Readable } from "node:stream"; import { expect, it } from "vitest"; import { ArtifactReaderFailure } from "../../../src/artifacts/ArtifactReader.js"; import { SafeOutputTree } from "../../../src/artifacts/SafeOutputTree.js"; const scratch = process.env.REA_VERIFY_ROOT!; it.each([false, true])("rollback resumes after root removal denial with failed write=%s", async (failedWrite) => { const parent = await mkdtemp(join(scratch, "tree-")); const output = join(parent, "output"); const tree = await SafeOutputTree.create(output); const bytes = Buffer.from("x"); try { const write = tree.write("partial.txt", Readable.from([bytes]), { bytes: failedWrite ? 2 : 1, sha256: createHash("sha256").update(bytes).digest("hex"), }); if (failedWrite) await expect(write).rejects.toMatchObject({ reason: "integrity" }); else await write; await chmod(parent, 0o500); const first = await tree.rollback().catch((cause: unknown) => cause); expect(first).toBeInstanceOf(ArtifactReaderFailure); expect((first as Error).cause).toMatchObject({ code: "EACCES", syscall: "rmdir" }); expect(await readdir(output)).toEqual([]); await chmod(parent, 0o700); const second = await tree.rollback().catch((cause: unknown) => cause); console.log(JSON.stringify({ case: "rollback", failedWrite, firstCauseCode: (first as any).cause?.code, fileRemovedByFirst: true, retryStatus: (second as any).status ?? null, retryCauseCode: (second as any).cause?.code ?? null, retryMessage: second instanceof Error ? second.message : null, rootRemains: await access(output).then(() => true, () => false), cleanup: tree.cleanup })); expect(second).toEqual({ status: "complete", residualPaths: [] }); } finally { await chmod(parent, 0o700); await tree.closeHandles(); await rm(parent, { recursive: true, force: true }); } }); ``` The finally block restores permissions, closes remaining handles and removes the fixture. Remove the temporary test afterward. ### Expected behavior After the obstruction is removed, the retained owner should continue from its confirmed cleanup progress and return `{ status: "complete", residualPaths: [] }`. Files already unlinked must not be re-admitted using closed descriptors. The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L119-L131) requires retrying unresolved children while retaining successful progress; the [cleanup regression guidance](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/testing.md#L251-L260) specifically requires restoring the denied resource and verifying the owning retry. ### Impact and scope P2: a transient removal failure becomes a persistent cleanup failure for a tree containing a failed write. In the reproduction only an empty operation-owned root remains; no user-data loss or security boundary crossing was observed. Manual removal of that known empty residual and abandoning the affected scope is possible, but retrying the same owner is broken. Source inspection shows ArtifactResourceScope retains output-tree owners and retries rollback before admitting new work, so this can keep the affected scope blocked after the original permissions are restored. The scope/public CLI/MCP behavior was not executed here. Runtime coverage is macOS with a size-mismatch write; cancellation, Linux, Windows and other obstruction timings were not tested. Natural incidence is unknown. ### REA version rea-agents 6.7.0; source revision `52415975942a4b9c53a817915b313ccc404f280e`. ### Runtime and platform macOS 27.0.1 arm64; Node.js v26.12.0 and repository-pinned v24.18.0; locked Vitest 5.0.3; non-root account and actual local filesystem permissions. ### Target and artifact context One synthetic regular file in an operation-created output tree. A size-mismatch write leaves its descriptor pending; the output parent is temporarily changed to mode 0500 and restored to 0700. ### Interface Other ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Executed observations on both Node versions: ```text Failed write: ArtifactReaderFailure, reason=integrity First rollback: cause.code=EACCES, cause.syscall=rmdir Output directory after first rollback: [] Parent permissions restored: 0700 Second rollback: ArtifactReaderFailure, reason=integrity Second rollback cause: code=EBADF, syscall=fstat, message="file closed" Second rollback message: "Extraction output cleanup could not be verified: file closed" Root still exists; cleanup={status:"incomplete", residualPaths:["output"]} Completed-write control: second rollback={status:"complete", residualPaths:[]}; root absent ``` This was an exported production filesystem boundary test. Source-derived public reachability: [artifact materialization](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/extraction/ArtifactExtraction.ts#L257-L295) creates and writes this tree, [failed extraction cleanup](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/extraction/ArtifactExtraction.ts#L357-L381) retains its output-tree owner, and [scope cleanup](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/ArtifactResourceScope.ts#L301-L303) calls that same tree’s rollback. ### Typed outcome or recovery expected Keep the first permission failure attributed to the output root. Once the obstruction is removed, resolve the same retained cleanup owner and report no residual paths. Preserve the initiating write-integrity diagnostic independently of cleanup. ### Analysis or suspected cause A failed write keeps its destination in [openFiles](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/SafeOutputTree.ts#L198-L205). [rollback](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/SafeOutputTree.ts#L370-L379) admits pending files and closes their handles before removal. [closeHandles](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/SafeOutputTree.ts#L572-L594) removes owners from openHandles but does not clear openFiles; only the successful-write closeFileHandle path clears that map. [removal](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/SafeOutputTreeCleanup.ts#L46-L67) deletes the ownedFiles identity immediately after unlink and can then fail at root rmdir. On retry, [pending-file admission](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/SafeOutputTree.ts#L596-L603) sees the stale openFiles record without an ownedFiles identity and calls readFileIdentity on the closed handle. This matches the observed fstat EBADF. ### Additional context Fetched upstream main `6d356b84344659085f2b912857b5e27fe0160c9b` and compared SafeOutputTree and SafeOutputTreeCleanup: unchanged from the executed baseline. The newer revision was not executed. #5898 concerns syncing nested directories before successful publication, not rollback state. #5956 concerns preserving export cancellation classification after rollback failure; its owner retry succeeds. #4483 and PR #4493 concern readback failure/cancellation precedence, #4928 concerns stale error markers after successful owner release, and #137 concerns mutable cleanup getters. PR #5672 changes source error lifetime and cleanup composition but does not repair openFiles admission after partial removal. No matching issue or PR was found. Last known good revision and natural failure frequency: NOT_ENOUGH_INFO.

bsakaguchi closed this issue

2026-10-31 00:43:00 +00:00

bsakaguchi commented

2026-10-31 00:43:01 +00:00

Author

Owner

@coygeek commented on GitHub (Oct 24, 2026):

Checked the reported fix on main at d87e73f2db.

Pending files are admitted once into identity tracking and removed from the pending set; successful unlink deletes retained file identity. Retry therefore no longer readmits an already closed handle or stats an already removed file. The regression performs a short failed write, denies root rmdir, restores permissions and requires the second rollback to remove the root; it also covers the completed-write control.

Implementation: SafeOutputTree.ts, SafeOutputTreeCleanup.ts.

The relevant regression checks in safeOutputTreePublication.test.ts passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

@coygeek commented on GitHub (Oct 24, 2026): Checked the reported fix on `main` at d87e73f2db50be72d33a4b3c7199af35518b441c. Pending files are admitted once into identity tracking and removed from the pending set; successful unlink deletes retained file identity. Retry therefore no longer readmits an already closed handle or stats an already removed file. The regression performs a short failed write, denies root rmdir, restores permissions and requires the second rollback to remove the root; it also covers the completed-write control. Implementation: [SafeOutputTree.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/artifacts/SafeOutputTree.ts#L672), [SafeOutputTreeCleanup.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/artifacts/SafeOutputTreeCleanup.ts#L63). The relevant regression checks in [safeOutputTreePublication.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/filesystem/safeOutputTreePublication.test.ts) passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

bsakaguchi referenced this issue

2026-11-01 21:10:58 +00:00

[GH-ISSUE #3796] [Tracker] Open feature requests by area (October 2026) #1471

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#3681

No description provided.

Rows
Columns