Wave-parallel recalculation foundation + evaluator hardening: aggregate-fold memoization, stack-safe Tarjan, dynamic-name classification (GH-520)

[arcaputo3]

Closes #520. Final PR of the perf stack (#521 → #523 → this).

Two commits:

1. Wave-parallel non-iterative recalculation (equivalence-gated)

recalc --parallel N / recalculateParallel(n): cells partition into longest-path depth classes (antichains — no dependency paths within a wave); each wave evaluates on a fixed thread pool, results fold back in sequential order for bit-for-bit parity. Refuses seeded-Rng and iterative modes. Seven equivalence gates in ParallelRecalcSpec pin parallel ≡ sequential on wide grids, deep chains, error books (including error ORDER), cyclic cores, INDIRECT buckets, and repeated runs.

2. Evaluator hardening + acceleration

• Aggregate-fold memoization with single-flight: repeated SUM/AVERAGE/... over the same range fold the range ONCE per calculation generation; parallel readers single-flight one eligible fold. Generation-keyed so cache writes that leave the aggregated range unchanged reuse snapshots (AggregateMemoSpec, 8 gates).
• Stack-safe iterative Tarjan SCC engine shared by cycle detection paths; cycle diagnostics now report an actual directed ring, not Tarjan pop order (100k-node cycle stack-safety pinned).
• Workbook-aware dynamic classification: INDIRECT/OFFSET reachability resolves through parseable defined-name chains with evaluation's own case-insensitive, sheet-scoped-shadowing rules.
• Bounded-dependency extraction caching; staged -o/-i output commits (a killed process can no longer leave a torn destination).

Benchmarks (M-series, JVM assembly; baselines = 0.19.1 release)

workload	0.19.1	after #521+#523	this PR	total
9,900 × SUM($A$1:$A$5000)	47.9s / 6.0GB RSS	27.9s / 1.7GB	1.30s / 0.71GB	36.8×
200k-formula book	95.0s	20.0s	13.8s	6.9×
200k @ -Xmx512m	137.9s (450s user)	17.4s	10.8s	12.8×
50k-formula book	8.87s	4.39s	3.14s	2.8×

Honest --parallel assessment: ~1.0× on these shapes today — the aggregate memo eliminated the redundant folding that parallelism previously attacked, and the remaining cost is serial graph construction + ordered evaluation. It ships as a correctness-proven foundation that inherits every future serial-cost reduction.

Full suite: ./mill __.test → 1028/1028 SUCCESS (including post-merge with main's lint additions).

🤖 Generated with Claude Code

[claude]

Review — wave-parallel recalculation (GH-520)

I traced the wave partition against kahnOrder, fromWorkbookBounded, EvalMemo, and Rng.system. The core design holds up, and I convinced myself of the things that matter most:

• The partition is genuinely safe. depth(u) > depth(v) whenever u depends on v, so same-depth cells have no path between them. The depths are also computed over the right graph: a bucket node can never precede a main node (the bucket is closed under qualifiedTransitiveDependents), so the longest path to any main node lies entirely inside orderedMain — restricting the depth scan to inOrder loses no edge.
• No shared mutable state leaks into the workers. The EvalMemo default arg (Evaluator.scala:347) constructs a fresh instance per call, so each cell recursion tree gets its own; Sheet/Workbook have no lazy vals or caches; Rng.system really is ThreadLocalRandom. Publication is correct too — pool.submit and Future.get give the happens-before edges on both the snapshot going in and the results slots coming out, and workers only read.
• Extracting evalOne/foldResult and sharing them verbatim is the right call: it makes per-cell divergence structurally impossible rather than test-enforced.
• Refusing a seeded-Rng variant and an iterative variant, rather than papering over them, is also right. So is the honest benchmark table.

Findings below, most significant first.

---

1. Error-order parity rests on an undocumented invariant of kahnOrder

evalWaves folds results wave-by-wave, so RecalcResult.errors comes out in wave order, while evalPass produces it in orderedMain order. Those agree only because FIFO-seeded Kahn happens to emit longest-path level order: with all in-degree-0 nodes seeded up front and a strict FIFO queue, a level-k+1 node is enqueued during the level-k block (its last-dequeued predecessor is necessarily its level-k one), so levels come out contiguous. That holds for DependencyGraph.scala:846-884 today, so the parity claim is true as written.

But nothing at that call site records that the emitted order is load-bearing for anything beyond topological validity. Its docstring is careful about preserving the exact emitted order versus the pre-GH-518 formulation — exactly the kind of comment a later optimization relaxes. Swap the ArrayDeque for a stack, a priority queue, or a DFS-ish variant for cache locality and errors silently reorders on the parallel path only. GH-521 already touched this area, so it is live code.

Two ways to de-risk, either is fine:

• Note it in the kahnOrder scaladoc: emitted order must stay level-monotone, because WorkbookEvaluator.evalWaves partitions by longest-path depth and folds in wave order.
• Or make it structural: carry each result orderVec index through the fold and sort errs by it at the end. Then error order stops depending on the grouping at all, and the invariant cannot rot. sheets and evaluated are already order-independent, so this is the only field at risk.

2. ordered (line 322) is now dead, and its comment no longer matches

val ordered = orderedMain ++ orderedBucket has no remaining reader — the only other mention is the comment at line 550 ("literally the same single evalPass(ordered, ...) with the caller clock"), which no longer describes the code. It also allocates a full concatenated List of every formula node for nothing: roughly 200k cons cells on the exact book this stack is tuned for. Delete the val and refresh the comment.

3. inOrder (line 433) is redundant

The depthOf key set is exactly orderVec, so depthOf.getOrElse(p, 0) already yields 0 for every p outside the order — the inOrder.contains(p) guard can only skip a lookup that would have contributed 0 to a math.max. Dropping it removes an n-element Set allocation from a path that runs at 200k nodes under a 512 MB heap.

4. f.get() can escape, and the pool keeps burning after it does

evalOne catches NonFatal per cell, which is what keeps recalculate total. In the parallel path a fatal escape (a StackOverflowError from a deep recursive re-derivation, OOM) comes back out of f.get() wrapped in ExecutionException — a different exception type than the sequential path surfaces for the same input. Meanwhile finally pool.shutdown() does not interrupt, so the remaining chunks of a 9,900-cell wave keep computing after the failure. shutdownNow() in the finally, plus unwrapping ExecutionException so callers see what they would see sequentially, costs a few lines.

5. The determinism caveats omit the clock

The scaladoc lists two caveats (seeded Rng, iterative) but not Clock. recalculateParallel(parallelism) passes Clock.system and the non-iterative branch never pins it, so NOW() cells within a single wave sample at different instants. Sequential has the same property, so this is not a regression — but "identical element for element" has a third asterisk that neither the scaladoc nor the PR body states. The iterative path already pins once via Clock.fixed for precisely this reason; doing the same for the parallel entry point would make the equality claim unconditional instead of caveated.

(continued in next comment: CLI purity, test coverage, performance, docs)

[claude]

(continued from the previous comment)

6. CLI: System.err.println in a pure region, at the wrong time

In WriteCommands.recalc, val result = ... is computed eagerly at IO-construction time, so the NOTE prints when the IO is built, not when it runs — and it prints even if the IO is never run. The eager computation is pre-existing (and harmless while it is pure), but adding a raw side effect to it is not.

Every other advisory in this same function goes through the returned summary (renderSeedWarning into warningLines). Following that convention fixes purity, ordering, and testability in one move; Console[IO].errorln sequenced into prepared also works if stderr is deliberate.

While you are there: the new wb.metadata.calcPr.filter(_.iterativeCalculation).map(IterativeCalc.fromCalcPr) duplicates recalcHonoringCalcPr (line 911) verbatim. Threading parallel: Option[Int] into that helper keeps the "does this book iterate?" decision in one place — its docstring already claims to be the single funnel for every recalculating verb.

7. MaxParallelism clamps silently; the pool can be over-provisioned

--parallel 128 on a 128-core box becomes 64 with no feedback, and there is no auto / availableProcessors default, so the user has to guess. Separately, threads = min(parallelism, widest wave) would avoid spinning up 64 pool threads for a book whose widest wave is 20 — tasks chunk to one cell each in that case, so the threads really do get created.

---

Test coverage

The new CLI behavior is untested. BatchRecalcSpec covers parsing only. Nothing exercises WriteCommands.recalc(..., parallel = Some(n)), and nothing pins the iterative-fallback NOTE — which the PR body states as a contract ("never silent"). That is the branch most likely to rot, precisely because no unit test touches it.

This PR thesis is a law, and the repo tests laws with ScalaCheck. recalculateParallel(n) == recalculate() is exactly the shape of the law-based testing posture in docs/design, and Generators.scala is already there. Seven hand-built books cannot reach the irregular graph shapes where a wave-placement mistake would actually hide; a generator can:

property("GH-520: parallel recalculation equals sequential") {
  forAll(workbookGen) { wb => assertEquals(wb.recalculateParallel(8), wb.recalculate()) }
}

This is the single highest-value addition to the PR, and it would independently guard finding 1 as well.

Smaller test notes:

• assertSameResult checks five fields but skips iterationsUsed. RecalcResult derives CanEqual, so appending one assertEquals(parallel, sequential) closes the gap; keep the field-wise asserts first for readable failure messages.
• Race coverage is thin: the widest wave under test is 40 cells across 1,200 formulas, repeated three times. A cross-thread visibility bug would show up as flake, not failure. One grid with a few thousand cells per wave in the repeat test would give the assertions something to bite on.

Performance

• Static contiguous chunking has no work stealing. Uniform waves are fine, but a wave mixing =A1+1 with =SUMPRODUCT(...) over a large range strands one chunk while the other seven threads idle. A shared AtomicInteger cursor — or just submitting per-cell tasks and letting the pool queue balance — is a few lines and removes the failure mode entirely.
• 1.14x from 8 threads on the heavy book deserves one more look before it is attributed to Amdahl. 9,900 SUMs over 5,000 rows each is both heavy and embarrassingly parallel; if serial graph traversal were the whole story I would expect the eval portion to scale better than that. A competing explanation is that the range-read path is allocation-rate-bound (BigDecimal churn), making the bottleneck GC and memory bandwidth, neither of which scales with cores. A GC log or allocation profile at --parallel 1 vs 8 distinguishes the two, and if it is allocation that redirects where the next optimization should go.
• Peak heap is not reported. The headline result for this stack is 200k formulas at 512 MB; N threads each holding live evaluation garbage raises peak heap over sequential. Worth a number before pointing memory-constrained users at --parallel.

Docs

--parallel exists only in recalcHelp. --tables got lines in plugin/skills/xl-cli/SKILL.md and docs/STATUS.md; this deserves the same, in particular (a) the "silently ignored on iterate-declared books" rule and (b) the honest framing from the PR body. Left undocumented, --parallel 8 reads as "8x faster" to every user who finds it.

---

Nothing here blocks the concurrency design, which I believe is correct. The items I would want before merge are the dead ordered val (2), the CLI purity/duplication fix (6), and CLI coverage for the fallback NOTE. Finding 1 and the ScalaCheck law are the two that pay off over the long run.

[arcaputo3]

Follow-up audit and optimization commit 58fe3969 is now pushed to this PR.

This addresses the original review findings and the additional correctness issues found while reviewing the full #521 → #523 → #524 stack:

• updates every Recalc constructor/extractor for the new arity;
• resolves defined-name chains when classifying dynamic INDIRECT/OFFSET formulas;
• pins volatile clock capabilities independently and serializes custom clock access;
• cancels workers and shuts down the pool on interruption/failure;
• recalculates targeted dependents in one workbook-wide order, isolating cycles instead of abandoning healthy branches;
• makes aggregate memoization generation-scoped, single-flight, mode-aware, and safe for changing full-row/full-column bounds;
• stages both -o and -i writes before atomic publication, preserving destinations on partial failure.

The main additional performance work is:

• formula-only topology plus a symbolic range dependency index (no million-cell/full-column edge materialization);
• linear mutable BFS and stack-safe deterministic cycle traversal;
• streaming aggregate accumulators and safe repeated-range memoization;
• allocation-free Sheet.usedRange scanning;
• lazy bounded wave executors with atomic work stealing.

Representative local profiles:

• sparse targeted SUM(A:A) graph/index: ~1,008 ms / 656 MB → ~0.30 ms / negligible allocation;
• 9,900 formulas over the same bounded 5,000-row range: ~9.84 s / 26.8 GB caller-thread allocation → warmed ~75 ms / ~132 MB sequential;
• correctness-safe full-column shape: 5.08 s sequential → 2.53 / 1.40 / 1.01 s at 2 / 4 / 8 workers.

The remaining structural opportunity is still #522's range-node/interval-index end state: distinct rolling ranges can retain O(formulas × unique ranges) candidate scans, and full-column bounds still need snapshot-aware work.

Validation:

• repository-wide pre-commit formatting and WartRemover compile hooks passed;
• ./mill --no-server __.checkFormat;
• ./mill --no-server __.compile;
• ./mill --no-server __.test — 1,028/1,028 Mill tasks;
• native image built and installed; installed binary reports 0.19.1.

A final independent audit found no remaining P0/P1 correctness blocker.

[claude]

Review: Wave-parallel non-iterative recalculation (GH-520)

I read the full fix/522-range-edge-alloc...feat/520-parallel-eval diff (3,098 / 602 across 28 files). The wave-partition argument holds up: FIFO Kahn does emit longest-path depth classes contiguously, orderedMain is closed under predecessors (the dynamic bucket is dependent-closed), and prunedDeps has the cyclic core and blocked cells removed — so depthOf never silently drops a real precedent, and the intra-wave fold order matches the sequential order exactly. Sharing evalOne/foldResult verbatim between both paths is the right call, and ParallelRecalcSpec pins the equality on the shapes that would actually break it. Thread-safety also checks out: EvalMemo is per-eval-call (Evaluator.scala:951), Rng.system is ThreadLocalRandom-backed, the parser has no global mutable cache, and results(i) writes are published via Future.get's happens-before edge.

I also traced the AggregateMemo staleness question and could not find a hole: stripFormulaCaches puts every bucket cell into Formula(_, None, _) before the pass, so any range containing one is marked Uncacheable on first probe; range edges to formula cells are retained by fromWorkbookFormulaGraph, so a consumer always evaluates after its range's formulas; DataTable cells are excluded from the graph on both sides and never re-evaluated. Nice.

Findings below, most impactful first.

---

1. fromWorkbookFormulaGraph range lookup can be quadratic where fromWorkbookBounded was linear

DependencyGraph.scala:1304 — cellsFor resolves each range by scanning every formula ref on the target sheet:

refsBySheet.getOrElse(sheet, Vector.empty).iterator.filter(range.contains)

Cost is O(unique ranges × formula cells on that sheet). That is a big win on the shape the PR benchmarks (9,900 formulas sharing one SUM($A$1:$A$5000) → 1 unique range → 1 scan). It inverts on the equally common shape where each formula has its own small range: per-row totals =SUM(B5:M5) dragged down 50k rows give 50k unique ranges × 50k formula refs ≈ 2.5e9 contains probes, where the old range.intersect(bounds).cells did 50k × 12 ≈ 600k. Rolling averages, =SUM($A$1:A2) running totals, and per-row rollups all land here — and DependentRecalculation now calls this on every targeted recalc too.

The fix is the adaptive strategy already written in AggregateMemo.cacheable (Evaluator.scala:527): pick per range whichever side is smaller.

val formulaRefSet: Map[SheetName, Set[ARef]] = refsBySheet.view.mapValues(_.toSet).toMap
// in cellsFor:
val refs = refsBySheet.getOrElse(sheet, Vector.empty)
if range.cellCount <= refs.size then
  range.cells.iterator
    .filter(formulaRefSet.getOrElse(sheet, Set.empty).contains)
    .map(QualifiedRef(sheet, _)).toSet
else
  refs.iterator.filter(range.contains).map(QualifiedRef(sheet, _)).toSet

FormulaGraphPerfSpec only gates the shared-range direction ("constant driver ranges contribute no topology edges"). Worth adding a mirror gate: N formulas, N distinct small ranges, same sheet.

2. -o staging changes the destination file's permissions and identity

Main.scala:2886 — Files.createTempFile creates rw------- (0600) on POSIX; after Files.move(..., REPLACE_EXISTING) the destination inherits 0600 instead of the umask default, and any pre-existing mode / ACL / ownership / group on the target is discarded. xl -f in.xlsx -o /srv/reports/out.xlsx recalc used to produce a 0644 file the web server could read; now it does not. Related: -o pointing at a symlink now replaces the link rather than writing through it.

Suggest setting the temp's permissions to the process default before the move (probe the effective umask via a throwaway file in the same directory, or Files.setPosixFilePermissions), and when the target already exists, copying its PosixFileAttributes onto the temp first. Worth a test either way — this is a visible behavior change for -o, which the PR description frames as unchanged.

(The flip side is a genuine fix: -i with a read-only subcommand previously moved an empty temp onto the input. The Files.size(tmp) > 0L guard closes that. Good catch.)

3. An aborted parallel recalc still writes the output file with exit 0

WorkbookEvaluator.scala:587 — on interruption or worker failure, failPass turns every unfinished cell (including cells in the aborting wave that had already succeeded, whose results(i) are discarded) into FormulaError("Parallel recalculation interrupted" / "...worker failed"). That is a total RecalcResult, so WriteCommands.recalc proceeds to writeWorkbook and exits 0 unless --strict is set. A user who Ctrl-Cs gets a saved workbook where most formulas now cache an error string, silently overwriting a good one.

Consider threading the abort reason out of RecalcResult (or raising a dedicated RecalcAborted) and failing the command, so the staged temp is discarded by the outputComplete = false path just added. As-is, --parallel has a failure mode recalculate() does not.

4. AggregateMemo pays a full extra range scan per never-reused key

Evaluator.scala:527 — cacheable scans the whole range before the first fold, and getOrCompute always allocates an entry. When the hit rate is zero (the distinct-range shapes from #1, e.g. =SUM($A$1:A2) dragged down), that is a straight 2× cost on top of an already-quadratic fold, plus one retained TrieMap entry per distinct range for the generation.

Cheap guard: do not probe cacheability on first sight. Add a Seen state — first request computes and returns without caching, second request for the same key runs cacheable and fills. Single-use ranges then pay only a hash probe. The bypasses / fills counters already added make this easy to validate.

5. Blast radius is much wider than "opt-in --parallel"

The default recalculate() path also changes here: shared generation AggregateMemo, pinned calculation clock (NOW()/TODAY() are now fixed per recalc, previously per cell), fromWorkbookBounded → fromWorkbookFormulaGraph, workbook-aware defined-name dynamic analysis, a rewritten DependentRecalculation (global cross-sheet order + dynamic-closure cache stripping), firstCycleOf replacing Tarjan for cycle diagnostics (the reported cycle path changes shape), and -o staging. Several are improvements, but none are behind the flag. Worth saying so explicitly in the PR body / release notes so the default path gets the same scrutiny — a reader could reasonably assume recalculate() is untouched.

6. Smaller items

• pinnedCalculationClock (WorkbookEvaluator.scala:766): the gate.synchronized inside each lazy val initializer is redundant — Scala 3 lazy vals already initialize exactly once with safe publication. Harmless, but it reads as if the lazy val were not sufficient. (No deadlock risk: neither initializer touches the other.)
• MaxParallelism = 64 silently clamps. --parallel 10000 succeeds with no note, unlike the iterative case which does emit a NOTE. Either document the cap in recalcParallelOpt's help text or surface it in the summary. Also consider capping at Runtime.availableProcessors rather than a fixed 64 — on a 2-core CI container a 64-wide pool is pure contention.
• ParallelWaveCutoff = 16 is presented as tuned but no measurement is referenced; a one-line note on how it was chosen would help the next person who touches it.
• Worker-liveness assertions are cross-suite fragile. ParallelRecalcSpec filters Thread.getAllStackTraces for any thread named xl-recalc-worker-*; if munit runs suites concurrently and another spec's pool is still draining its 5s awaitTermination, this fails spuriously. A per-run id in the thread name (or a scoped registry) would make it hermetic.
• FormulaGraphPerfSpec: assert(anchored eq relative) asserts reference identity on a cache internal — it breaks the moment memoizedCells returns anything defensive. assertEquals plus a separate probe on expansion count would be sturdier.
• DependentRecalculation.recalculateQualifiedInOrder: changed += index fires for every processed ref even when recalculateOne returns the sheet unchanged (pinned caches), so those sheets are put back and the workbook marked modified. Conversely, stripFormulaCaches applied to a sheet with no recalculated refs is silently dropped, since only changed indices are committed. Both minor, but the asymmetry is easy to trip over later.
• PR body honesty — the "1.14× on the heavy book, ≈1× on chains" table is exactly the right way to present this, and the docs say the same. Appreciated; that is rarer than it should be.

---

Nothing here blocks the wave-parallel design itself, which looks correct and is well gated. #1 and #2 are the ones worth resolving (or consciously accepting) before merge, since both are regressions on the default path rather than the new opt-in one.

[claude]

Review: wave-parallel non-iterative recalculation (GH-520) — part 1/2

I read the full diff of both commits (9617c8d parallel eval, 650b53b harden/accelerate) plus the surrounding code in WorkbookEvaluator, Evaluator, DependencyGraph, DependentRecalculation, StructuralEditor and the CLI staging path.

This is very strong work, and the equivalence argument holds up under scrutiny. I verified the two non-obvious preconditions behind the wave lemma: (a) evalOrder really is level-monotone in longest-path depth, because kahnOrder uses a FIFO queue seeded with the whole in-degree-0 layer — so folding wave-by-wave reproduces the sequential error order, not just its contents; and (b) every precedent of an orderedMain node is itself in orderedMain, because bucket is closed under transitive dependents and prunedDeps values have removed deleted — which is what makes the depthOf.get(p).foreach(...) "missing parent contributes nothing" shortcut safe rather than accidentally safe. Calling the FIFO property load-bearing in the kahnOrder scaladoc is exactly the right place to have put it.

Other things done right: sharing evalOne/foldResult verbatim so the paths cannot drift; refusing to offer seeded-Rng and IterativeCalc variants instead of pretending; Future.get's happens-before edge plus a single-assignment atomic cursor as the publication story for the results array; honest benchmarks with the ~1x chain numbers left in. Good incidental fixes too — detectCrossSheetCycles is no longer stack-recursive, firstCycleOf stops inventing a closing edge from a Kahn remainder tail, and evaluateAll/evaluateRange now thread the workbook instead of handing cross-sheet readers a stale sheet.

Findings, roughly by severity.

1. Wave workers get the default thread stack size — a real equivalence hole

WorkbookEvaluator.scala:494-502 creates workers with new Thread(runnable, name). On HotSpot the main thread's stack comes from the process limit (commonly 8 MB on Linux) while created threads get -Xss (commonly 1 MB). EvaluatorImpl.eval recurses over the AST, so a deeply nested formula (=1+1+1+... with thousands of terms, a deep LET/IF nest) can evaluate fine sequentially and StackOverflowError inside a worker. That breaks two documented promises at once:

• element-for-element equality with recalculate();
• totality — StackOverflowError is not NonFatal, so evalOne's guard misses it, and evalParallelWaves deliberately rethrows a fatal ExecutionException cause (WorkbookEvaluator.scala:572-574), unwinding the whole recalculation.

Cheap fix: give the factory an explicit stack size, e.g. new Thread(null, runnable, name, RecalcWorkerStackBytes). If you would rather not pick a number, at minimum add a pathologically-nested-formula test so the behaviour is pinned either way. This is the one item I would like resolved before merge.

2. -o output files now inherit createTempFile permissions

Main.stagedOutput (Main.scala:2878-2896) allocates the staging path with Files.createTempFile, which on POSIX creates rw------- (0600), and Files.move preserves the source's attributes. So xl -f in.xlsx -o out.xlsx put A1 1 now produces a 0600 out.xlsx where it previously produced a umask-derived 0644 — anything reading those outputs as another user or through a group-readable share breaks. Two related consequences: an existing -o target has its mode/ACLs replaced rather than preserved, and an -o target that is a symlink now has the link replaced instead of being written through.

Suggest setting the staging file's permissions before the move — to 0666 & ~umask, or to the existing target's PosixFilePermissions when it exists. InPlaceSpec covers the staging/commit/discard matrix thoroughly but nothing asserts the resulting mode.

3. QualifiedDependencyIndex.transitiveDependents is O(visited x ranges-on-sheet)

For every dequeued ref it linearly scans every distinct range on the sheet:

rangeDependents.getOrElse(ref.sheet, Vector.empty).foreach { entry =>
  if entry.range.contains(ref.ref) then entry.dependents.foreach(enqueue)
}

The symbolic index is a clear memory win over expanding A:A to a million entries, and the motivating shape (9,900 formulas sharing one SUM($A$1:$A$5000)) has exactly 1 unique range. The adversarial shape is dragged =SUM(B$1:B2) down 200k rows: ~200k distinct ranges, and a dirty cone touching many of them goes quadratic. That code is on dirtyCone (every mutating CLI verb), StructuralEditor.staleCaches (every row/col insert/delete) and both recalculateDependents overloads — so a regression shows up on ordinary put/insert-rows, not just recalc. Worth measuring before merge; bucketing rangeDependents by row band (or an interval tree on the row axis) keeps the memory win with a bounded probe count. FormulaGraphPerfSpec pins storage but not lookup cost.

4. AggregateMemo.cacheable leans entirely on graph-edge completeness

Evaluator.scala:520-534 treats a range as cacheable when it contains no Formula(_, None, _) — a formula carrying a previous generation's cache counts as safe. That is sound only because the graph guarantees every formula in an explicit range is evaluated before its aggregate consumer. Where an edge is genuinely missing, the memo is strictly worse than no memo: it freezes the pre-evaluation fold and serves it to later readers that would otherwise have recomputed. The missing-edge classes are the ones you document elsewhere — a defined name whose refersTo the parser rejects (GH-468 blind names), and a formula whose own text fails to parse (fromWorkbookFormulaGraph gives it Set.empty deps). Both paths share generationEvaluator so sequential and parallel still agree with each other; the silent divergence is against pre-PR sequential behaviour.

A tightening that makes the guard independent of edge completeness: require that no Formula cell at all remains in the range, since foldResult collapses each evaluated formula cell to a bare CellValue in the temp sheets — pending and failed cells then correctly bypass. The case needing an explicit exemption is a pinned cache (SheetEvaluator.pinnedCache: external-workbook and FormulaKind.DataTable records), which stays a Formula forever and genuinely cannot change; without that exemption any range containing a data table becomes permanently uncacheable.

Also worth stating explicitly at AggregateMemoEntry.lookupOrCompute (Evaluator.scala:571-587): compute runs inside the monitor. Fine today because a cacheable raw-range fold only reads values and never re-enters the memo, but it is a lock held across arbitrary formula evaluation — the no-reentrancy assumption should be written down rather than inferred, since a future nested-aggregate path could deadlock on two keys acquired in opposite orders.

[claude]

Review: GH-520 — part 2/2 (lower-severity findings, scope, tests)

5. recalculateQualifiedInOrder marks sheets modified unconditionally

val updated = recalculateOne(sheets(index), q.ref, Some(currentWorkbook), clock)
changed += index

changed is set for every ref whose sheet resolves, even when recalculateOne returned the sheet unchanged (pinned cache, non-formula cell, or a recomputed value identical to the existing one). Each Workbook.put then calls markSheetModified, and per the reasoning in recalculateImpl that forces the writer to regenerate the worksheet XML and drop unparsed parts (pivot tables, slicers, ctrlProps) that only survive byte-for-byte preservation. Not a regression — the pre-PR code put unconditionally too — but the rewrite puts the changed set right there, and if (updated ne sheets(index)) changed += index is a one-line reference comparison that brings targeted recalc in line with what recalculateImpl is careful about.

6. Sequential recalculate() semantics changed: the clock is now pinned

pinnedCalculationClock is applied on every path, not just the parallel one, so a workbook with many NOW() cells now gets one timestamp for the whole pass instead of one per cell. This is the right semantics and is necessary for wave equality, but it is a user-visible behaviour change to the existing sequential API. Worth a line in docs/STATUS.md / release notes, not only in the scaladoc.

7. DependencyGraph.boundedCells is now dead

The TExpr.Call branch was its last caller and now uses the cached boundRange. boundedCells (DependencyGraph.scala:71-74) is unreferenced — please delete it so it cannot drift out of sync with boundRange.

8. --parallel is silently clamped to 64

MaxParallelism = 64 clamps with no feedback, while the iterative fallback goes out of its way to say so on stdout. --parallel 10000 is nonsense input, granted, but given the otherwise-consistent "never silent" posture, either surface a note or document the cap in the recalc help text and docs/reference/cli.md.

9. Nit: toUpperCase allocated per candidate name

DependencyGraph.scala:311:

if names.exists(n => expression.toUpperCase(java.util.Locale.ROOT).contains(n)) =>

The explicit lambda re-uppercases expression once per dynamic-function name, for every formula cell on the sheet. Hoisting val upper = expression.toUpperCase(Locale.ROOT) out of the guard is free and matches the allocation discipline the rest of this commit enforces. (The workbook-level candidateTokens.exists(expression.toUpperCase(...).contains) is fine — eta-expansion evaluates the prefix once.)

10. Nit: evalWaves pays the depth build even when it goes fully sequential

When no wave reaches ParallelWaveCutoff, evalWaves has already built a full HashMap[QualifiedRef, Int] plus one ArrayBuffer per depth before falling back to evalPass(order, ...). That is the likely explanation for the 50k CHOOSE-chain going 4.6 s -> 4.85 s. Not worth contorting the code for, but the PR body could say why the chain case regresses rather than just that it does.

11. Scope

The PR is framed as GH-520, but 650b53b also lands a DependencyGraph rewrite (+581/-234), the new symbolic QualifiedDependencyIndex with three call-site migrations, a Sheet.usedRange rewrite, aggregate accumulator-streaming in FunctionSpecsAggregate, and the -o atomic-staging CLI change. Several are independently reviewable and independently bisectable; the -o staging in particular is a user-visible CLI contract change landing inside a recalc-performance commit. Nothing needs reverting, but splitting it out — or at minimum calling it out in the PR body and docs/reference/cli.md — would make this much easier to reason about six months from now.

12. Test coverage

Genuinely thorough: equality on wide grids, deep chains, error books, cyclic cores with blocked dependents, INDIRECT buckets; error order; degenerate parallelism; a ScalaCheck law; both interruption paths; pinned-clock laziness; and the memo's mode/anchor/bounds keying. AggregateMemoSpec's "uncached formula ranges bypass; cached formula ranges may reuse" and "parallel readers single-flight one eligible range fold" are exactly the right tests to have written. Two gaps and one robustness nit:

• Nothing exercises deep AST nesting under --parallel (section 1).
• Nothing pins the resulting file mode for -o (section 2), nor lookup cost for the symbolic index (section 3).
• ParallelRecalcSpec:321-324 and :337-340 assert on a global thread scan — Thread.getAllStackTraces filtered by the xl-recalc-worker- prefix. ParallelWorkerIds is a process-global counter with no per-pool scoping, so if any other suite runs recalculateParallel concurrently (Mill does run suites in parallel) this asserts on someone else's live workers and flakes. Scoping the name — xl-recalc-<poolId>-worker-N, filtered to this pool — would make the assertion mean what it says.

---

I reviewed statically and did not run the suite locally, so I am taking the "full ./mill __.test green" claim at face value; CI is the arbiter there.

Overall: the design, the documentation density, and the equivalence gating are all excellent, and the honesty about Amdahl in the PR body is the right call. Section 1 is the only item I would block on; section 2 should at least get a recorded decision, since it silently changes output-file permissions.

[claude]

Review: wave-parallel non-iterative recalculation (GH-520) — part 1/2

Scope note: fix/522-range-edge-alloc shares no merge base with the PR head, so GitHub renders the whole stack (#521 staged writes, #523 range-edge allocation, #520 waves). I reviewed all of it.

The equivalence discipline here is exemplary. Extracting evalOne/foldResult and sharing them verbatim between evalPass and evalWaves is the right structural answer to "the two paths cannot diverge", and ParallelRecalcSpec pins the contract on exactly the shapes that would break it (error order, cyclic cores with blocked dependents, the INDIRECT bucket, degenerate parallelism). The FIFO-Kahn to level-monotone argument is correct — a FIFO Kahn order is sorted by longest-path depth, so waves.flatten == orderedMain and the fold order is preserved — and I like that kahnOrder now documents that this is load-bearing.

I also traced the AggregateMemo safety argument and it holds: every formula cell inside an aggregated range is a graph precedent of its consumer, so a fill can never observe a pre-refresh cache; and since cacheable excludes uncached formulas, compute never re-enters the evaluator while holding the entry monitor, so there is no lock-ordering hazard between entries. I checked Sheet/Workbook for lazy or var state (none), EvalMemo confinement (fresh per Call node, since EvaluatorImpl.memoOpt is None), Rng.system (ThreadLocalRandom), and the parser and function specs for shared mutable statics (none). The concurrency story is sound.

Findings 1-4 below are the ones I would want addressed before merge; 5-8 in part 2 are follow-ups.

1. Worker threads get the default stack size — recalculateParallel can throw where recalculate() returns

In WorkbookEvaluator.evalParallelWaves the factory is new Thread(runnable, s"xl-recalc-worker-..."). The evaluator recurses over the formula AST and through cross-sheet chains; this codebase went to real trouble to make Tarjan, Kahn and the cycle walk iterative precisely because "recalculate is documented total" on deep books. But the main thread typically has an 8 MB stack (ulimit -s) while a JVM-spawned thread gets -Xss (commonly 512 KB to 1 MB). A deeply nested book that recalculates fine sequentially can therefore StackOverflowError only under --parallel. StackOverflowError is a VirtualMachineError, so it is not NonFatal: it escapes the evalOne guard, arrives as ExecutionException, and hits case Some(fatal) => stopNow(...); throw fatal — the parallel entry point throws where the sequential one returns a RecalcResult.

Suggest new Thread(null, runnable, name, StackSizeBytes) with a generous size, so the parallel path is no less total than the sequential one, plus a deep-chain regression test under --parallel.

2. -o output files now get temp-file permissions (0600), and replace the destination permissions

Main.stagedOutput uses Files.createTempFile(directory, prefix, suffix), whose POSIX initial permissions are owner-only (rw-------) by design; the subsequent Files.move carries those onto the destination. So xl -f in.xlsx -o out.xlsx put A1 1 now yields 0600 where it previously yielded a umask-derived 0644, and an existing 0644 / group-writable / ACL-bearing destination is silently narrowed. This affects every -o mutation and the rasterized exports at ReadCommands.scala:190. -i already had this; the PR extends it to -o. Suggest copying the destination PosixFilePermissions onto the staging file before the move, falling back to umask when the destination does not exist.

3. -o staging failures bypass the friendly error renderer

runStagedOutput acquires the staging path outside the execute(...).attempt in runResult, and Main has no top-level handleErrorWith (only atomicMoveOrFallback). A createTempFile failure therefore escapes to CommandIOApp as an unhandled exception with a stack trace, where -o missing-dir/out.xlsx previously produced a one-line renderErrorMessage. Two regressions: a non-existent output directory now stack-traces, and -o into a read-only directory holding a writable existing file now fails outright (staging needs directory write permission). The first is worth fixing (.attempt around acquisition, or pre-flight the parent); the second is probably acceptable but should be documented. Neither is tested. Related and smaller: -o onto a symlink now replaces the link rather than writing through it.

4. Targeted recalc can strip a cache it never recomputes

DependentRecalculation.recalculateQualifiedInOrder strips dynamicClosure -- skipped, but the scheduled order is toRecalc - skipped, and toRecalc explicitly removes the caller modified seeds: ((modifiedDependents ++ dynamicClosure) -- qualifiedRefs) ++ dynamicQualified.

A modified seed that is a static dependent of a dynamic cell is in dynamicClosure but not dynamicQualified, so it is stripped and never scheduled. Concretely: A1 = INDIRECT("C1"), the user rewrites B1 to =A1+1 and calls recalculateDependents(sheet, Set(B1)) — B1 lands in the output with cachedValue = None. Worse, whether the strip survives depends on the changed bitset: it only reaches the result if some other cell on the same sheet was recalculated. The sheet-level variant has the same shape. Suggest intersecting the strip set with the refs actually scheduled, so stripping and evaluation are the same set by construction.

[claude]

Review (GH-520) — part 2/2: follow-ups, tests, verdict

5. dynamicCells(workbook) re-resolves every defined name once per sheet

The memo key is (lookupFrom, fallbackSheet, upperName), so a workbook-scoped name — the overwhelming majority — is looked up and its refersTo re-parsed once per sheet. On a 50-sheet, 500-name model that is roughly 25k lookupDefinedName scans and 25k parses before a single cell formula is examined. Only names carrying a localSheetId genuinely need per-sheet resolution; the global ones could be classified once and the shadowing pass restricted to sheets that actually define an override. The correctness is right (the multi-hop shadowing test is a nice catch) — it is the constant factor I would tighten.

6. Targeted recalculation now pays whole-workbook analysis three times over

recalculateDependents (both variants) now builds fromWorkbookFormulaGraph and fromWorkbookDependencyIndex and calls dynamicCells(workbook) — three passes that each re-parse every formula in the book. The sheet-level variant pays that for an API returning a single Sheet, and SheetEvaluator.dynamicCellsFor does wb.put(sheet) plus a full-workbook dynamicCells on every evaluateWithDependencyCheck. Since this PR is fundamentally about recalculation cost, it is worth confirming the cheap targeted path did not just become as expensive as the full one; a RecalcPerfSpec-style gate on recalculateDependents would make it visible. At minimum, dynamicCellsFor could short-circuit to the sheet-only scan when definedNames.isEmpty — the common case, and it restores the old cost exactly.

7. IO.delay around the recalculation blocks an uncancelable compute fiber

Deferring the pure work into IO.delay(recalcHonoringCalcPr(wb, parallel)) is a good fix — constructing the IO no longer burns CPU. But the parallel path blocks the calling thread in Future.get(), and IO.delay is uncancelable, so the careful interruption handling in evalParallelWaves (cancel outstanding, restore interrupt, failPass the remainder) can only be reached by an external Thread.interrupt, never by CE cancellation — which is exactly the SIGTERM path GH-519/#521 is about. IO.interruptible(...) would wire the two together.

8. Smaller items

• MaxParallelism = 64 clamps silently: --parallel 10000 passes validation and is quietly reduced. Either document the cap in the flag help and cli.md or reject above it; the spec covers "degrades safely" but not the clamp.
• Stale comments say "stderr": Command.scala and the recalcParallelOpt comment both claim the iterative fallback is a stderr note, but printRunResult uses IO.println and the test asserts it is in the summary. cli.md gets it right.
• pinnedCalculationClock gate is redundant: a Scala 3 lazy val already guarantees single initialization with a happens-before edge for readers, so the extra monitor is a second mechanism for the same invariant, and the comment implies the lazy val alone would not suffice.
• AggregateMemo.cacheable double-scans a cold range (eligibility walk, then the fold). Amortized away when many readers share a range, but a book of many distinct one-shot ranges pays 2x. Computing optimistically and discarding on the first uncached formula would avoid it.
• firstCycleOf sorts unconditionally: detectCycles runs it on every graph including acyclic ones, so the acyclic path pays O(V log V + E log E) where Tarjan was O(V + E). The canonical diagnostic is worth having and the allocation savings probably dominate — worth confirming rather than assuming.
• Public-surface changes not in the summary: this PR also changes sequential recalculate() — the clock is pinned per generation (so NOW() is uniform across cells where it previously ticked per cell) and the shared AggregateMemo is on by default. Both look correct, and the pinning is the more Excel-faithful semantics, but someone auditing "opt-in parallel recalc" would not expect the default path to move. Worth calling out in the release notes. CLAUDE.md Essential APIs also does not mention recalculateParallel, and its test count is stale.

Tests

Strong, and the property tests are well chosen — GH-518: optimized Kahn preserves the immutable FIFO order exactly pins the one invariant the wave partition silently depends on, and assertActualCycle turns "is this an edge path" into a checkable law. Gaps I would add: -o staging against a missing or unwritable destination directory (3) and destination permissions (2); a deep-recursion book under --parallel (1); the MaxParallelism clamp as an observable property rather than only "does not crash"; and the strip/schedule mismatch in (4).

Process note: FormulaGraphPerfSpec and RecalcPerfSpec assert wall-clock budgets inside the ordinary unit suite. The budgets are generous, but timing gates on shared runners are a classic source of intermittent red — if they start flaking, consider tagging them out of the default __.test run.

Verdict

The design is right, the equivalence argument is load-bearing rather than decorative, and I appreciate that the description is honest about Amdahl instead of quoting a headline number. Findings 1-4 before merge; the rest are follow-ups.

[claude]

Review: wave-parallel recalculation (GH-520) — part 1/2

Reviewed the full 5-commit stack as it lands on main (#521 + #523 + this). CI is green.

The equivalence argument holds up. I worked through the parts that carry it:

• Error-order parity rests on FIFO-Kahn being level-monotone (all depth-d nodes before any depth-d+1). That is true: a node is enqueued only after its last parent is dequeued, so the queue never holds depth d+2 while depth d remains. Concatenating waves in index order therefore reproduces orderedMain exactly. Good call recording it as a load-bearing invariant in the kahnOrder docstring (DependencyGraph.scala:1080) instead of leaving it implicit.
• orderedMain filtering cannot break it, because bucket is closed under dependents: no non-bucket node can have a bucket parent, so depths computed over prunedDeps equal the full-graph levels.
• depthOf.get(p) skipping absent parents (WorkbookEvaluator.scala:471) is safe — a missing parent is a cyclic-core or bucket member, and neither is written during the main pass.
• Publication of results(i) across the Future.get happens-before edge is correct; the shared AtomicInteger cursor keeps slot assignment single-owner.
• Sheet has no lazy vals or caches and Rng.system is ThreadLocalRandom, so the shared snapshot reads really are thread-safe. EvalMemo (identity-keyed, not thread-safe) correctly stays per-cell and never reaches a worker.

Sharing evalOne/foldResult verbatim between both paths is the right structural choice.

Two findings I would hold the merge on, then follow-ups in part 2.

---

1. QualifiedDependencyIndex.transitiveDependents reintroduces the 49.5M work it removes — at traversal time

DependencyGraph.scala:816-832:

while pending.nonEmpty do
  val ref = pending.removeHead()
  pointDependents.getOrElse(ref, Set.empty).foreach(enqueue)
  rangeDependents.getOrElse(ref.sheet, Vector.empty).foreach { entry =>
    if entry.range.contains(ref.ref) then entry.dependents.foreach(enqueue)
  }

Every visited ref rescans all range entries for its sheet, and every matching entry re-enqueues its entire dependent set. On the motivating shape from the PR body — 9,900 formulas over SUM($A$1:$A$5000) — if the A-column cells are formulas rather than constants, 5,000 get visited and each re-fires the same entry: 5,000 x 9,900 = 49.5M enqueue calls. That is precisely the figure the docstring says the symbolic index eliminates. The saving is real only when the covered cells are constants and so never enter the cone.

Worse in the drag-down window shape (=SUM($B$2:B5), =SUM($B$2:B6), ...): R distinct ranges, cone of V, and the entry.range.contains scan alone is O(V x R) — 2.5e9 containment checks at 50k formulas.

Both collapse with a fired-set, and it is exactly equivalent because enqueue is idempotent (visited.add): once an entry has fired, all its dependents are already in visited, so firing again can add nothing.

val fired = scala.collection.mutable.HashSet.empty[Int]   // or a per-sheet BitSet
val entries = rangeDependents.getOrElse(ref.sheet, Vector.empty)
var i = 0
while i < entries.length do
  if !fired.contains(i) && entries(i).range.contains(ref.ref) then
    fired += i
    entries(i).dependents.foreach(enqueue)
  i += 1

Related test gap: the FormulaGraphPerfSpec case "symbolic index stores a shared range once" asserts entries.size == 1 with constant cells in the range — the exact shape that hides this. A gate with (a) formulas inside the shared range and (b) N distinct overlapping ranges would catch it.

2. -o output files change permissions (0644 to 0600) and replace the destination identity

Main.scala:2887-2917 routes every -o write through Files.createTempFile + Files.move. Per the JDK contract, createTempFile on the default POSIX provider creates the file with permissions permitting access only to the current user, and a same-directory move is a rename carrying the source attributes across. So:

• An -o output that used to land umask-derived (typically rw-r--r--) now lands rw-------. Any workflow writing a report into a shared directory for another user or service to read breaks silently.
• Replacing an existing target discards its permissions, ownership, and ACLs.
• If the target is a symlink, REPLACE_EXISTING replaces the link with a regular file rather than writing through it.

-i already had this, but -o is new here, and nothing in docs/reference/cli.md or InPlaceSpec covers it. Minimum: copy the destination attributes onto the staging file before the move when the destination exists (getPosixFilePermissions then setPosixFilePermissions, guarded on provider support); otherwise widen the temp to the process umask. A test asserting the committed mode matches a direct write would pin it.

Otherwise the staging change is a genuine improvement — the Files.size(tmp) > 0L guard in particular fixes a latent footgun where -i on a command that writes nothing would have moved an empty temp onto the input.

[claude]

Review: wave-parallel recalculation (GH-520) — part 2/2

3. Read-only commands with -o now fail where they used to work

runStagedOutput allocates the staging file before dispatch, so -o creates a file in the target parent directory even for commands that never write. xl -f data.xlsx -o /nonexistent/dir/out.xlsx view A1:B2 previously printed the view (the flag was inert); now it raises NoSuchFileException. It also creates-and-deletes a file on every read-only -o invocation. Staging lazily — on first write, or gated on the command being a mutation — avoids both.

4. recalculate() own semantics changed; the PR frames everything as opt-in

• Pinned clock. pinnedCalculationClock now wraps all non-iterative recalculation (WorkbookEvaluator.scala:705), where the old code passed the caller clock straight into evalPass. NOW() in two cells of one recalculate() previously got two timestamps; now they get one. This matches Excel and is the better behavior — but it is a user-visible change to a published method, and I do not see it in the PR body, docs/STATUS.md, or the method scaladoc.
• Aggregate memo. recalculationInstance is used by the sequential path too, so any residual unsoundness in the memo (the blind-defined-name class the recalculateParallel docstring names; DataTable cells excluded from formulas at DependencyGraph.scala:760) now changes recalculate() results, not only the opt-in path.

The memo safety argument itself checks out: cacheable requires no Formula(_, None, _) in the range, bucketIter members have caches stripped so any range containing one bypasses, cyclic-core members are frozen for the pass, and the full-column key correctly uses effectiveRange. I confirmed evalAggregateNode iterates range.cells with no usedRange constraint, so the TypedNode path keying by the raw range is fine. The invariant "every formula cell in an explicit range is evaluated before its aggregate consumer" is doing a lot of work and only holds because fromWorkbookFormulaGraph expands ranges against refsBySheet unbounded — worth a comment where it is relied on.

5. Advisory goes to stdout, not stderr

Main.scala:98 ("with a stderr note") and the PR body ("stderr NOTE") disagree with WriteCommands.scala:1345, which appends the advisory into the rendered summary — stdout. docs/reference/cli.md gets it right. Pick one; scripts grepping stderr for warnings will miss it as written.

---

Performance

AggregateMemo.cacheable is pure overhead when ranges are never shared. Evaluator.scala:530-542 costs O(min(rangeSize, sheetSize)) per distinct key. On the shared-range shape that amortizes to nothing. On the drag-down running-total shape (every key distinct) it roughly doubles per-formula aggregate cost — 50k formulas x ~25k average range = 1.25e9 extra map probes, added to the sequential path. WorkbookEvaluator already knows which cells remain unevaluated; threading that set (or just a per-sheet count of remaining uncached formulas, short-circuiting to cacheable = true at zero) makes the check ~O(1) and strictly more precise.

The wave fold is the serial bottleneck, and it is fixable. WorkbookEvaluator.scala:568-570 folds each wave one cell at a time: one Sheet.put (persistent map update), one Vector.updated, one nested-Map update. For cheap formulas that is plausibly most of the per-cell cost — a good candidate explanation for the 1.14x ceiling surviving 8 threads. Grouping a wave successes by sheet index and bulk-applying once per sheet per wave is element-for-element equivalent (each ref appears at most once per wave, so final contents are identical) provided error appends stay per-cell in wave order. Highest-leverage follow-up.

Memory. Array.fill(wave.length)(None) plus boxed Eithers retains every result before folding; a book that is one wide wave (200k independent formulas) holds 200k results live at peak. Worth a docs line given how much of the value story is the 512 MB heap row.

--parallel bounds. MaxParallelism = 64 clamps silently and is not in --help; nothing relates it to Runtime.availableProcessors. --parallel 32 on a 2-core container spawns 32 threads that also contend with the CE compute pool, since recalculateParallel runs inside IO.delay and starts its own raw pool. Either clamp to cores by default or say so in the flag help.

Nits

• evalParallelWaves is ~110 lines carrying pool lifecycle, an abort state machine, and the fold, with six vars and a manual java.util.ArrayList plus index loops. (0 until workerCount).map(_ => pool.submit(task)).toVector then futures.foreach(_.get()) removes three index loops at identical allocation cost. The hardStopped/finally interaction is correct but takes a careful read to confirm; splitting pool lifecycle out from the wave loop would help.
• pinnedCalculationClock takes both the lazy-val init lock and gate (WorkbookEvaluator.scala:773). Deadlock-free (single gate, always acquired last) but redundant — Scala 3 lazy vals already publish safely. Worth a word on why, or dropping it.
• ParallelWorkerIds is a process-global counter that never resets, so thread names drift upward across recalcs. Cosmetic.
• On abort, failPass(waves.drop(waveIndex)) overwrites cells in the failing wave that had already succeeded with the abort error. Defensible, but it deserves a line in the RecalcResult scaladoc — callers cannot distinguish "this cell failed" from "the run was cancelled".
• A fresh newFixedThreadPool per recalculateParallel call: fine for the CLI single shot, less so for a library caller in a loop.

Test coverage

Strong. ParallelRecalcSpec covers wide grids, sub-cutoff chains, error order, cyclic cores with blocked dependents, the INDIRECT bucket, repeat determinism, degenerate parallelism, multi-hop dynamic names, clock pinning with a concurrency-detecting GuardClock, and both interruption paths with a live-thread assertion. The AggregateMemoSpec case "uncached formula ranges bypass; cached formula ranges may reuse" is the right negative test.

Gaps I would add:

1. The symbolic-index shape from finding 1 — formulas inside a shared range, and N distinct overlapping ranges.
2. -o committed-file permissions vs. a direct write (finding 2).
3. Read-only -o with a nonexistent or unwritable parent directory (finding 3).
4. A NOW()-in-two-cells assertion on plain recalculate(clock), pinning finding 4 deliberately rather than incidentally.

The honesty of the benchmark table and the explicit "NOT a headline speedup" framing are the right call, and the reasons the ceiling sits where it does are correctly diagnosed. Findings 1 and 2 are the ones I would hold the merge on; the rest are follow-ups.