♻️ Give every generated eval module a scope-owned lifetime - #408
Merged
Conversation
The temp-file compiler wrote `.xmd-eval/<uuid>.ts` and then dropped its removal into a `finally` as a fire-and-forget promise, so the file outlived the compilation by an unbounded amount and a removal that failed was discarded unread. Each compilation now runs in a private scope, and the removal is registered against the UUID path before the write can begin. The file is gone before the compilation settles — before a block is returned, before a failing import reaches the caller, and before a cancelled compilation finishes halting — and a removal that fails for any reason other than the file already being absent leaves that scope. Filesystem work goes through `@effectionx/fs`, and the dynamic import through `until`, so no `node:fs` operation and no `call()`-wrapped promise remain.
PR #408: ♻️ Give every generated eval module a scope-owned lifetime3 files, +248 / -17 Scope✅ PR scope looks good. Structural✅ No structural bloat detected. Slop✅ Slop indicators look low. Static AnalysisOxlint: 2 diagnostics across 1 file (1 rule) no-unassigned-import (2): packages/core/src/temp-file-compiler.ts CorrectnessNo extraneous code patterns detected. |
The spec implied `.xmd-eval` follows the running document. It does not: `compileTempFile()` resolves the relative literal against the host process's working directory, which running `path/to/document.md` never moves and which the contextual `API.Env.cwd` does not control. The claim that a removal failure other than an already-absent file leaves the private scope had no test behind it. TC4 performs the real removal and then fails, and holds that exact error to be the compilation's outcome.
`node:path.resolve()` reads the process working directory when it is called, and `compileTempFile()` is publicly callable, so a host may have moved that directory since startup. Saying `.xmd-eval` follows the directory the process was started in claimed an immutability the compiler does not have.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #182.
Base:
35060af(origin/main, PR #406). The plan named0f11a5e(#405); mainadvanced one commit before this branch was cut, so it is based on
35060af.No
ci-main-redissue was open at push time.Why
packages/core/src/temp-file-compiler.tsis the portable eval-block compiler —the one Node's tsx loader accepts, and the one every runtime can load. It
generates a real file on disk and did not own it: removal was a fire-and-forget
unlink(tmpPath).catch(() => {})inside afinally. Nothing waited for theremoval, nothing reported its failure, and suspending cleanup inside
finallyis what
local/no-yield-in-finallyexists to prevent. The same file alsopredated Code Rules 2 and 3:
node:fs/promisesimports andcall()-wrappedpromises throughout.
What changes
Before:
.xmd-eval/<uuid>.tsoutlived its compilation by an unboundedamount. A cancelled compilation could leave the file behind entirely, because
the
finallynever ran the removal to completion.was swallowed by
.catch(() => {}).node:fs/promises; every promise went throughcall().After:
scoped()region. Removal is registeredwith
ensure()against the UUID path before the write can begin, so thefile is gone before the compilation settles, whichever way it settles:
before a compiled block is returned, before a failing import reaches the
caller, and before a cancelled compilation finishes halting.
becomes the compilation's outcome instead of being discarded.
{ force: true }is what makes "already absent" the one tolerated condition.
ensureDir,writeTextFileandrmcome from@effectionx/fs; the dynamicimport goes through
until(import(fileUrl)). Nonode:fsoperation and nocall()-wrapped promise remain.How it works
scoped()is what makes the ordering a fact rather than an intention: it closesits scope, running the registered destructor to completion, before it returns a
value, rethrows, or finishes halting.
Review guide
Start with:
packages/core/tests/temp-file-compiler.test.tsThen review:
compileTempFileinpackages/core/src/temp-file-compiler.ts— thescoped()boundary and whereensure()sits relative to the writespecs/executable-mdx-spec.mdand the Tier TC rowsLook carefully at:
ensure()registration point. Registering it one line later — after thewrite — is a mutation the halt test is there to kill.
recorder's safety cleanup, registered outside it, can still remove files a
failing run leaves behind.
What must stay true
scoped()boundary plus anensure()registered before the write, andchecked by TC1, TC2 and TC3.
rm(path, { force: true })insideensure()rather than a.catch(), and checked byTC4.
.xmd-eval, UUID.tsfilenames, standard plus user import construction,
file://loading, and thedefault-export validation and its diagnostic. Checked by the Tier T2 suite
(
packages/core/tests/eval-context.test.ts), which drives this compiler.How to verify it
@effectionx/fsis itself a contextual Api, soFsApi.around()observes thecompiler's real write and real removal as they happen. Each test asserts the
exact recorded order at the moment the compilation settles, which is what makes
"before it returned" falsifiable rather than eventually-true.
name the same
.xmd-eval/<uuid>.tsand that the removal completed beforecompileTempFile()returned. Fails if the removal is launched without beingawaited, or if the cleanup is registered on the caller's scope instead of a
private one.
still propagates, and proves the file was removed before that failure reached
the caller. Fails if teardown is skipped on the error path.
writeTextFile, delegates so the real file exists, signalsthe test, then suspends — handshake-driven, no timer. It halts the spawned
compilation and proves the file is gone once
halt()settles. Fails if thecleanup is registered after the write.
rmfor that compilation's generated path, performs thereal removal, then throws a unique sentinel. It proves
compileTempFile()fails with that exact
Error— by identity — rather than returning a block,swallowing the failure, or substituting an error of its own. Fails if the
cleanup error is caught or discarded.
Every test registers a force-removal safety cleanup for the paths it caused, so
a failed mutation cannot leave scratch files behind. None asserts that
.xmd-evalis globally empty — other tests compile into it concurrently.Mutation evidence
Each mutation was applied to the committed implementation by an edit that
asserts it matched exactly once, then reverted from a pristine copy taken with
git show HEAD:…and confirmed byte-identical afterwards(
deno task test packages/core/tests/temp-file-compiler.test.ts):ensure()cleanup removedscoped()boundary removedscope.run, unawaited)try/catchinsideensure)Each mutation is killed, and the last one is killed by TC4 alone — the row that
shows the new test carries its own weight.
.xmd-evalwas empty after the wholemutation run, which is the safety cleanup doing its job.
Commands
All run on
5c432f1;94f85b7adds only the specification wording correction above, re-verified withdeno task lint,deno task check, the focused lifecycle suites andgit diff --check:Scope
Included
compileTempFile()'s generated-file lifecycle, and its@effectionx/fsanduntilmigration.specs/executable-mdx-spec.md: the compiler section states theownership in present tense, and four conformance rows match the four tests
one-to-one.
Intentionally unchanged
EVAL_DIRstays the relative literal".xmd-eval". Bring temp-file compiler in line with the Effection and filesystem rules #182 notes thecwd-dependence and does not require changing it. The spec now says precisely
what that means: the path is resolved against the host process's current
working directory at the moment a compilation chooses it — read then, not
fixed at startup, since
compileTempFile()is publicly callable and a hostmay have moved that directory. Running
path/to/document.mddoes not itselfmove it to the document's directory, and the contextual
API.Env.cwddoesnot control it, because this compiler never consults that Api. No test
changes process-global cwd: doing so would make the parallel test corpus
unsafe.
access, so it does not route through
API.Files. A document never names thispath.
node:pathandnode:cryptoremain —@effectionx/fshas no equivalent.packages/cli/src/node.tsstill spells its providerreturn yield* compileTempFile(...). It type-checks unchanged and is outside this change.useDataUriCompiler()writes nothing and needed nothing here.Generated or mechanical changes
Risks and limitations
compileTempFile's declared return type narrows from a generator toOperation<EvalBlock>because it now returnsscoped(...). Everyyield*call site is unaffected — a
Generatoralready satisfiedOperation— anddeno task check,check:jsrand the Node typecheck all pass. Only a callerstepping the generator by hand, which nothing does, would notice.
silent. That is the point of the change, but it does convert a class of host
filesystem faults — a read-only mount, a revoked permission on
.xmd-eval—from invisible into a failed eval block.
or source map resolved later cannot read it back. That was already true: the
previous
finallyremoved it too, just at an unpredictable moment.Scope confirmation