diff --git a/DESIGN.md b/DESIGN.md index c224dbc3a..f0a75c50f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -144,7 +144,13 @@ supplies — rather than a special case. The order of work follows from that: 1. **Share the skeleton.** Take the existing implementation as the core, with - its behaviour unchanged. + its behaviour unchanged — unchanged *as it stands when the port begins*. + When an idea lands first in the existing context (the idea-first order + below), the core the port takes already carries it, and the port copies + that. Such a port does acquire the new policy, and stays separate from it + all the same: the policy was argued, landed and proved in its own change, + in the context that could prove it, so the port's argument is only the + port. 2. **Adjust the parts** the new context genuinely requires, or extend the skeleton so it can express what the new context needs. 3. **Document every difference that remains,** at the part where it is made. @@ -173,11 +179,21 @@ the new context is often a decision made in the old one. Copy it first; if it turns out to be wrong, it is wrong in both places and worth an issue that says so. -**Keep the port separate from everything it inspires.** Land the sharing change -on its own, with behaviour unchanged. Anything new — a different scheduling -policy, a better measurement, an extra guard — is its own change afterwards. -Combined, they cannot be reviewed: an argument about the new idea becomes an -argument about the port. +**Keep the port separate from everything it inspires.** Anything new — a +different scheduling policy, a better measurement, an extra guard — is its own +change, never part of the port. Combined, they cannot be reviewed: an argument +about the new idea becomes an argument about the port. What the rule forbids is +the combination, not a fixed order. The common order is port first, behaviour +unchanged, because the port is usually what reveals the idea. When the idea is +the *premise* — decided before any port, and provable in the existing context +on its own — the same separation runs the other way: land the idea first, in +the context that can prove it, then the port, which then carries no idea of its +own beyond what the shared code already does. (An earlier version of this rule +said "with behaviour unchanged... afterwards", prescribing the order; the +sequential-runner plan in +[share-browser-console-runner](fjs/emergent_testing/todo/share-browser-console-runner.md) +is the case that showed the order is the consequence, not the rule — porting +first would have moved a context onto semantics about to change under it.) ### Exception to DRY: performance measurement diff --git a/fjs/effects/todo/all-argument-limit.md b/fjs/effects/todo/all-argument-limit.md new file mode 100644 index 000000000..b595e69ee --- /dev/null +++ b/fjs/effects/todo/all-argument-limit.md @@ -0,0 +1,120 @@ +## all-argument-limit. `all` cannot fan out more siblings than the engine allows arguments + +**Priority:** P3 +**Status:** open + +### Problem + +`All` is declared variadic — `readonly['all', (...effects: Effect[]) => …]` — so +every fan-out reaches it as a spread, and each one is a separate instance of the same +ceiling. Every site in the repository today: + +| site | what it fans out | +|-|-| +| `emergent_testing/module.f.mjs` `walkEntries` | one module's sibling leaves | +| `emergent_testing/module.f.mjs` `runModuleMap` | the modules of a run | +| `emergent_testing/module.f.mjs` `registerModule` ×2, `registerModuleMap` | the same two, for the framework-registration path | +| `dev/module.f.mjs` ×2 | files to load, and their imports | + +They fail independently: a suite of a hundred thousand *modules* breaks the outer spread +however few leaves each holds, and one module of a hundred thousand leaves breaks the inner +one however few modules there are. A fix has to be the operation's, not a site's. + +A spread is a call, and a call has an argument limit. Measured on node 22: + +| siblings | result | +|-|-| +| 50,000 | ok | +| 100,000 | `RangeError: Maximum call stack size exceeded` | + +The throw is in **building** the effect, before any interpreter sees it, so no runner can +recover from it and no `catch` operation is in the path. Today only `fjs t` is on this +path, and it panics. (The reverted functionalscript#1759 briefly put the browser page on +it too, where the page's run-failure guard reported one `infrastructure-error` — the guard +working as intended, but not an answer; the current page takes the `Promise.all` path +below and never builds the effect.) + +The ceiling applies **per fan-out**, and a run has two: one module with too many sibling +leaves breaks the inner spread, and a run with too many *modules* breaks the outer one in +`runModuleMap` — the independence the table above states. Nothing in this repository is +close to either — the browser suite is 3,461 leaves across 138 modules, three orders of +magnitude under both — so this is a real ceiling rather than a live problem, and it is +recorded rather than fixed for that reason. + +The browser runner is immune for a reason that has nothing to do with its batching: +`Promise.all(batch.map(…))` passes one iterable argument, so no spread exists there at any +batch size — the ceiling is the *variadic operation's*, not fan-out's in general. (An +earlier version of this paragraph credited `batchSize = 25` with staying under the limit; +that was a misattribution, corrected in the pitfall catalog in +[share-browser-console-runner](../../emergent_testing/todo/share-browser-console-runner.md).) +The reverted functionalscript#1759 routed the page through the shared traversal and so +briefly gave both runners the same ceiling; the sequential plan that replaced it removes +the traversal's fan-outs entirely, which retires the `walkEntries` and `runModuleMap` rows +above. What remains then is the registration path and `dev` — still the operation's +problem, at fewer sites. + +### Proposal + +Make `all` take a list rather than an argument list: + +```ts +export type All = readonly['all', (effects: readonly Effect[]) => OpResult[]>] +``` + +Then `allOk(entries.map(one))` builds an array and hands it over, and no call in the path +grows with the suite. Every `all` handler changes shape — `effects/node`'s real and +virtual runners, the mock, and any fixture that supplies one — which is what makes this +its own step rather than a fix inside another change. Not a browser interpreter: under +the sequential plan the traversal performs no `all`, so no browser implements it. + +The variadic spelling is nicer at the two-or-three-effect call sites that motivated it +(`both`, hand-written fan-outs in proofs), so a wrapper that keeps that shape over the +list-shaped operation is worth having in the same change. **Both callables get +unambiguous names, whichever branch is taken**: if the wrapper is kept it keeps +the published `all`/`allOk` names (that is what narrows the break, per the task +below) and the list-shaped operation is exported beside it under its own names +(say `allList`/`allOkList`); if the wrapper is dropped, the list shape takes +the old names. Every arbitrary-length fan-out — the traversal sites in the +table, and combinators born after this issue +([allvoid-combinator](./allvoid-combinator.md), +[allreduce-combinator](./allreduce-combinator.md)) — calls the *list-shaped* +callable by whichever name this decision lands on, so those designs are +buildable under every permitted outcome. + +### Alternatives considered + +- **Chunk the traversal.** Fan out in groups below the limit. This puts a constant back + into the shared walk, which is the mistake + [share-browser-console-runner](../../emergent_testing/todo/share-browser-console-runner.md) + spends several pages on, and it changes the concurrency of every run to work around an + argument-passing detail. No. +- **Leave it.** Defensible today, and what this issue does for now. It stops being + defensible the first time a generated suite puts tens of thousands of leaves in one + module. + +### Tasks + +- [ ] Decide the list-shaped `All` signature and whether a variadic wrapper stays. + **Either way this is breaking, and the entry must say so.** Changing the + *operation* breaks every `all` handler however it is spelled at call + sites; dropping the wrapper additionally changes the published + `all`/`allOk` call shape, which reaches every fixed-arity caller + (`both`, hand-written fan-outs in proofs) and any external importer — + so the PR migrates every in-repo caller in the same change and carries + a `**BREAKING CHANGES:**` changelog entry naming what moved. Keeping + the wrapper narrows the break to the handlers, which is the argument + for keeping it. +- [ ] Move every interpreter and fixture to it in one change, and every spread site in the + table above with them. Future combinators scheduled after this issue are + consumers too, born list-shaped rather than migrated: + [allvoid-combinator](./allvoid-combinator.md) and + [allreduce-combinator](./allreduce-combinator.md) both say so in their + proposals — an arbitrary-length fan-out combinator with a spread in its + body would rebuild this ceiling inside itself. +- [ ] Prove a fan-out above the current ceiling — the number itself is engine-specific, so + the proof asserts that a large fan-out completes rather than asserting the ceiling. + +### Related + +- [share-browser-console-runner](../../emergent_testing/todo/share-browser-console-runner.md) + — where the browser's accidental protection was removed, and why. diff --git a/fjs/effects/todo/allreduce-combinator.md b/fjs/effects/todo/allreduce-combinator.md index 91b11960b..dbb297bba 100644 --- a/fjs/effects/todo/allreduce-combinator.md +++ b/fjs/effects/todo/allreduce-combinator.md @@ -15,34 +15,49 @@ The pattern `step(all(...xs.map(f)), rs => pure(rs.reduce(op, init)))` — fan o ```ts export const allReduce = - ( - f: (item: T) => Effect, + ( + f: (item: T) => Effect, ) => (op: (a: R) => (b: R) => R) => (init: R) => - (items: List): Effect => - step( - all(...toArray(items).map(f)), - rs => pure(rs.reduce((a, b) => op(b)(a), init))) + (items: List): Effect => + mapStep( + allOk(toArray(items).map(f)), + rs => rs.reduce((a, b) => op(b)(a), init)) ``` -Note the standalone `step`: `all(...)` returns a raw `Effect`, which is plain -data with no methods, so `all(...).step(...)` — as an earlier draft of this -issue wrote it — would not compile. If -[map-step-combinator](./map-step-combinator.md) lands first, the body is -`mapStep(all(...toArray(items).map(f)), rs => rs.reduce(...))`. +**Built on `allOk`, not on raw `all`, and the error channel is a parameter.** +`all`'s continuation receives `readonly Result[]` — the children's +failures arrive *inside* the value — so a monoid folding those elements as +`R` either does not type-check or aggregates failure tuples as data. An +earlier sketch of this issue did exactly that. `allOk` collapses the list to +`readonly R[]` and lifts the first failure into the effect's error channel, +which is how every named consumer below already behaves at its existing +`allOk` call sites; `NotImplemented` is the runner's, inherited from `allOk`, +and `E` is the children's. -`op` must be **commutative** — results may arrive in any order when the runner schedules sub-effects in parallel. +**The body hands `allOk` the list, not a spread**, per +[all-argument-limit](./all-argument-limit.md)'s naming rule (`allOk` above +names the list-shaped callable — `allOk` itself if the variadic wrapper is +dropped, the list-shaped sibling if it is kept): a combinator built for +arbitrarily long lists must not become another instance of the ceiling that +issue removes. This issue therefore lands after all-argument-limit; until +then only the variadic spelling compiles. -After adding `allReduce`, `runModuleMap` in `fjs/emergent_testing/module.f.mjs` simplifies to: +Note the standalone `mapStep`: `allOk(…)` returns a raw `Effect`, which is +plain data with no methods, so `allOk(…).step(…)` — as an earlier draft of +this issue wrote it — would not compile. -```ts -return allReduce - (([k, v]: Entry) => runModule(reporter)(k, v)(zero)) - (mergeState) - (zero) - (modules) -``` +`op` must be **commutative** — results may arrive in any order when the runner schedules sub-effects in parallel. + +**`runModuleMap` is no longer a consumer.** An earlier draft of this issue +rewrote it with `allReduce`, and the sequential plan in +[share-browser-console-runner](../../emergent_testing/todo/share-browser-console-runner.md) +decides the opposite: the proof traversal runs one leaf's whole chain before +the next, deliberately, and fanning its modules back out would undo that +decision. The combinator's consumers are the sites that *want* fan-out — the +framework-registration path and `dev/module.f.mjs`'s file loading — and it +must not be applied to the traversal. ### Naming diff --git a/fjs/effects/todo/allvoid-combinator.md b/fjs/effects/todo/allvoid-combinator.md index 506ba5dae..2b7f859d1 100644 --- a/fjs/effects/todo/allvoid-combinator.md +++ b/fjs/effects/todo/allvoid-combinator.md @@ -58,12 +58,26 @@ sites already spell it that way. export const allVoid = (f: (item: T) => Effect) => (items: readonly T[]): Effect => - mapStep(allOk(...items.map(f)), () => undefined) + mapStep(allOk(items.map(f)), () => undefined) ``` +The body hands the *list-shaped* callable the list, not a spread: `allVoid` +exists for arbitrary-length fan-outs, which is exactly where +`allOk(...items.map(f))` would rebuild the engine argument ceiling +([all-argument-limit](./all-argument-limit.md)) inside the new combinator — +the same correction [allreduce-combinator](./allreduce-combinator.md) +carries. `allOk` in the sketch names that list-shaped operation under +all-argument-limit's naming rule: it is `allOk` itself if the variadic +wrapper is dropped, and the list-shaped sibling (`allOkList` in that issue's +sketch) if the wrapper keeps the published names — either way the body's +call shape is one array argument. That makes this issue's landing depend on +the list-shaped operation from that issue; until it lands, the spread +spelling is the only one that compiles, which is one more reason this issue +is scheduled after the `All` move rather than before it. + `NotImplemented` in the error channel is the runner's, inherited from `allOk`; `E` is the children's. Written with the standalone `step` instead — -`step(allOk(...items.map(f)), () => pureOk(undefined))` — it is the same effect +`step(allOk(items.map(f)), () => pureOk(undefined))` — it is the same effect said less directly; either works. Note `pureOk`, not `pure`: `pure` takes a `Result` (`pureOk = v => pure(ok(v))`), so `pure(undefined)` would yield a bare `undefined` where the chain expects `ok(undefined)`. Both spellings must also @@ -87,10 +101,13 @@ no host API in it. The three call sites become `allVoid(e => registerOne(t, e))(sub)` etc. If [allreduce-combinator](./allreduce-combinator.md) lands first, consider deriving `allVoid` from `allReduce` with a unit monoid instead of -duplicating the `allOk(...map)` core — but only once `allReduce` is itself -built on `allOk`. As proposed it folds over `all(...)`, so its monoid receives -the children's `Result`s as ordinary values, and a unit monoid over those -would discard precisely the failures this section exists to keep. +duplicating the shared core — its proposal is now built on the list-shaped +`allOk`, so its monoid receives plain `R`s and the first failure travels the +error channel, which is exactly what a unit monoid needs. (An earlier sketch +of that issue folded over raw `all(...)`, whose monoid would have received +the children's `Result`s as ordinary values — a unit monoid over those would +discard precisely the failures this section exists to keep; that sketch is +recorded as superseded there.) ### Tasks @@ -98,6 +115,10 @@ would discard precisely the failures this section exists to keep. `All`/`all`/`both` **and `allOk`** to `fjs/effects/all/module.f.mjs`. `allVoid` is built on `allOk`, so moving one without the other inverts the layering. +- [ ] Wait for [all-argument-limit](./all-argument-limit.md)'s list-shaped + `allOk`, and hand it the list: `allVoid` is an arbitrary-length + fan-out, so a spread in its body would rebuild the argument ceiling it + is called at (the note under the proposal). - [ ] Add `allVoid` there (next to `all`/`both`) with proof coverage — **not** to `fjs/effects/node/module.f.mjs`, per the note at the top of this issue. - [ ] Convert the three `mapStep(allOk(...), () => undefined)` call sites in diff --git a/fjs/effects/todo/node-module-layering.md b/fjs/effects/todo/node-module-layering.md index 81acbb6f2..389b2d3ae 100644 --- a/fjs/effects/todo/node-module-layering.md +++ b/fjs/effects/todo/node-module-layering.md @@ -51,11 +51,11 @@ provides*. Proposed destinations: | Moves to | Contents | |---|---| | `fjs/effects/all/module.f.mjs` | `All`, `all`, `allOk`, `both`, and `allVoid`/`allReduce` when they land | -| `fjs/effects/sandbox/module.f.mjs` | `Sandbox`, `SandboxResult`, `sandbox`, `Await`, `awaitIfPromise` — the "run foreign code and observe what happened" pair | +| `fjs/effects/sandbox/module.f.mjs` | `Sandbox`, `SandboxResult`, `sandbox`, `Await`, `awaitIfPromise`, and `Catch`/`catch_` (landed after this table was written) — the "run foreign code and observe what happened" family. This row is what [share-browser-console-runner](../../emergent_testing/todo/share-browser-console-runner.md) step 4's "shared module" resolves to: a browser gives `Sandbox` and `Catch` their second implementer; `Await` moves on this issue's layering argument alone, since it belongs to the registration path no browser runs | | `fjs/effects/console/module.f.mjs` | `Read`, `Write`, `ReadConsoles`, `WriteConsoles`, `Console`, `log`, `error`, `readLine`, `errorExit`, and a **new named `Std`** (see below) | | `fjs/effects/test/module.f.mjs` | `Test`, `TestFn`, `TestContext`, `test` — registration with an external framework, not I/O | | stays in `fjs/effects/node` | `Fs` and its members, `Http`, `Forever`, `RandomInt`, `isNotFound`, `Env`, `Engine`, `NodeOp`, `NodeProgramOptions`, `Program`, `NodeProgram`, `NodeOperationMap` | -| unsettled | `Now`, `Fetch`, `Import` — this issue and share-browser-console-runner's step 4 disagree; step 5 decides (see the judgement call below) | +| stays, now settled | `Now`, `Fetch`, `Import` — the browser interpreter implements none of them, so none has a second implementer (see the judgement call below) | | already moved to `fjs/effects` | `OpResult`, `IoChannel`, `IoError`, `IoErrorInfo`, `IoResult`, `ioError`, `toIoError` — the vocabulary every operation is declared in; `effects/node` re-exports them (see the judgement call below) | `NodeOp` stays where it is and keeps unioning every family — it is the @@ -69,23 +69,37 @@ Judgement calls worth deciding explicitly rather than by accident: - **`RandomInt` stays.** An ambient host capability with no cross-runtime abstraction to gain and no consumer outside `fjs/cas` and the interpreters. Moving it would be motion without a reader benefit. -- **`Now`, `Fetch` and `Import` are unsettled, and step 5 of +- **`Now`, `Fetch` and `Import` stay, and this was settled by building the + browser interpreter rather than by arguing.** This issue and [share-browser-console-runner](../../emergent_testing/todo/share-browser-console-runner.md) - decides them.** This issue put all three in the "stays" row on the reader-benefit - argument above; that issue's step 4 lists `now`, `fetch` and `import` among the - operations to move. Both were written without the fact that settles it — **which - operations a browser interpreter actually implements** — so neither ruling is - authoritative and the disagreement is recorded here rather than resolved by - whichever file a later reader opens first. - - The test to apply is the one `isNotFound` failed: not "does a browser also - have one of these", but "is this operation about no host in particular". By - that test `now` and `import` look likely to move — a browser proof run needs a - clock and dynamic import, so step 5 gives them a second implementer — and - `fetch` looks likely to stay, since nothing in the shared runner performs one - and DESIGN.md §4 extracts at the second *real* consumer, not the second - possible one. Those are expectations, not rulings: whichever way step 5 goes, - it updates both files in the same change. + step 4 disagreed: this file put all three in "stays" on the reader-benefit + argument, that one listed them among the operations to move. Neither was + written knowing the fact that decides it — which operations a browser + interpreter actually implements — so both recorded the disagreement and left + it to step 5. + + Step 5's answer was `sandbox`, `catch` and `all`, and nothing else: the + browser interpreter built (and later reverted, with its record) in + functionalscript#1759 implemented those three because the shared proof + traversal performed those three. A page loads its modules through its own + importer rather than an `import` operation, measures its own wall clock + rather than dispatching `now`, and performs no `fetch` at all. So none of + the three gained a second implementer, and DESIGN.md §4 keeps them here + until one does. The sequential plan that replaced that attempt (see + share-browser-console-runner) shrinks the measured set once more: a + sequential traversal performs no `all`, so the operations a browser gives a + second implementer are `sandbox` and `catch` alone. That takes `all` out of + *step 4's* motivation, not out of this issue's: its move to `effects/all` + above rests on the layering argument, and its implementers stay the Node + runners and the registration path. + + Worth recording, because the earlier expectation written here was wrong about + two of them: "a browser proof run needs a clock and dynamic import" is true of + the *page* and false of the *effect set* — the page does both directly, in the + impure shell where host values belong, which is exactly the boundary this + whole exercise is drawing. Reasoning from what a host can do predicted the + wrong answer; reading what the interpreter had to implement gave the right + one. - **`isNotFound` stays, and this was tested.** It encodes `ENOENT` specifically — a POSIX filesystem code that a host without a filesystem never reports — so it *is* a Node-layer concern. A change that moved it to the core @@ -200,15 +214,43 @@ Judgement calls worth deciding explicitly rather than by accident: concern per PR, update every importer in the same PR, and prefix the CHANGELOG entry with `**BREAKING CHANGES:**`. Do not leave re-export shims behind. - **The vocabulary move is the one exception, and for a reason that does not - generalize.** A re-export is a shim when it keeps a *dead* coupling alive — - which is the case for every move in the table above, where the whole goal is - that `fjs/text/sgr` stops naming `effects/node` at all. It is not the case - for `IoChannel` and its siblings: node's own operations are declared in - them, so `effects/node` re-exporting what it genuinely uses keeps one - vocabulary readable at one import rather than preserving a coupling anyone - wants gone. That is why that move was additive and needed no importer churn, - and why the moves below still need theirs. + **The exception is decided by a test, not by a list.** A re-export is a shim + when it keeps a *dead* coupling alive; it is legitimate where the + re-exporting module genuinely uses the names. The vocabulary move passed + that test — node's own operations are declared in `IoChannel` and its + siblings — and so does the whole sandbox row, `Await` included: `NodeOp` is + declared over `Sandbox`, `Catch` and `Await`, and both node runners + implement all three, so `effects/node` re-exporting them keeps one + operation set readable at one import for node-side callers, while the + modules the move exists for (the shared traversal, a browser interpreter) + import the new home directly. The sandbox row's move is therefore + additive — and so is the `all` row's, by the same test applied honestly: + `NodeOp` unions `All` and both node runners implement it, so `effects/node` + re-exporting it is the same one-import convenience, not a dead coupling. + The console and test rows *split* under the same test rather than failing + it wholesale, because the test applies per name, not per concern: the + surviving `effects/node` declarations still reference the operation + types — `NodeOp` unions `Read`, `Write` and `Test`, and + `NodeProgramOptions` names `WriteConsoles` and `TestContext` — so those + names stay re-exported by the same argument as `Sandbox` and `All`. The + test reaches the helpers one name at a time, the surviving *code* counts + as much as the declarations, and it is applied to the module **as it + stands after the move**: `exitStep` stays — it is the node program's + exit-code policy, consumed repo-wide — and it calls `errorExit`, so + `errorExit` stays re-exported. `errorExit`'s own call to `error` moves to + the console module with its body, and nothing that remains in + `effects/node` references `error` after that — so `error` is *not* kept + by `errorExit`'s keeping, and joins `log`, `readLine` and the `test` + combinator as the dead couplings: their consumers are exactly the ones + the moves exist to decouple, so they move as hard cutovers, every + importer updated in the same PR, no re-export left behind. Draw the exact + split at move time by this test — grep what the post-move `effects/node` + declarations *and function bodies* reference — and note + that the decoupling each move exists for is enforced by its own step's + check (`fjs/text/sgr` no longer importing `effects/node`), which a type + re-export for node-side callers does not weaken. + [share-browser-console-runner](../../emergent_testing/todo/share-browser-console-runner.md) + step 4 states the same policy from its side. - **The obsolete Playwright adapter is already gone.** This task must preserve only the process-side `TestContext` fields that still have consumers. It must not use relocation as a reason to revive the Playwright engine, context, @@ -235,7 +277,8 @@ Judgement calls worth deciding explicitly rather than by accident: `allOk` is the ok-channel wrapper over `all` and belongs with it; [allvoid-combinator](./allvoid-combinator.md) builds on it, so leaving it behind would make `effects/all` import from `effects/node`. -- [ ] Move `Sandbox` / `Await` and helpers to `fjs/effects/sandbox/module.f.mjs`. +- [ ] Move `Sandbox` / `Await` / `Catch` and helpers to + `fjs/effects/sandbox/module.f.mjs`. - [ ] Move the console family to `fjs/effects/console/module.f.mjs`, add the named `Std` type there as `RequiredMap`, point `NodeProgramOptions.std` at it, and narrow `csiWrite` to take `Std` diff --git a/fjs/emergent_testing/todo/65z-tf-test-tree-walker.md b/fjs/emergent_testing/todo/65z-tf-test-tree-walker.md index 5c884559a..d843853a8 100644 --- a/fjs/emergent_testing/todo/65z-tf-test-tree-walker.md +++ b/fjs/emergent_testing/todo/65z-tf-test-tree-walker.md @@ -55,8 +55,9 @@ Both implementations: `registerModule` is a process-adapter path for the surviving external frameworks. It cannot always reuse `runModule`'s `Reporter` because of the external-framework constraint discussed in the module doc (lines 144-153). But the *traversal* (collect -leaves, recurse into function-return sub-trees, fan out with `all`) is shared and decouples -cleanly from the per-leaf action. +leaves, recurse into function-return sub-trees, combine the siblings — today both do that +with `all`, though the sequential plan changes `runModule`'s side; see the note under the +sketch) is shared and decouples cleanly from the per-leaf action. The removed Node-side Playwright integration is not a consumer of this design. A future Playwright Test adapter opens the shared browser application and consumes its report; it @@ -91,6 +92,22 @@ export const walkTests = (w: Walker) => { } ``` +**The sketch above predates the sequential plan and hard-codes the one thing +the two consumers no longer agree on.** The sequential plan in +[share-browser-console-runner](share-browser-console-runner.md) makes +`runModule`'s traversal sequential — one leaf's whole chain finishes before +the next starts — while `registerModule` keeps its `all` fan-out (its +recursion drives an external framework's own scheduling, and it is a site in +[all-argument-limit](../../effects/todo/all-argument-limit.md) either way). +So `all(...collectTests(...).map(...))` cannot live inside a shared walker: +scheduling is the *instantiation's* contract, not the walker's. A `walkTests` +that survives this takes the sibling combination as a parameter alongside +`merge` — a sequential fold for the run path, a fan-out for the registration +path — or it does not qualify. Any spike happens after the sequential +traversal lands, against the code as it then is; a walker that quietly +restores concurrency to `runModule`, or quietly serializes `registerModule`, +has broken a scheduling contract this repository has already paid to settle. + `runModule` instantiates `S = RunTotals`, threads `Sandbox`/`Reporter` effects in `onLeaf`, and returns the sub-tree value on success-without-`throws`. @@ -105,8 +122,9 @@ inside the page. Playwright itself remains outside that walker and only controls The exact `Walker` shape is open — it may be cleaner to split "should we recurse?" from "give me the sub-tree value" so the abstraction doesn't force a -boolean discriminator. The point is the recursion shape (collect → fan-out → -merge) lives in one place for the process-side implementations, while the browser runner +boolean discriminator. The point is the recursion shape (collect → visit each +sibling, under the instantiation's scheduling → merge) lives in one place for +the process-side implementations, while the browser runner shares the semantics rather than the obsolete Playwright registration path. ### Why this qualifies @@ -114,10 +132,11 @@ shares the semantics rather than the obsolete Playwright registration path. - **DRY at the right altitude.** `collectTests` already names the static walk; this names the dynamic one. Two process-side consumers exist today, and another process adapter, JSON reporter, or coverage instrumenter would otherwise copy it. -- **Separation of concerns.** The recursion structure (fan-out, merge, when - to stop) is one concern; the per-leaf action (sandbox+reporter vs. - framework registration) is another. Today they're entangled inside two - near-identical functions. +- **Separation of concerns.** The recursion structure (visit siblings, merge, + when to stop) is one concern; the per-leaf action (sandbox+reporter vs. + framework registration) is another — and the sibling *scheduling* belongs + to neither: it is each instantiation's contract, per the note under the + sketch. Today all three are entangled inside two near-identical functions. - **Documents the contract.** The "function-return sub-tree is walked the same way as the static export tree, with `throws` reset to `false` and a `null` marker appended to the path" rule is currently a comment in @@ -148,11 +167,16 @@ shares the semantics rather than the obsolete Playwright registration path. ### Tasks -- [ ] Spike a `walkTests` shape against the existing `runModule` and surviving - process-adapter `registerModule` implementations. +- [ ] Spike a `walkTests` shape against `runModule` and the surviving + process-adapter `registerModule` — after the sequential traversal from + [share-browser-console-runner](share-browser-console-runner.md) lands, + with the sibling combination as a parameter, per the note under the + sketch. - [ ] Keep Playwright out of `TestContext`, `registerModule`, and the process-side walker. - [ ] Define runner-independent fixtures for recursive return-value subtrees, `throws` - reset, path construction, and sibling fan-out. + reset, path construction, and sibling scheduling — proving the run path + sequential and the registration path fanned out, since the walker takes + the combination as a parameter. - [ ] Run those fixtures against both the process walker and the shared browser runner. - [ ] Keep the browser runner free of Node and Playwright imports. - [ ] Land the abstraction only when the existing process-side implementations become @@ -160,6 +184,9 @@ shares the semantics rather than the obsolete Playwright registration path. ### Related +- [Share the browser and console proof runners](share-browser-console-runner.md) + — the sequential plan that settled `runModule`'s scheduling, which this + issue's walker must take as a parameter rather than decide. - i183 — broader work on the `tf` framework; this is a structural cleanup that lands cleanly alongside it. - [i157](../../djs/todo/157-json-djs-shared-value-machine.md) — same flavour: two parallel diff --git a/fjs/emergent_testing/todo/browser-test-controls.md b/fjs/emergent_testing/todo/browser-test-controls.md index 77c391782..7fb648504 100644 --- a/fjs/emergent_testing/todo/browser-test-controls.md +++ b/fjs/emergent_testing/todo/browser-test-controls.md @@ -36,8 +36,15 @@ Cancellation must be semantic, not merely visual. It should prevent unstarted proofs from running, ignore late module imports and proof completions from the cancelled run, and prevent that run from replacing a later run's progress, report, promise, or completion event. Work already executing in JavaScript -cannot always be interrupted; cancellation should be cooperative at module, -batch, and proof boundaries and document that limitation. +cannot always be interrupted; cancellation should be cooperative at module +and leaf boundaries and document that limitation. (When this was filed the +page ran batches, and the batch boundary was a natural check point; the +sequential plan in [share-browser-console-runner](share-browser-console-runner.md) +removes batching, so the check point is before each leaf invocation — the +next *sibling* and each returned *child* alike, since a cancel that lands +during a parent's awaited report must keep its unstarted children unstarted, +per the requirement above. The un-interruptible unit is one leaf's own test +and report, which is finer-grained than the batch was.) The final cancelled result needs a serializable status distinct from `failed` and `infrastructure-error`. Decide whether cancellation dispatches the existing @@ -55,8 +62,9 @@ module or a default query parameter. - [ ] Add a `Cancel` button and implement the inverse enabled/disabled states for `Run` and `Cancel`. - [ ] Add a per-run cancellation token or equivalent identity checked during - loading, between execution batches, and before every UI/global/event - publication. + loading, before each leaf invocation — sibling and returned child alike + (the between-batches check this task once named — gone with batching, + per the note above), and before every UI/global/event publication. - [ ] Define the serializable cancelled report and completion-event behavior. - [x] Prove initial idle behavior and `Run`'s state transitions across loading, running, and both terminal outcomes; cancellation-related diff --git a/fjs/emergent_testing/todo/report-before-running.md b/fjs/emergent_testing/todo/report-before-running.md index 1a0be4351..087f2b41e 100644 --- a/fjs/emergent_testing/todo/report-before-running.md +++ b/fjs/emergent_testing/todo/report-before-running.md @@ -35,29 +35,54 @@ start event is adding a third event kind, not building the stream first. Add a `start` (or `begin`) event to the reporter, called with the file and path before the leaf is sandboxed, and let each host decide what to do with it: -- **`fjs t`** prints the name, then completes the line with `ok`/`error` and the - duration when the result lands — the standard runner shape, in the format it - already prints. Interleaving is the thing to get right: leaves - run concurrently, so a half-written line cannot be left open across another - test's output. Either the name and its outcome are one deferred line with the - name shown live elsewhere, or output is a two-column log that names the start - and closes it by identifier. -- **The browser page** renders a row in a pending state and settles it in place, - which is the same list it renders now with one more state per row. +- **`fjs t`** prints a complete, newline-terminated start record, then a + separate result line that names the test again. Not an open line completed + in place: no *other leaf* runs between a start and its own result under the + sequential runner, but the leaf itself does, and anything it writes to the + terminal — a proof that logs at runtime (purity is a convention the sandbox + does not enforce; see [hostile-proof-values](hostile-proof-values.md)), a + Node warning on stderr — would splice into an open line and attach the + later `ok`/`error` to unrelated output, corrupting the log for readers and + line-oriented consumers alike. Two self-contained lines per leaf survive + that; in the common case the result still directly follows its own start, + and the repeated name is what keeps the pair legible when something + intervenes. +- **The browser page** renders a row in a pending state and settles it in + place — the same list it renders now with one more state per row — **and + its start handler awaits one macrotask after rendering the pending row**, + exactly as its `report` handler does after a result. Appending a DOM node + does not paint it: without the yield, a proof that runs synchronously for + seconds would run and settle the row before the first paint, and the + running test this issue exists to show would never be visible. The yield + sits before the sandboxed clock reads, so the reported duration is + untouched (the constraint below). - **A result type** may not need to change at all: a start is an event, not a result. Whether the reporter grows a sibling operation or its existing one gains a status is part of the design. -The reporter change is small; the interleaving question is the real one, and -it is the same question in both hosts, which is an argument for settling it in -the shared core rather than twice. +The reporter change is small, and the question that was the real one when +this was written — interleaving — is gone with the concurrency: under the +sequential runner nothing runs between a start and its own result. What +remains is the event's shape: whether the reporter grows a sibling operation +or its existing one gains a status, and what a start-then-result pair looks +like in each host. That is still the same question in both hosts, which is +still the argument for settling it in the shared core rather than twice. ### Constraints - A start event must not cost a `sandbox` call or a clock read of its own: the - duration reported is still the sandboxed one. -- Concurrency stays. Naming a test before running it must not serialize the - suite to keep the output tidy. + duration reported is still the sandboxed one. The browser start handler's + macrotask yield (above) is compatible: it lands before the sandbox's + adjacent clock reads, so it delays the start, not the measurement. +- The runner's scheduling is not this issue's to change, in either direction. + When this was written that meant "concurrency stays"; the sequential plan in + [share-browser-console-runner](share-browser-console-runner.md) has since + made the traversal sequential, which this issue simply inherits — and + benefits from: one leaf's events no longer interleave with another's, so a + start is followed by its own result, in both hosts. What sequential does + *not* buy is an empty gap between them — the leaf itself runs there, and + its output can land on the same stream — which is why the terminal format + above emits two complete records rather than completing an open line. - Whatever is emitted has to be as useful to an automated consumer as to a reader — a start with no matching result is precisely the signal a crashed run leaves behind, and a controller should be able to read it. @@ -69,8 +94,24 @@ the shared core rather than twice. - [ ] Add the start event to the reporter and call it before the leaf is sandboxed. -- [ ] Decide the terminal format for concurrent output, and prove it. -- [ ] Render a pending row in the browser page and settle it in place. +- [ ] Decide the terminal format, and prove it. Under the sequential runner + leaves do not interleave, so the question is the shape of a + start-then-result pair rather than how to keep concurrent lines + legible — but a leaf's *own* output can still land between its start + and its result, so the proof includes a proof that writes to the + terminal mid-test and shows both records intact around it. +- [ ] Render a pending row in the browser page, await one macrotask in the + start handler, and settle the row in place — and prove the *yield*, + not the append. A proof body that reads the DOM proves nothing here: + the pending node is appended synchronously before the await, so the + DOM looks identical with the yield deleted, and a blocking body cannot + see from inside its own task whether the browser painted first — an + item-11 coincidence proof in either shape. The proof is an ordering + sentinel: a macrotask enqueued before the start handler runs must be + observed to fire before the proof body starts (or a real-browser + observation of the painted row, as the burst was measured), and the + mutation check is deleting the await and watching the sentinel land + after the body instead. - [ ] Prove that a run killed mid-test leaves the running test's name behind. ### Related diff --git a/fjs/emergent_testing/todo/share-browser-console-runner.md b/fjs/emergent_testing/todo/share-browser-console-runner.md index a7caf9d9f..91b8c9952 100644 --- a/fjs/emergent_testing/todo/share-browser-console-runner.md +++ b/fjs/emergent_testing/todo/share-browser-console-runner.md @@ -23,7 +23,9 @@ entry much larger than it needs to be. ### How to do this — read before designing -An attempt at this issue was written, reviewed, approved and then reverted. It +A first attempt at this issue was written, reviewed, approved and then +reverted (a second, #1759, followed and is recorded in its own section below). +It worked: one shared `runModuleMap`, a `Reporter` per host, an `effects/common` layer, a browser interpreter, 100% coverage, green CI, a real Chromium run of 3435 proofs. It was reverted anyway, because *how* it got there is a cost this @@ -32,11 +34,19 @@ the code was. **The order of work is the deliverable here, not just the final shape.** See [DESIGN.md §4, "Follow the example"](../../../DESIGN.md). **One skeleton, with named parts.** The thing to share is the *runner itself*: -the order in which modules are linked, leaves discovered, bodies executed, -throws inverted, results counted and the run concluded. Both hosts run that same -skeleton. Everything host-specific is a **part** the skeleton calls at a place it -names — where the leaf body is executed, where a result is reported, where a -module is linked — and a part is where a browser is allowed to be a browser. +the order in which leaves are discovered, bodies executed, throws inverted, +results counted and the run concluded. Both hosts run that same skeleton. +Everything host-specific is a **part** the skeleton calls at a place it names — +where the leaf body is executed, where a result is reported — and a part is +where a browser is allowed to be a browser. This paragraph originally listed +"where a module is linked" among the parts, and building it settled the +boundary the other way: **linking happens before the skeleton, in host code, +and the skeleton accepts linked modules** — `fjs t` loads through its module +map, the page through its own importer with its own loading UI, and neither +shape fits a part the other host could supply. The tasks below record the +consequence: the runner exposes an entry point for a host that enumerates its +own modules, and enumerating a module's export is that host's own guarded +read. That gives exactly two ways to accommodate a host, both additive: change *that host's part*, or *improve the skeleton so every host benefits*. There is no @@ -47,7 +57,7 @@ supplies, rather than a special case. Differences between the parts are fine and expected: a DOM row and a terminal line are two implementations of the same named part, and the skeleton above them -cannot tell which it has. *Undocumented* differences are not. The attempt shared +cannot tell which it has. *Undocumented* differences are not. The first attempt shared the modules and then let the browser keep its own test-name format, its own scheduling policy and its own clock — none of which its host forced, and none of which belonged in a part. That is the failure mode: it *looks* like success — @@ -55,8 +65,11 @@ one module, one name — while two behaviours hide behind it, and two implementations behind two names would have been more honest, because nothing about the shared name signals the difference. -**`fjs t` is sequential, and that is a decision to copy, not a gap to fill.** -The attempt gave the browser a batch size — proofs launched in groups with a +**`fjs t` was sequential when that attempt forked from it, and that was a +decision to copy, not a gap to fill.** (Today's `fjs t` fans out through +`all`; the sequential plan below returns it to this paragraph's state, which +is the state everything here argues for.) The first attempt gave the browser a batch +size — proofs launched in groups with a yield between groups. Nobody had asked for it, no measurement motivated the constant, and it was premature optimization in the strict sense: it made the runner different from the example in order to solve a problem no one had @@ -68,8 +81,17 @@ made first-opener-wins; the yield needed `MessageChannel` rather than `setTimeout` only because `setTimeout` clamps to 4 ms once nested; and the `MessageChannel` proof then failed under bun, which drains port messages before running a due timer. Six rounds of review, every one of them downstream of a -constant that was finally deleted. The end state — no batch size at all — is -the state that copying `fjs t` would have produced on day one. +constant that was finally deleted. + +**And "no batch size" is not "no yielding" — this file said so badly enough to +mislead a later reader, which was me.** Copying today's concurrent `fjs t` +exactly *does* freeze a page: without a yield the whole suite runs as one +task, measured at 54.7 s on this repo's own browser suite, and the line above +about the batching having no paint boundary is about a bug in that attempt +rather than a finding that the yield did nothing. What the browser needs is a +turn per unit of work, and what it never needed was a number of proofs. The +second attempt's record below carries where that ended: with a *sequential* +run, the turn is one macrotask per report, in the page's own handler. **A problem the browser reveals is not a browser problem.** Two came up, and both are properly issues rather than fixes inside a port: @@ -84,24 +106,206 @@ both are properly issues rather than fixes inside a port: [Hostile thrown values and cross-realm promises](hostile-proof-values.md) and [Imports, promises and realms](imports-promises-realms.md). -The rule that follows: **land the shared skeleton with behaviour unchanged, then -take each new problem as its own change — in the skeleton where it belongs -there, so both runners get it, or in every part at once.** An improvement the -browser could have is an issue, not something to introduce inside a port. A -behaviour the port cannot preserve is a finding to record before it merges, not -a silent divergence to explain in review. - -**Keep the change reviewable.** The attempt was 2646 insertions and 1408 -deletions across 35 files in one PR — a move, a rewrite, a new effects layer, a -new host interpreter and a scheduling invention at once, which is why the -scheduling argument could not be separated from the sharing argument. Sequence -it: the shared semantics first, with `fjs t` unchanged in behaviour and the -browser file only calling into it; the layout moves after; anything genuinely -new last, on its own. +The rule that follows: **a port changes only the behaviour its own argument +requires, named and proved — everything else lands as its own change, in the +skeleton where it belongs there, so both runners get it, or in every part at +once.** An improvement the browser could have is an issue, not something to +introduce inside a port. A behaviour the port cannot preserve is a finding to +record before it merges, not a silent divergence to explain in review. (An +earlier version of this rule said "with behaviour unchanged" — right against +smuggled improvements, but stated too strongly once the plan itself became a +scheduling change the port necessarily brings to the page; the next paragraph +names what step 7b changes and why.) + +**Keep the change reviewable: one argument per PR.** The first attempt was +2646 insertions and 1408 deletions across 35 files in one PR — a move, a +rewrite, a new effects layer, a new host interpreter and a scheduling +invention at once, which is why the scheduling argument could not be separated +from the sharing argument. The sequence that keeps them separate is the one +the plan below orders: the scheduling change first, alone, in the console +runner where it is observable and provable without any port (step 7a); then +the port (step 7b) — which is itself a behaviour change for the *page*, three +times over, because 7a touches only `module.f.mjs` and the page does not run +that code until the port: the page's scheduling goes from 25-at-a-time +concurrent batches to sequential, its live progress goes from +children-before-parent to the structural order, and `runModuleMap`'s answer +changes shape. One argument per PR still holds — 7b's argument is the port, +and its behaviour changes are the browser's side of decisions 7a and this +plan already made and named, each carried in 7b's changelog and proofs rather +than discovered in review; the layout moves after. An earlier version of this paragraph said "shared +semantics first, with `fjs t` unchanged in behaviour" — right about +separation, wrong about order once the plan itself became a scheduling change: +porting first would have moved the browser onto semantics about to change +under it. + +### The second attempt (#1759), and the plan it simplified to + +A second attempt was also written, reviewed — twenty-one review threads, two +independent approvals — and reverted with every gate green: `tsc`, 3,547 +proofs, 100% coverage, a real Chromium run. It shared the traversal exactly as +the steps below asked: one `runModuleMap`, a `Reporter` answering each +host's own leaf record, a browser interpreter for `sandbox`, `catch` and +`all`. The owner reverted it for a reason the first attempt's record already +contains but did not say loudly enough: **the concurrency was the complexity.** +Every hard problem the review fought traces to the traversal fanning out with +`all`, and the machinery each fix added — a frame budget, a guessed 8 ms +constant, `scheduler.yield`/`MessageChannel` selection — is infrastructure a +test runner shouldn't need. The requirement, stated by the owner: a simple, +sequential run, no optimization, a clear message after each test, exactly as +the CLI works. Speed is explicitly not a goal. + +#### The plan: sequential + +Run one leaf's **whole chain** — test, report, children — to completion before +the next leaf starts. That is the entire design. Its consequences: + +- **The reporting burst is impossible by construction.** Each leaf's report is + awaited before the next leaf runs; the interleaving is the control flow, not + a property to enforce or prove around. +- **The page yields in its own `report` handler**: append the row, await one + macrotask, answer. That is the browser's spelling of what the CLI's `write` + already is — print the line, let the terminal show it, run the next test. It + is page code in the impure shell, so no scheduling policy touches shared + code, and there is no constant to guess. (`setTimeout(0)`'s nested 4 ms + clamp costs ~4 ms per test; speed is not a goal, and bun never runs page + code, so the clamp forces nothing.) +- **No frame budget, no yield primitive selection, no batch size.** The longest + blocking task is the longest single proof, with zero tuning. +- **The traversal never fans out**, so the variadic-`all` argument ceiling + ([all-argument-limit](../../effects/todo/all-argument-limit.md)) leaves the + traversal entirely, and the browser interpreter needs only `sandbox` and + `catch`. +- **`fjs t`'s output becomes honest**: lines print after each test in + structural order, and per-leaf durations stop being inflated by concurrent + wall time — today a browser-suite leaf reports ~20 s because ~130 others + share its clock. +- **The cost**: wall clock becomes the sum of awaits instead of the max, and a + proof that secretly depends on a sibling running concurrently deadlocks. + Both are accepted; the second is a timing dependency being flushed out. + `all` and `both` remain as *operations* for programs that want concurrency — + only the traversal stops using them. + +Sequence it as two PRs: **first the sequential traversal in `module.f.mjs` +alone** — console-observable, `fjs t` prints each line as its test finishes, +the full suite run under it is what finds any concurrency-dependent proof, and +the scheduling change is breaking and gets its own changelog entry — **then the +browser port**, which invents no scheduling of its own: the sequential order +arrives with the shared traversal the page now calls (a page-behaviour change, +named as such in step 7b and the reviewability paragraph above), the page's +`report` handler yields one macrotask as page code, and the browser +interpreter contains no scheduling at all. This order — the idea first, in the +context that can prove it, then a port that carries no idea of its own — is +the sequencing [DESIGN.md](../../../DESIGN.md) describes for a change that is +the plan's premise rather than the port's discovery. + +#### The pitfall catalog + +Every problem the second attempt met, its cause, and the solution that worked. +The first group is dissolved by the sequential plan; the second group applies +to **any** implementation and the next implementer must not rediscover them; +the third is about method. + +**Dissolved by sequential:** + +1. **The single-task freeze.** Leaves resolve through microtasks, and a + microtask drain never returns to the event loop, so the whole suite ran as + one task — measured in Chromium: **54.7 s**, zero paints, the browser + offering to kill the page. #1759's fix was a frame budget in the + interpreter, which worked (longest task 97–104 ms) and is exactly the + machinery the sequential plan deletes: one macrotask per report gives a + task per test with no budget at all. +2. **The reporting burst.** Under `all`, every child starts before any is + awaited, so each leaf's `report` — a *continuation*, a microtask — queues + behind the entire suite's execution. Measured: first row in the DOM at + **44.3 s of a 50 s run**, 90% of 3,461 rows within ~30 ms of each other. + No budget can fix this — the ordering is the traversal's, and disabling the + budget left the burst unchanged. `fjs t` has it by construction too. +3. **The variadic `all` ceiling.** Every fan-out is a spread, a spread is a + call, and a call has an argument limit: 50,000 siblings build, 100,000 + throw `RangeError` **while building the effect**, before any interpreter + can catch it. Sequential removes every traversal site; + [all-argument-limit](../../effects/todo/all-argument-limit.md) keeps the + rest. +4. **`batchSize = 25` was doing two unnamed jobs**: its `setTimeout` between + waves was the page's only macrotask boundary, and awaiting each batch + bounded how far reporting lagged execution. Nobody chose it for either. (A + third was claimed during review — staying under the argument ceiling — and + was a misattribution: `Promise.all(batch.map(…))` passes one iterable, so + the old runner had no spread at any batch size; the ceiling is item 3's, + the variadic operation's.) The lesson is not that the constant was right — + it was indefensible — but that **before deleting unmotivated code, + enumerate what it does, not what it was for.** + +**These survive into any implementation:** + +5. **Enumerating is user code; read once.** A getter runs on every read. A + preflight `collectTests` that only *checked* the tree ran every getter a + second time, and one that succeeded then threw escaped as a synchronous + throw — page stuck in `running`, no report, no completion event. The same + bug recurred one layer down in the same PR: a collision check enumerated + the interpreter's `extra` map and the construction enumerated it again, so + a proxy could hide a key from the check and reveal it to the build. The + rule both times: **read a user value once, and derive everything from that + one reading.** +6. **The page's modules are a list, not a map.** Routing them through a + record-shaped `ModuleMap` let `Object.fromEntries` keep only the last of + two same-labelled modules and report it twice. Two entries with one label + are two runs, in the order passed. +7. **A run must not start before its promise is published.** A leaf executes + synchronously inside its handler, so without a deferral the first proofs + run while `runBrowserProofs` is still building what it returns — a proof + reading `fjsBrowserTestReport` sees the previous run's promise. Defer + everything that runs user code (enumeration included) behind one + `Promise.resolve().then(...)`. +8. **Both ways a run fails as a runner must end in a report.** The error + channel carries what an operation reported; a *rejection* carries what the + interpreter could not dispatch at all, and an unhandled one is a page stuck + in `running` forever. Handle both into the `infrastructure-error` report. +9. **Joins must be linear, and sequential does not grant that for free.** + Pairwise immutable concatenation was Θ(N²) twice — across siblings, then + again down a parent/child chain, where "flatten once at the end" recopies + each subtree once per ancestor and is the same Θ(N²) moved. The fix that + worked was a rope: joining is one node naming both sides, `toArray` walks + it once where the run ends. A sequential fold changes execution order, not + concatenation cost — an immutable `[...acc, r]` append copies the prefix + every iteration and is the same Θ(N²) — so the port keeps the rope, or + another accumulator that is demonstrably linear. +10. **A new exported boundary that its own consumers cast past is not typed.** + `browserRun` began as `(effect: unknown) => Promise` with `any` + casts at both call sites, and its `extra` was `Partial` — advertising a + recovery the dispatcher does not perform (it panics on an unclaimed + command, by design). Make it generic over the effect and its `Result`, + take a complete map, panic on a handler that claims a core operation + (silently letting either side win makes the type or the caller a liar), + and carry handlers by property *descriptor* — `match` looks handlers up + with `getOwnPropertyDescriptor`, so a spread-merge silently drops a + non-enumerable handler the layer's dispatch would have accepted. + +**Method:** + +11. **A proof that observes a coincidence is worse than no proof, because it + is counted as cover.** A proof that the budget yielded watched for *a* + macrotask turn during a run; under the full suite a neighbouring proof + supplies one anyway, so it stayed green with the defect present — sound in + isolation, inert where the project runs it. Assert by *ordering* (a + macrotask cannot run until every pending microtask has) or by structure, + never by observing that the loop turned. And mutation-check under the full + `npm test`, which is the only run that counts — the inert proof passed its + own isolated mutation check. +12. **Measure what the user sees, not a proxy for it.** "392 frames served and + 194 progress updates" was reported as "rows painting as they land"; the + frames were real and dominated by the loading phase, and row count over + time — the thing a person watches — was never sampled. It read 0 until the + end. Sample the artifact itself. +13. **When a decision changes, grep the markdown for the old one.** Seven + review findings on one branch were the same shape: the new answer written + down with the superseded instruction left standing beside it, handing a + future implementer two designs. This file is long precisely so it can be + wrong in one place; keep it saying one thing. ### Steps -**One step per pull request.** The reverted attempt did the whole issue at once +**One step per pull request.** The first attempt did the whole issue at once — 2646 insertions and 1408 deletions across 35 files — and that is why its arguments could not be separated: a question about scheduling became a question about the port. Each step below stands on its own, leaves both runners working, @@ -125,23 +329,46 @@ and is reviewable without the next one. in [imports, promises and realms](imports-promises-realms.md); the scope rule they rest on is in [browser testing](browser-testing.md). -- [ ] **4. Common effects.** Move the host-independent operations (`all`, - `await`, `sandbox`, and whichever of `now`, `fetch` and `import` survive - the test below) out of `effects/node` into a shared module that - `effects/node` re-exports unchanged, so nothing has to move with them. - - **Three of that list are unsettled, and this step does not get to assume - them.** `all`, `await` and `sandbox` are agreed: - [node-module-layering](../../effects/todo/node-module-layering.md) moves - them too. But that issue keeps `Now`, `Fetch` and `Import` in - `effects/node` on a reader-benefit argument, and this step was written - listing all three as moving. Neither was written knowing the fact that - decides it — which operations the step-5 interpreter actually implements — - so step 5 settles them and updates both files in the same change. The - expectation recorded there: `now` and `import` move (a browser proof run - needs a clock and dynamic import), `fetch` stays (nothing in the shared - runner performs one, and DESIGN.md §4 extracts at the second *real* - consumer). +- [ ] **4. Common effects.** Move `sandbox` and `catch` out of `effects/node` + into a shared module that `effects/node` re-exports unchanged, so + node-side callers keep one import. The re-export is legitimate here by + [node-module-layering](../../effects/todo/node-module-layering.md)'s own + test — a re-export is a shim only when it keeps a *dead* coupling + alive, and `NodeOp` is declared over `Sandbox` and `Catch`, so + `effects/node` genuinely uses what it re-exports. The modules the move + exists for — the shared traversal, the browser interpreter — import the + new home directly. + + **The list was settled by measurement, then shrank again by design.** + The reverted #1759 interpreter implemented exactly `sandbox`, `catch` + and `all`, so exactly those three had a second implementer. Under the + sequential plan the traversal performs no `all`, so the set with two + implementers — and this step's whole scope — is **`sandbox` and + `catch`**. `all` is not this step's to move at all: its home is + [node-module-layering](../../effects/todo/node-module-layering.md)'s + question, which moves it to `effects/all` on the layering argument, with + the Node runners and the registration path as its implementers. `await` never qualified: it belongs to that + registration path, which no browser runs — though it *moves* with + `sandbox` and `catch`, to the same `effects/sandbox` home, on + node-module-layering's layering argument rather than on this step's + second-implementer one; that move is that issue's, not step 4's. `import`, `now` and `fetch` + never qualified either: a page loads modules through its own importer + and reads its own wall clock, in the impure shell where host values + belong. Everything without a second implementer stays in `effects/node` + until something gives it one — the same rule that shrank this list + twice. [node-module-layering](../../effects/todo/node-module-layering.md) + carries the same answer. + + **The expectation this step was written with was wrong, which is why the + list was measured rather than argued.** `all`, `await` and `sandbox` were + agreed all along. `Now`, `Fetch` and `Import` were not: this step listed + all three as moving, on the reasoning that a browser proof run needs a + clock and dynamic import. That is true of the *page* and false of the + *effect set* — the page reads its own clock and calls its own importer, + in the impure shell where host values belong, and neither reaches the + interpreter as an operation. Reasoning from what a host *can* do + predicted one answer; reading what the interpreter had to implement gave + another. **The vocabulary went first, and it was not speculative.** Before an operation can move, the types it is *declared in* have to have a home: @@ -175,21 +402,22 @@ and is reviewable without the next one. and this took it. `fjs t` gained the behaviour in the process, which is what made that change worth landing on its own rather than inside the port. -- [ ] **5. A browser interpreter** for exactly those operations, with no - scheduling policy of its own. This is also what earns step 4's *operation* - move its second consumer: until a second host implements `sandbox`, - `await` and `all`, moving them out of `effects/node` makes nothing shorter - or clearer, and DESIGN.md §4 says to extract at the second real consumer - rather than before it. The two are therefore one design in two commits, - not one step deferred. - - **Its operation set is also the ruling** on `now`, `fetch` and `import`, - which step 4 and - [node-module-layering](../../effects/todo/node-module-layering.md) - currently disagree about. What this interpreter implements is what has a - second consumer; what it does not implement stays in `effects/node` until - something needs it. Write the answer into both files in this step's own - change, so neither is left asserting what the other denies. +- [ ] **5. A browser interpreter** for `sandbox` and `catch`, plus whatever + operations the application adds — for the page, one `report`. Nothing + else: a sequential traversal performs no `all`. `sandbox` is + `effects/node`'s, copied rather than redesigned, because two runners + that disagreed about an awaited leaf would not be one runner; `catch` + dispatches to `types/result`'s `tryCatch`, the same helper + `effects/node` uses. + + **No scheduling policy of its own — and this time that holds without a + footnote.** The reverted #1759 interpreter had to carry a frame budget + because the concurrent traversal ran as one microtask drain (catalog + item 1). Sequentially, the page's own `report` handler yields, and the + interpreter's handlers are dumb. Its contract still wants the reverted + attempt's proofs re-landed: a complete non-`Partial` map, a panic on a + colliding or unclaimed command, handlers carried by descriptor, the map + read once (catalog items 5, 8, 10). - [x] **6. One reporter.** The event stream — a leaf landed, a run ended — that both hosts subscribe to. Step 2 gave them the *value*; this gave them the seam it travels through. `Reporter.result` now receives the @@ -204,15 +432,88 @@ and is reviewable without the next one. the raw `SandboxResult` still travels next to the `TestResult`, because describing a *thrown value* is each host's part (step 2's finding); and the browser report's own `duration` stays wall-clock rather than the - fold's summed durations, because its leaves run concurrently and the sum - only means "how long the run took" for a sequential runner — - `RunTotals` documents that. -- [ ] **7. One skeleton.** The page's proof-tree walk is deleted and the shared - traversal runs it. The walk's `batchSize = 25` batching goes onto the - table with it: that is a scheduling policy of the page's own — the same - kind the reverted attempt was faulted for inventing, though this one - predates it in `browser.mjs` — and step 7 is where it gets decided - rather than silently inherited. + fold's summed durations. When this landed the reason was concurrency; + under the sequential plan the two draw closer but stay distinct — wall + clock also carries what is *between* the leaves: the per-report yields, + enumeration, joining, everything the run does that no leaf owns. (Not + module loading: the page's timer starts after its imports settle, and + keeps doing so.) Step 7b updates this reasoning where it is published, + in `RunTotals`'s JSDoc (`types.ts`), which today still explains the gap + by concurrency. +- [ ] **7. One sequential skeleton.** Two PRs, in this order. + + **7a. Make the shared traversal sequential**, in `module.f.mjs` alone. + Replace the `all` fan-outs with a sequential fold: one leaf's whole + chain — test, report, children — awaited before the next leaf starts, + for siblings and for modules alike. Console-observable and + console-provable: `fjs t` prints each line as its test finishes, in + structural order, and per-leaf durations become the leaf's own time. + Breaking (scheduling semantics), so it carries its own changelog entry. + Run the full suite under it *in this PR* — a proof that depends on a + sibling running concurrently deadlocks here, where it is cheap to find, + not in the browser port. The suite run is *not* the proof of the + sequential contract, though: the suite is green under the concurrent + traversal too, so it would stay green if a later edit restored a + fan-out. The contract gets its own proof, one that fails when work + overlaps — leaves that record enter/exit order under a mock interpreter + and assert no interleaving between one leaf's start and its finish, or + an assertion that the traversal's chain issues no `all` command — and + per catalog item 11's discipline the proof is mutation-tested: restore + one fan-out, watch it fail, revert. + + **7b. The page runs the shared traversal** through the step-5 + interpreter. `browser.mjs` stops discovering leaves, applying the throw + expectation, walking return values and counting: it supplies a + `Reporter` whose `result` hands the record to its `report` operation, + and a `report` handler that appends the row and awaits one macrotask. + That await is the port's only boundary against the single-task freeze + (catalog item 1), and the page proof's fake document cannot see + painting — every semantic assertion stays green with the await + deleted, the incidental-yield trap item 11 names. So the boundary gets + its own ordering proof: a macrotask enqueued before a result is + reported must be observed to fire before the next leaf runs, + mutation-checked by removing the await and watching the sentinel land + after the whole suite instead. + Update `RunTotals`'s JSDoc in `types.ts` here too: it explains + wall-clock-vs-summed-duration by leaves running concurrently, and under + this step the gap is what the run does *between* leaves — per-report + yields, enumeration, joining — not concurrency. Module loading is not + part of it: the page's timer starts after its imports have settled, and + stays there (step 6's note carries the same correction). + What the reverted #1759 validated and this PR re-lands: the traversal + threads a `RunOutcome` — folded totals plus each host's leaf records + in the walk's order (`fjs t` answers `void` and collects nothing). + **That is a breaking change to `runModuleMap`'s exported answer** — + today it is an exit code, `0 | 1` — and re-landing it carries the same + obligations it carried the first time: an `exitCodeOf` helper for + callers that want the code, every in-repo importer migrated in the same + PR, and a changelog entry with the `**BREAKING CHANGES:**` prefix + naming the return-shape migration — and, in the same entry, the two + page-behaviour changes this port carries: the page's scheduling moves + from 25-at-a-time concurrent batches to the sequential traversal (7a + changed only `module.f.mjs`; the browser acquires the scheduling + here), and live progress adopts the structural order (the paragraph + below). Both are proved in this PR, not just listed. Also re-landed: the page's modules + stay a *list* entered at a seam for already-collected leaves, because + labels may repeat and an export is enumerated exactly once, under the + page's own guard (catalog items 5, 6); the run starts only after its + promise is published (item 7); and both runner-failure routes end in + the `infrastructure-error` report (item 8). + + **One observable ordering change rides with this port, deliberately.** + Today's page announces a returned tree's *children before their parent* + — the parent's `result` callback fires after `Promise.all(children)` — + while the shared traversal reports a parent before the children its + return value produced, which is the structural order the report and + `fjs t` already use. The port adopts the shared order for live progress + too; prove it rather than inheriting it silently. + + **What stays the page's own, with the reason:** reading a *module's* + exported tree. The shared walk guards a returned tree through `catch` + (see [hostile proof values](hostile-proof-values.md)) but deliberately + not the exported one, because there is no leaf to attribute that failure + to. `fjs t` panics; the page catches it and reports one failed module. + - [ ] **8. The layout move**, and the website preparation program. Steps 3 and 7 are the ones that change behaviour, so they are the ones to keep @@ -372,15 +673,15 @@ an effect adds an operation for every DOM detail without improving the shared API. Add `fjs/effects/browser/` only after the required operation set is clear; do not create a mirror of `effects/node` merely for directory symmetry. -A shared `all` that starts every child before awaiting any is worth stating as a -contract rather than leaving to each interpreter: a child may wait on something -a later sibling produces, so an interpreter that awaits one child before -starting the next hangs a graph the other host completes. Beyond that, **the -browser gets no scheduling policy of its own until someone reports a problem -with the one `fjs t` has.** If a page turns out to need a task boundary to -paint, that is a separate, measured change with its own issue — and the measure -is a boundary per unit of work, never a tuned count of proofs, because proofs -differ in cost by orders of magnitude. +**The traversal is sequential, and that is the scheduling policy — the whole +of it.** The second attempt proved the alternative: a concurrent traversal +needed a frame budget to stay responsive and still delivered its log as one +burst, because no scheduling layer can reorder a continuation ahead of work +already queued (catalog items 1–2). Sequentially, the only scheduling decision +left is the page's one macrotask per report, in the page's own handler. `all` +keeps its start-every-child-before-awaiting-any contract for the programs that +still use it — the registration path, and any program that wants concurrency — +but the traversal is no longer one of them. An executor boundary will still be necessary because the console runner uses the Effects sandbox while a browser catches synchronous throws and awaits @@ -442,13 +743,22 @@ are shared. ### Tasks -- [ ] Inventory duplicated semantics in `emergent_testing/module.f.mjs` and - `emergent_testing/browser.mjs`, and define the smallest shared API. -- [ ] Name the skeleton's parts explicitly — execute a leaf, report a result, +- [x] Inventory duplicated semantics in `emergent_testing/module.f.mjs` and + `emergent_testing/browser.mjs`, and define the smallest shared API. The + shared API is `Reporter` and the `RunOutcome` the traversal + answers with; the page supplies the parts and nothing else. Implemented + and review-validated in the reverted #1759; the design survives as the + plan and re-lands with step 7. +- [x] Name the skeleton's parts explicitly — execute a leaf, report a result, link a module — and check that nothing host-specific is left outside one - of them. + of them. `test`, `result` and `summary` are the parts; linking a module + stays outside the skeleton, which is why the reverted #1759 gave + `runModuleMap` a sibling entry point taking already-collected leaves, + and step 7b does again. - [ ] Make the existing `collectTests`/path behavior the single source of truth - for console and browser execution. + for console and browser execution. Done in the reverted #1759 — the + page's walk was deleted, `collectTests` called once under the page's own + guard — and re-lands with step 7b. - [x] Share the test-name format, and prove both runners name the same leaf identically. The browser report carries a `name` built by `fmtImport`, and `nameMatchesTheConsoleRunner` pins it to that function rather than to a @@ -458,8 +768,13 @@ are shared. `TestResult`, built by `testResult`, carrying identity, status and duration. Progress, infrastructure-error, totals and report values are still each host's own. -- [ ] Decide whether browser import/time/yield/publication justify +- [x] Decide whether browser import/time/yield/publication justify `fjs/effects/browser/`; document the decision before adding operations. + They do not: the reverted #1759 interpreter needed `sandbox`, `catch` + and `all` and nothing else, and the sequential plan drops `all` too — + import, time, yield and publication are all the page's, in its impure + shell. Recorded in + [node-module-layering](../../effects/todo/node-module-layering.md). - [ ] Move static proof discovery and `_browser-suite.mjs` generation into `fjs/website/module.f.mjs`; extend `fjs/effects/node/` only for a concrete missing capability and prove the real and virtual interpretations. @@ -475,11 +790,50 @@ are shared. - [ ] Update the generated website entry and browser-test application imports to the new module paths. - [ ] Prove both runners produce equivalent paths, throw outcomes, recursive - test counts, and normalized failures from the same fixtures. -- [ ] Record every behaviour the browser file has today and the shared core will - not keep, as an issue, before the sharing change merges. + test counts, and normalized failures from the same fixtures. The + existing `nameMatchesTheConsoleRunner`, + `expectedThrowStatusMatchesTheSharedOne` and + `normalizedResultMatchesTheSharedOne` already assert against the console + runner's own functions; step 7b makes the four properties shared code + rather than agreeing implementations. +- [x] Record every behaviour the browser file has today and the shared core will + not keep, as an issue, before the sharing change merges. Two: the + `batchSize = 25` yielding — whose *constant* was the mistake and whose + *yielding* was load-bearing, see below — and the unguarded read of a + module's *exported* tree, which stays the page's own and is tracked by + [hostile-proof-values](./hostile-proof-values.md). - [ ] Close each of those issues for both runners at once, so the two stay in sync rather than drifting from the day the core is shared. +- [x] Decide where a browser run gives the thread back. **One macrotask per + report, in the page's own `report` handler** — the sequential plan's + answer, superseding the reverted #1759's frame budget. The full story + of how the frame budget was got wrong three times before being measured, + and why even measured-correct it could not fix the reporting burst, is + the pitfall catalog above (items 1, 2, 4, 11, 12). +- [ ] Prove `runBrowserProofs`'s `infrastructure-error` branch — the run's + own failure, as opposed to any proof's — **in step 7b, with the + minimal seam that makes it reachable.** Neither half of the branch (an + operation reporting through the error channel, or one the interpreter + cannot dispatch, which rejects) is reachable through the public entry + point — the reverted #1759 proved that by mutation: removing the guard + stayed green. An earlier version of this task concluded "land the + guard in 7b, record it unproven, prove it at step 8's + `module.f.mjs`/`module.mjs` split" — superseded, because that ships a + branch known to be untested whose failure mode is a page stuck in + `running` forever, exactly the class of hazard catalog item 11 exists + for. 7b instead carries the seam itself, at its smallest: the page's + run core takes its interpreter (or reporter) as an argument and is + exported for proofs from the page's own module, so proofs drive **each + failure route separately** — one case for an operation answering + through the error channel, one for an operation the interpreter cannot + dispatch, which rejects — and watch the `infrastructure-error` report + land from both. Two routes need two mutations: delete either half of + the guard alone and its case fails while the other stays green, or the + surviving half is masking an untested branch that can still leave the + page in `running` forever. This is a testing seam, not a public-API widening — the + page's published entry point is unchanged, which is what the rejected + "widen the API to reach the branch" alternative got wrong. Step 8's + full layout split then absorbs the seam rather than creating it. ### Related @@ -488,7 +842,10 @@ are shared. - [Test-runner behavior](661-test-runner-behavior.md) — documented differences that must remain intentional after sharing the core. - [Test tree walker](65z-tf-test-tree-walker.md) — earlier work around recursive - proof-tree traversal. + proof-tree traversal. Its sketch predates the sequential plan and hard-coded + `all` sibling fan-out; that issue now requires the sibling combination to be + the instantiation's parameter (sequential for the run path, fan-out for + registration), so a later walker cannot undo step 7a's scheduling. - [Hostile thrown values and cross-realm promises](hostile-proof-values.md) — a behaviour the browser has and `fjs t` does not; decide it, do not inherit two answers. diff --git a/fjs/emergent_testing/todo/timer-precision.md b/fjs/emergent_testing/todo/timer-precision.md index edb330b27..ea88502ea 100644 --- a/fjs/emergent_testing/todo/timer-precision.md +++ b/fjs/emergent_testing/todo/timer-precision.md @@ -53,12 +53,17 @@ before changing the measurement. report is serializable and consumed by controllers, so a `resolution` field would let a consumer decide what is significant instead of guessing. - **Accumulate over a group.** The idea raised when this was filed: time a - batch of leaves with one pair of reads and divide, so the clamp is amortized + group of leaves with one pair of reads and divide, so the clamp is amortized across many proofs instead of applied to each. This is speculation — it - trades a per-test number for an average, it cannot attribute a slow proof, - and it interacts with concurrency, since `all` interleaves launches and a - group's wall time would then include siblings' work. Worth prototyping, - not worth assuming. + trades a per-test number for an average and cannot attribute a slow proof. + When this was filed the objection was concurrency: `all` interleaved + launches, so a group's wall time included siblings' work. The sequential + plan in [share-browser-console-runner](share-browser-console-runner.md) + retires that objection — one leaf's whole chain finishes before the next + starts — but replaces it with a smaller one: a group's span now also + carries the between-leaves overhead (the per-report yield, enumeration), + so the divided average still is not the leaves' own time. Worth + prototyping under the sequential traversal, not worth assuming. - **Cross-origin isolation.** Serving the eventual application root with `COOP: same-origin` and `COEP: require-corp` buys Chromium's 5 µs clock and is a header change in the shared controller, not a design change. It does @@ -88,8 +93,11 @@ before changing the measurement. and WebKit from inside the runner, and record the figures here. - [ ] Decide whether the report carries the resolution, and whether a row below it renders a duration at all. -- [ ] Prototype accumulated timing over a group of leaves and check what it - costs in attribution and what concurrency does to it. +- [ ] Prototype accumulated timing over a group of leaves under the + sequential traversal, and check what it costs in attribution and how + much between-leaves overhead (report yields, enumeration) lands inside + the group's span — the concurrency half of this question is gone with + the sequential plan. - [ ] Check whether cross-origin isolation is worth the headers in the shared controller.