Repository navigation
Resume-path file handle leak: Windows cleanup fails on open files across the whole resume/rehydrate test family #393
Description
Every graph-resume test on Windows passes its assertions and then fails only at cleanup: TempDir RemoveAll cleanup: unlinkat ...: The process cannot access the file because it is being used by another process. Something on the resume path holds an open file handle to test-tree content at test end. Unix tolerates unlinking open files, so this leak is invisible everywhere but Windows — which is precisely why it was never seen before the platform matrix (#373).
Evidence (PR #389 windows leg)
--- FAIL: TestGraphResumeThenFail_RollsBack_ViaPublicAPI/json
testing.go:1464: TempDir RemoveAll cleanup: unlinkat C:\...\001: being used by another process
Same signature on .../yaml, TestGraphResumePromiseFidelity_ViaPublicAPI, TestGitCloneResumeThenFail_RollsBack_ViaPublicAPI, and TestResourceLedgerRehydrate_PreservesIDs — the resume/rehydrate family exactly.
Investigation shape
The leak is in whatever the resume path opens and does not close: candidates are the persisted trace/journal being re-read at resume, the receipt document reader, or (for the git case) repository handles. Find it by instrumenting close on the resume read path or running one failing test under a handle tracer on Windows. The assertions all pass, so this is a resource-lifecycle defect, not a correctness one — but a long-lived writ process leaking a handle per resume is a real production concern, not just test noise.
Found in the post-#377 remainder triage; part of #373 phase 3e's path to a green windows leg.
Activity
David-Noble-at-work commentedon Aug 13, 2026
Scope widened by PR #398's leg: clearing the fixture-root panics unmasked the same
unlinkat … being used by another processcleanup signature across the plan package's graph tests —TestAssembleDefinition_MaterializesGraphFromInvocations,_TransfersCatalogOwnership,_OrphanInvocation_Errors,TestPlan_UnknownActionProvider,TestPlan_UnknownMethod— previously hidden behind a panic aborting the binary. The leak is not specific to the resume family: it is the graph-execution path generally (assemble + run holds something open through test end). That narrows the candidate set usefully — the common machinery between plain execution and resume is the executor's trace/receipt persistence, not resume-only re-reading.David-Noble-at-work commentedon Aug 13, 2026
Diagnosed. The leaked handle is the confined Root's directory handle:
fsroot.OpenConfinedwrapsos.OpenRoot, which holds the anchored directory open, and on Windows that blocksRemoveAllof exactly that directory — theunlinkat …\001in every failure is the temp dir the Root is anchored at.The happy path is correct:
GraphExecutor.Rundefersenvironment.Close(), andRuntimeEnvironment.Closecloses the Root (runtime_environment.go:298). The leaks are the paths that mint a Root but never reach a closing Run:plan.Provider.Spec(provider.go:475) and devlore-test'sbuildSpec(test_context.go:829) mint a confined Root into a spec. When assembly or plan validation fails before an executor runs — exactly whatTestAssembleDefinition_OrphanInvocation_Errors,TestPlan_UnknownActionProvider, andTestPlan_UnknownMethodexercise — nothing closes it. Error-path tests dominating the leak list is the tell.plan.Specdoc's 'so successive Run calls don't share a Root that closes when the first executor finishes' shows the seam was known.Fix shape needing a ruling (root lifetime ownership): the clean design moves Root ownership to the executor at construction —
NewGraphExecutortakes the spec and owns closing its Root at every terminal state (complete, fail, pause-then-abandon), with pre-Run failures closing on the error path; resume re-mints a fresh Root at the same anchor rather than reusing a closed one (theSpecdoc already gestures at this). The cheap alternative — close-on-error sprinkled at each mint site — fixes the tests but leaves the resume reuse smell standing.Windows-side verification is the only real proof either way; Unix cannot observe the leak.
David-Noble-at-work commentedon Aug 13, 2026
Ruling (2026-08-13): option 1 — the executor owns the Root from construction.
NewGraphExecutortakes ownership of the spec's Root and closes it at every terminal state (complete, fail, pause-then-abandon); pre-Run failures close on the error path; resume re-mints a fresh Root at the same anchor rather than rebuilding around a closed one. The close-on-error-per-site alternative was considered and rejected: it clears the test failures but leaves the resume family building environments around a closed Root — a latent correctness bug.Implementation deferred to a fresh session by decision; this issue carries the complete diagnosis, the fix design, and the verification note (Windows-side only — Unix cannot observe the leak, so CI's
test (windows-latest)leg is the proof). Expected effect when it lands: −18 of the standing 48.docs: record the honest Windows baseline and the banked #393 ruling (#…
David-Noble-at-work commentedon Aug 13, 2026
Ruling amended (2026-08-13): option 4 — specs carry no live Root; every RuntimeEnvironment mints its own.
Supersedes the option-1 ruling above. The deciding fact was verified across the tree: nothing between spec construction and
GraphExecutor.Run(orNewRuntimeEnvironment, for direct-environment hosts) dereferences the spec's Root. The field's only production readers areruntime_environment.go:200,206; every host gap between mint and hand-off is clean; no.starscript or doc example touchesspec.root; plan-timeConvertruns against the planning environment, never the run spec's Root.Design:
RuntimeEnvironmentSpeccarries the anchor path and access mode (the closed three-mode set matching the fsroot constructors: confined / read-only unconfined / writable unconfined) — serializable values, no handle.WithRoot(root)is replaced accordingly.NewRuntimeEnvironmentmints the Root from anchor + mode;RuntimeEnvironment.Closecloses it — the existing single-owner contract. Executors own environment lifecycle at every terminal state (unchanged), so Root lifetime rides the existing teardown. No ownership transfer exists anywhere; no per-site error closes remain.plan.Provider.Speckeeps its fail-fast contract by probing the anchor at spec time (open-and-release — the mint itself is the check, so no second definition of "valid" exists); the authoritative gate stays the mint at Run's preflight (PhasePreparing; failure landsReasonPreflightFailedbefore any dispatch). The spec type stays passive — spec-time checks are advisory under TOCTOU, so validation behavior does not move onto the data type.Expected effects: the pre-Run error-path leaks (the plan-package graph tests) become structurally impossible — nothing mints before environment build. Two additional defects found during verification heal or dissolve:
cmd/lore/lore/commands.go:259reuses one spec's Root across N executors in a loop, so iteration 2+ runs against a closed confined Root today (a live production instance of this bug); and devlore-test'srunner.go:281aliases the spec's root into TestContext for post-close reads, benign only because unconfined Close is a no-op.Verification unchanged: Windows-side only (
test (windows-latest)is the proof). Expected −18 of the standing 48.test(plan): close the runtime environments the plan tests mint
fix(op): mint each runtime environment's Root from the spec's anchor (#…
David-Noble-at-work commentedon Aug 13, 2026
Closed by PR #402 (merged to develop as
810745cf). The leak signature on the windows leg went 18 → 0; the burn-down moved 48 → 28, with zero panics and zero[build failed]packages, measured uncapped against head27d23e8e's check-runs.The option-4 design landed as ruled:
RuntimeEnvironmentSpeccarries only an anchor path and anfsroot.Mode(zero valueModeConfined) — never a live handle;NewRuntimeEnvironmentmints via the newfsroot.Open(dir, mode)andRuntimeEnvironment.Closereleases it; a mint failure atRunlands the preflight-failed terminal before any dispatch; resume re-mints from the anchor;plan.Provider.Speckeeps its fail-fast contract through an open-and-release probe. Six writ spec factories became infallible along the way.Correction to this issue's diagnosis, for the record. The framework change alone delivered −3, not −18. The remaining −17 came from a second commit closing fifteen test runtime environments that were never closed. The production leaks diagnosed here were real and are fixed — lore's deploy loop handed one spec to N executors, so every iteration after the first dispatched against a Root the previous run had closed (
cmd/lore/lore/commands.go);writ verifyandwrit readbacknever closed their loading environments; devlore-test aliased the spec's root intoTestContextfor post-close reads plus a double close. But they were not what the eighteen failing tests were failing on.The proof was a natural experiment inside the plan package:
lifecycle_e2e_test.go,lifecycle_starlark_test.go, andresult_flow_starlark_test.goeach close their environment and none of their tests appeared in the failure list;provider_test.go(2 mints),lifecycle_api_test.go(10),gather_api_test.go(1),git_resume_test.go(1), andreceipt_integration_test.go(1) closed nothing and all of their tests did. Fifteen unclosed mint sites, fifteen leak failures.The verification that preceded the option-4 ruling asked whether anything dereferences
spec.Rootbetween spec construction andRun— it does not, and that finding holds, which is what made option 4 the right choice over option 1. It never asked who closes the environment in the failing tests. A right number reached by wrong reasoning is still a miss.Details:
docs/plans/env-minted-root.md; campaign ledger updated indocs/plans/platform-test-matrix.md(#373).docs: record the 48 -> 28 burn-down and correct the #393 attribution (#…
feat(fsroot): reach *os.Root parity and add a self-removing scratch r…
docs: rule that filesystem access is received, never constructed (#409)