[PR #4185] [CLOSED] fix(verifiers): retry transient install-root cleanup on Windows (#4136) #6115

Closed
opened 2026-10-31 01:11:58 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4185
Author: @DeryFerd
Created: 10/15/2026
Status: ❌ Closed

Base: main ← Head: fix/4136-transient-install-root-cleanup


📝 Commits (1)

  • 5896459 fix(verifiers): retry transient install-root cleanup on Windows

📊 Changes

4 files changed (+176 additions, -2 deletions)

View changed files

📝 scripts/lib/verifier-cleanup.d.mts (+15 -0)
📝 scripts/lib/verifier-cleanup.mjs (+37 -0)
📝 scripts/verify-packaged-wasm-artifact.mjs (+16 -1)
📝 tests/boundary/process/verifierCleanup.test.ts (+108 -1)

📄 Description

Summary

The packaged-WASM verifier installs the npm artifact into a leased private runtime root and removes it in a bare finally. When a just-exited npm child still holds an installed dependency open without FILE_SHARE_DELETE, the native close fails with Win32 32 and the owned root stays on disk. The finally then replaced the verifier's own result with that cleanup failure and offered no retry, so a passing run could be reported as failed. The verifier now retries only the proven transient sharing/lock conflict with a bounded budget and aggregates any persistent cleanup failure with the primary result instead of masking it.

Fixes #4136.

Problem and expected behavior

The smallest trigger is an owned runtime root that is closed while a child process still holds a file inside it without delete sharing. In the affected lane, the npm install child had already exited and the owning Windows Job had settled at active-process count zero, yet PrivateRuntimeRoot.close() opened an installed dependency (.../node_modules/rea-agents/node_modules/@babel/parser/CHANGELOG.md) relative to its retained directory handle and received Win32 32. Windows can keep a file locked briefly after the holder exits, so a single immediate close is racy.

The violated invariant is that a verifier must not let teardown overwrite its own verification result, and that cleanup of an owned resource must recover from a conflict the owner can prove is transient. The expected behavior is that a proven transient conflict is retried until it clears, and that a genuinely persistent cleanup failure is reported alongside the primary result rather than instead of it.

Change and scope

scripts/lib/verifier-cleanup.mjs gains retryTransientCleanup, which retries a task only when it fails with ERR_REA_WINDOWS_NATIVE and Win32 32 or 33 (sharing or lock), the exact codes the Windows native authority already treats as a transient conflict. Every other failure, including permission errors, missing allocation evidence, and opaque text, propagates unchanged on the first attempt. The retry is bounded and then surfaces the last cause.

scripts/verify-packaged-wasm-artifact.mjs stops closing its root in a bare finally. It captures the primary error, retries the close through the new helper, and routes both through the existing closeVerifierResources aggregator, so a persistent cleanup failure no longer hides the verification outcome. The change is limited to verifier teardown. It does not weaken Windows share or delete semantics, does not add an automatic retry loop elsewhere, and does not touch the native authority or any provider.

Contract and boundary impact

  • Semantic owner and earliest changed stage: verifier cleanup in scripts/lib/verifier-cleanup.mjs; the earliest changed stage is packaged-WASM verifier teardown in scripts/verify-packaged-wasm-artifact.mjs.
  • Contribution scope fit and linked maintainer decision: ordinary defect fix within existing verifier and cleanup authority; a separate maintainer scope decision is not applicable.
  • CLI and MCP/tool-catalog contract: no product CLI, MCP, schema, or catalog change.
  • Provider, bridge, target-format, or platform compatibility: Windows-only recovery of an owned-root close; no provider, bridge, or target-format change, and the POSIX path is untouched because the retry predicate requires the Windows native error code.
  • Evidence, artifact, provenance, or reconstruction contract: no product Evidence change; the verifier keeps ownership of its leased root until removal succeeds.
  • Target selection, authority, upstream behavior, and cleanup details: no change to target selection or execution authority; the retry applies only to the verifier's own leased root and only to a proven transient conflict, with no native sharing or delete behavior relaxed.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

These are the existing effects of running the package verifier; this change adds no new effect and only bounds how its teardown recovers.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/process/verifierCleanup.test.ts gains four cases. One retries a proven sharing conflict until cleanup succeeds and asserts the exact attempt count. One asserts a non-transient failure is not retried. One asserts the last cause surfaces once the bounded budget is exhausted. One asserts closeVerifierResources retains the primary result when a retried cleanup still fails.
  • Base reproduction or other evidence: independent reproduction on a real Windows 11 x64 host (Node 24.21.0) with the packaged rea-agents@6.5.0 native addon. A child held a file in a leased root without delete sharing; raw PrivateRuntimeRoot.close() failed with ERR_REA_WINDOWS_NATIVE, Win32 32, and left the root on disk, while the same close succeeded after the holder released. Routing the close through retryTransientCleanup resolved and removed the root without a spurious failure.
  • User-visible CLI/MCP output (if applicable): not applicable; this is a verifier teardown boundary.
  • Remaining proof gaps: the complete packaged-WASM lane was not run end to end on this host, because it needs a Windows npm install and the WABT toolchain. The cleanup recovery itself was verified directly at the PrivateRuntimeRoot boundary. Local tsc/check:fast did not run because the host ran out of memory during the TypeScript check; the changed .mjs files are outside the tsconfig include set and were checked with oxlint, oxfmt, and the module-boundary verifier instead.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run tests/boundary/process/verifierCleanup.test.ts --project process-boundary — 6 passed, including the four new cases.
  • npx vitest run tests/boundary/process/verifierCleanup.test.ts tests/boundary/process/verifierRun.test.ts --project process-boundary — 7 passed.
  • Red-green check — reverting only scripts/lib/verifier-cleanup.mjs makes the four new cases fail with retryTransientCleanup is not a function; restoring the file makes all six pass.
  • npx oxlint scripts/lib/verifier-cleanup.mjs scripts/verify-packaged-wasm-artifact.mjs tests/boundary/process/verifierCleanup.test.ts — 0 warnings, 0 errors.
  • npx oxfmt --check on the three changed files — all files use the correct format.
  • npm run verify:module-boundaries — 1945 source files checked.
  • Real Windows x64 native cleanup boundary — raw close failed Win32 32 and left the root; the retried close resolved and removed it.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: real Windows x64 native runtime-root cleanup boundary exercised with the packaged addon; no provider behavior changed.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the retry is bounded and fires only for a proven transient sharing or lock conflict on the verifier's own leased root. It does not relax Windows sharing or delete semantics, does not drop ownership, and does not retry permission failures or opaque errors.

Author readiness

Human owner: @DeryFerd

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/4185 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/4136-transient-install-root-cleanup` --- ### 📝 Commits (1) - [`5896459`](https://github.com/morluto/rea/commit/5896459f94955f4521bc88bb30ed1a6c1033f941) fix(verifiers): retry transient install-root cleanup on Windows ### 📊 Changes **4 files changed** (+176 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `scripts/lib/verifier-cleanup.d.mts` (+15 -0) 📝 `scripts/lib/verifier-cleanup.mjs` (+37 -0) 📝 `scripts/verify-packaged-wasm-artifact.mjs` (+16 -1) 📝 `tests/boundary/process/verifierCleanup.test.ts` (+108 -1) </details> ### 📄 Description ## Summary The packaged-WASM verifier installs the npm artifact into a leased private runtime root and removes it in a bare `finally`. When a just-exited npm child still holds an installed dependency open without `FILE_SHARE_DELETE`, the native close fails with Win32 32 and the owned root stays on disk. The `finally` then replaced the verifier's own result with that cleanup failure and offered no retry, so a passing run could be reported as failed. The verifier now retries only the proven transient sharing/lock conflict with a bounded budget and aggregates any persistent cleanup failure with the primary result instead of masking it. Fixes #4136. ## Problem and expected behavior The smallest trigger is an owned runtime root that is closed while a child process still holds a file inside it without delete sharing. In the affected lane, the npm install child had already exited and the owning Windows Job had settled at active-process count zero, yet `PrivateRuntimeRoot.close()` opened an installed dependency (`.../node_modules/rea-agents/node_modules/@babel/parser/CHANGELOG.md`) relative to its retained directory handle and received Win32 32. Windows can keep a file locked briefly after the holder exits, so a single immediate close is racy. The violated invariant is that a verifier must not let teardown overwrite its own verification result, and that cleanup of an owned resource must recover from a conflict the owner can prove is transient. The expected behavior is that a proven transient conflict is retried until it clears, and that a genuinely persistent cleanup failure is reported alongside the primary result rather than instead of it. ## Change and scope `scripts/lib/verifier-cleanup.mjs` gains `retryTransientCleanup`, which retries a task only when it fails with `ERR_REA_WINDOWS_NATIVE` and Win32 32 or 33 (sharing or lock), the exact codes the Windows native authority already treats as a transient conflict. Every other failure, including permission errors, missing allocation evidence, and opaque text, propagates unchanged on the first attempt. The retry is bounded and then surfaces the last cause. `scripts/verify-packaged-wasm-artifact.mjs` stops closing its root in a bare `finally`. It captures the primary error, retries the close through the new helper, and routes both through the existing `closeVerifierResources` aggregator, so a persistent cleanup failure no longer hides the verification outcome. The change is limited to verifier teardown. It does not weaken Windows share or delete semantics, does not add an automatic retry loop elsewhere, and does not touch the native authority or any provider. ## Contract and boundary impact - Semantic owner and earliest changed stage: verifier cleanup in `scripts/lib/verifier-cleanup.mjs`; the earliest changed stage is packaged-WASM verifier teardown in `scripts/verify-packaged-wasm-artifact.mjs`. - Contribution scope fit and linked maintainer decision: ordinary defect fix within existing verifier and cleanup authority; a separate maintainer scope decision is not applicable. - CLI and MCP/tool-catalog contract: no product CLI, MCP, schema, or catalog change. - Provider, bridge, target-format, or platform compatibility: Windows-only recovery of an owned-root close; no provider, bridge, or target-format change, and the POSIX path is untouched because the retry predicate requires the Windows native error code. - Evidence, artifact, provenance, or reconstruction contract: no product Evidence change; the verifier keeps ownership of its leased root until removal succeeds. - Target selection, authority, upstream behavior, and cleanup details: no change to target selection or execution authority; the retry applies only to the verifier's own leased root and only to a proven transient conflict, with no native sharing or delete behavior relaxed. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [x] Launch or attach to a process. - [x] Inject instrumentation or run scripts/code. - [x] Change process memory, registers, files, or configuration. - [x] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. These are the existing effects of running the package verifier; this change adds no new effect and only bounds how its teardown recovers. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/process/verifierCleanup.test.ts` gains four cases. One retries a proven sharing conflict until cleanup succeeds and asserts the exact attempt count. One asserts a non-transient failure is not retried. One asserts the last cause surfaces once the bounded budget is exhausted. One asserts `closeVerifierResources` retains the primary result when a retried cleanup still fails. - Base reproduction or other evidence: independent reproduction on a real Windows 11 x64 host (Node 24.21.0) with the packaged `rea-agents@6.5.0` native addon. A child held a file in a leased root without delete sharing; raw `PrivateRuntimeRoot.close()` failed with `ERR_REA_WINDOWS_NATIVE`, Win32 32, and left the root on disk, while the same close succeeded after the holder released. Routing the close through `retryTransientCleanup` resolved and removed the root without a spurious failure. - User-visible CLI/MCP output (if applicable): not applicable; this is a verifier teardown boundary. - Remaining proof gaps: the complete packaged-WASM lane was not run end to end on this host, because it needs a Windows npm install and the WABT toolchain. The cleanup recovery itself was verified directly at the `PrivateRuntimeRoot` boundary. Local `tsc`/`check:fast` did not run because the host ran out of memory during the TypeScript check; the changed `.mjs` files are outside the `tsconfig` include set and were checked with oxlint, oxfmt, and the module-boundary verifier instead. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run tests/boundary/process/verifierCleanup.test.ts --project process-boundary` — 6 passed, including the four new cases. - `npx vitest run tests/boundary/process/verifierCleanup.test.ts tests/boundary/process/verifierRun.test.ts --project process-boundary` — 7 passed. - Red-green check — reverting only `scripts/lib/verifier-cleanup.mjs` makes the four new cases fail with `retryTransientCleanup is not a function`; restoring the file makes all six pass. - `npx oxlint scripts/lib/verifier-cleanup.mjs scripts/verify-packaged-wasm-artifact.mjs tests/boundary/process/verifierCleanup.test.ts` — 0 warnings, 0 errors. - `npx oxfmt --check` on the three changed files — all files use the correct format. - `npm run verify:module-boundaries` — 1945 source files checked. - Real Windows x64 native cleanup boundary — raw close failed Win32 32 and left the root; the retried close resolved and removed it. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: real Windows x64 native runtime-root cleanup boundary exercised with the packaged addon; no provider behavior changed. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the retry is bounded and fires only for a proven transient sharing or lock conflict on the verifier's own leased root. It does not relax Windows sharing or delete semantics, does not drop ownership, and does not retry permission failures or opaque errors. ## Author readiness Human owner: @DeryFerd - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:11:58 +00:00
Sign in to join this conversation.
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#6115
No description provided.