feat(cats-effect): make the streaming writer's spill directory injectable

[arcaputo3]

Closes #514.

Problem

The two-pass streaming writers (writeStreamWithAutoDetect, writeStreamsSeqWithAutoDetect) buy their up-front <dimension> by spilling the worksheet body to a scratch file — one per sheet — and that file always landed in the JVM's java.io.tmpdir with no way for a caller to move it.

Two consequences, one for users and one for the test suite:

• Callers can't place the spill. Containers routinely give /tmp a small tmpfs or mount it read-only, and a large write wants its scratch on the same fast volume as its output. POI solved the same problem with TempFileCreationStrategy.
• The spill was unobservable in isolation. test(cats-effect): de-flake the writer's temp-file cleanup assertions #513 de-flaked the cleanup tests, but they could still only watch a directory several worker JVMs share, settling for up to 2s against foreign traffic.

API

val excel = ExcelIO.instance[IO].withSpillDir(fastVolume)  // must already exist
rows.through(excel.writeStreamWithAutoDetect(out, "Data")).compile.drain

A missing or unwritable directory fails at acquire with a message naming the setting, so it isn't mistaken for a problem with the workbook being written.

Three shape decisions

spillDir is an overridable member, not a constructor parameter. A parameter — even a defaulted one — changes the erased constructor from (Function1, Async) to (Function1, Option, Async), breaking consumers compiled against an earlier release. An auxiliary constructor doesn't rescue it either: the context bound makes it (Function1, Async, Async), which I confirmed with javap before backing it out. As shipped, the constructor is byte-identical to before and spillDir() is purely additive. (No MiMa in CI, so that's a manual guarantee — see follow-ups.)

Not a WriterConfig field, even though the config is already threaded to both spill sites and would have been the smaller diff. WriterConfig describes the document's shape — SST policy, compression, XML backend, escaping — and is also consumed by the in-memory writer, which never spills. A filesystem scratch location is the interpreter's business, not the format's.

Per-instance rather than per-call. Instances are cheap, so a call site that needs its own directory builds one and leaves the rest of the program on the default — one mechanism covering both.

Two bugs found on the way

• Partial acquire leaked. The multi-sheet writer takes one spill per sheet inside a single bracket acquire, and bracket's release never runs for a failed acquire — so a spill that couldn't be created partway through stranded every file already taken. Pre-existing, but this PR is what makes it reachable: a broken java.io.tmpdir fails on sheet 1 with nothing to clean up, whereas a caller-supplied directory adds ENOSPC, quota and permission failures that land on sheet 3 of 5, in the user's own directory. Acquire now unwinds what it took, suppressing any delete failure onto the original cause rather than replacing it.
• The failure message could name the wrong directory. createSpillFile read the overridable spillDir twice — once to create, once to build the message. Snapshotted once, with the arity now stated in the scaladoc.

What this buys the tests

The cleanup assertions now point the writer at a directory inside the test's own fixture, so they are exact: no name filter, no settle loop, no residual flake window. The positive control survives in sharper form — with an isolated directory, "no leftovers" would also hold if the writer ignored the setting, so each test proves the spill appeared there first.

One test deliberately still watches java.io.tmpdir, pinning that the default is unchanged. It's positive-only: a foreign spill can satisfy it, never break it.

Verification

Each fault was injected into production code, observed, then reverted:

injected fault	result
bracket release stubbed to a no-op	both single-sheet tests fail naming the leaked file, in 0.006s (was 2s of polling)
createSpillFile ignores its argument	4 of 5 tests fail — the writer must actually honor the setting
acquire unwind removed	the partial-acquire test fails naming both orphans (xl-stream-1-*.xml, xl-stream-2-*.xml)
withSpillDir rebuilt from a fresh instance	the warning-handler test fails — pinning why it copies warningHandler

Release gates, all green: ./mill __.test, ScalafmtModule/checkFormatAll, scripts/test-examples.sh, scripts/verify-skill-snippets.sh --local.

Also in this PR

The two hygiene items filed on #514: sibling fixtures move off the xl-stream- namespace (xl-fixture-sst- etc.) so the spill pattern is unambiguous, and remapWorksheetEntry writes into the fixture directory instead of stranding a file in the shared one when a run ends abruptly.

Docs: the performance guide gains a "where the scratch file goes" note under row-stream writes, including that redirecting the spill makes the directory's permissions the caller's business (the file itself is rw------- either way). Test counts to 5,455 / xl-cats-effect 149.

Follow-ups filed

• XlsxWriter.writeToBytes spills to java.io.tmpdir with no lever #516 — XlsxWriter.writeToBytes has the same failure with no lever (pure module, no interpreter to carry the setting).
• CLI: no way to redirect the streaming spill directory #517 — a CLI --spill-dir. The CLI's global options thread through many mapN combinators and an 8-parameter run, so it doesn't belong here, but xl import --stream pushes real volume through /tmp.

🤖 Generated with Claude Code

[claude]

Review: injectable spill directory

Read the diff plus the surrounding ExcelIO write paths and the test fixture. This is a well-scoped change, and the PR body does a lot of the reviewer's work for me — the three shape decisions are each the right call and each justified with the actual constraint rather than a preference:

• Overridable member over constructor param is correct, and the javap check on the auxiliary-constructor fallback is exactly the diligence this deserves. Worth noting the repo has no MiMa in CI, so that guarantee is currently manual — if binary compatibility is a standing promise for the published modules, a MiMa check would make it enforced rather than remembered.
• Not a WriterConfig field. Agreed, and the reasoning generalizes: WriterConfig is consumed by the in-memory writer too, which never spills, so the field would have been dead for half its consumers.
• Failing at acquire rather than validating in withSpillDir keeps the builder free of I/O, and the error message naming the setting is the difference between a 30-second diagnosis and a 30-minute one.

The test rework is a real improvement — swapping a 2s settle loop against a shared directory for an exact assertion in an isolated one, while keeping a positive control so "empty afterwards" cannot pass vacuously. The fault-injection table is the right way to demonstrate that.

Below: one design point, one latent leak, and some smaller items.

---

1. withSpillDir silently drops subclass behavior

xl-cats-effect/src/com/tjclp/xl/io/ExcelIO.scala:64

def withSpillDir(dir: Path): ExcelIO[F] =
  new ExcelIO[F](warningHandler):
    override def spillDir: Option[Path] = Some(dir)

ExcelIO is a non-final public class, and this PR makes spillDir a public overridable def — which invites overriding. But withSpillDir reconstructs from warningHandler alone, so for any subclass MyExcelIO extends ExcelIO[IO](h), myExcel.withSpillDir(d) returns a plain ExcelIO and every override is gone. Chaining and withWarnings(h).withSpillDir(d) both work correctly (the handler is captured), so this is narrowly about subclasses — but it is a silent downgrade, not a compile error.

Two ways out, either fine:

• Mark withSpillDir final and document that subclasses should override spillDir directly rather than call it.
• Or route it through a protected def copyWith(spill: Option[Path]): ExcelIO[F] that subclasses can override — more machinery than this probably needs.

2. Multi-sheet acquire can leak spills if createSpillFile throws partway

xl-cats-effect/src/com/tjclp/xl/io/ExcelIO.scala:1120

Stream.bracket(
  Sync[F].delay {
    sheetsWithIndices.map { case (name, sheetIndex, _) =>
      val tempFile = createSpillFile(s"xl-stream-$sheetIndex-")
      ...

If the map throws on sheet k, acquire fails and the release never runs, so sheets 1..k-1's scratch files stay on disk. This is pre-existing, but the PR makes it materially more reachable: previously the only realistic acquire-time failure was a broken java.io.tmpdir, which fails on sheet 1 and leaks nothing. A caller-supplied directory adds ENOSPC, quota, and per-directory permission failures — all of which can land on sheet 3 of 5, in the user's directory rather than one the OS eventually reaps.

Concretely: 5 sheets, spill volume with room for 3 → two orphaned xl-stream-*.xml files persist after the write fails. Cheap fix — accumulate the created paths and delete them before rethrowing. Note that #513's multi-sheet test covers release-path partial deletion, not acquire-path.

3. The load-bearing detail of the chosen shape is untested

The whole point of new ExcelIO[F](warningHandler) (rather than instance[F]) is that the handler survives. Nothing pins that: someone simplifying this to ExcelIO.instance[F].withSpillDir(...) would silently drop a caller's warning handler and all 5 new tests stay green. The companion ExcelIO.withSpillDir[F](dir) is also entirely uncovered. Three cheap assertions would close it:

• ExcelIO.withWarnings[IO](collect).withSpillDir(d) — read a warning-producing file, assert the warning still arrives.
• ExcelIO.withSpillDir[IO](d).spillDir == Some(d).
• instance.withSpillDir(a).withSpillDir(b).spillDir == Some(b) (works today; worth pinning since the anon class does not override withSpillDir).

4. Security note the docs should carry

The spill holds the full worksheet body in cleartext for the duration of the write. On the default path the JDK's TempFileHelper applies rw------- on POSIX filesystems, and Files.createTempFile(dir, ...) applies the same — so the file mode does not regress when redirected. But the containing directory's mode now becomes the caller's responsibility, and the natural target for this feature ("the fast volume", "the big scratch mount") is often shared and group- or world-traversable. One sentence in the performance-guide block — the directory's permissions govern who can see the scratch data, so prefer one the process owns — would land it where people are making the choice.

5. Smaller items

• xl-cats-effect/test/src/com/tjclp/xl/io/ExcelIOSpec.scala:10 — import scala.concurrent.duration.* is now dead; the poll loop was its only consumer and there are zero remaining .millis / .seconds / FiniteDuration references in the file. No -Wunused in scalacOptions, so nothing flags it.
• ExcelIOSpec.scala:46 — Files.createTempFile(source.getParent, ...) NPEs if source is ever a bare relative filename. Both call sites are fixture-rooted so it is correct today; Option(source.getParent) with a fallback would make it robust to reuse. Low priority.
• Missing-directory test — result.left.toOption.fold("")(e => s"${e.getMessage}") builds the message before the isLeft assertion, and getMessage is nullable in general (not for this throw site). The s"${...}" around a lone getMessage is redundant. Cosmetic.
• Error type — createSpillFile throws a wrapped java.io.IOException rather than an XLError, against CLAUDE.md's "always use XLResult[A]". It is consistent with the streaming writers around it, which already raise exceptions into F, so I would leave it — flagging only so the inconsistency is a decision rather than an oversight.
• Scope calls — deferring the CLI flag (-Djava.io.tmpdir covers it) and XlsxWriter.writeToBytes (pure module, no interpreter to carry the setting) are both right. The latter is worth a tracking issue if one does not exist; it is the same failure on the same small /tmp, just without a lever.

---

Nothing here blocks. #1 and #2 are the two I would want addressed before merge — #2 especially, since redirecting the spill is precisely what makes a mid-acquire failure plausible.

Reviewed statically — I was not able to run ./mill xl-cats-effect.test in this environment, so the green release gates in the PR body are taken as reported.

[arcaputo3]

Both of the pre-merge items taken, plus the smaller ones. 3aaa8d5.

#2 — acquire-path leak. Correct, and correctly identified as made reachable by this PR rather than introduced by it. Acquire now folds over the sheets and unwinds what it already took before rethrowing.

Driving it in a test turned out to be easy through the seam this PR added: override spillDir to hand back a real directory for the first two sheets and a missing one for the third, which is what a volume filling up mid-acquire looks like from inside createSpillFile. Without the fix it fails naming both orphans:

spills taken before the failure should be unwound, found:
  xl-stream-1-15106157766921873697.xml, xl-stream-2-17364042653267162372.xml

#1 — withSpillDir drops subclass behavior. Took the final option. copyWith is more machinery than a class with no other extension points earns, and final plus a scaladoc line ("carries the warning handler and nothing else; subclasses should override spillDir directly") states the trap where someone would hit it, without pretending the class supports cloning arbitrary subclasses.

#3 — the load-bearing detail was untested. Added all three assertions you listed. The warning one needed a warning-producing file, so the spec grew a withoutZipEntry helper that strips xl/styles.xml to provoke MissingStylesXml. Confirmed it bites: simplifying withSpillDir to build from a fresh instance fails it.

#4 — security note. Added. Kept your framing that the file mode does not regress (rw------- either way) and that what changes is who owns the directory's permissions.

#5 — smaller items. Dead duration import removed; both fixture-adjacent copies now go through a fixtureDirOf helper that tolerates a parentless path; the missing-directory test asserts isLeft before building its message and drops the redundant interpolation. On the error type: leaving the wrapped IOException, as you suggested — the streaming writers around it already raise exceptions into F, and an XLError here would be the odd one out. Recording it as a decision, not an oversight.

Filed rather than fixed:

• XlsxWriter.writeToBytes spills to java.io.tmpdir with no lever #516 for XlsxWriter.writeToBytes. It's the same failure with no lever, and I'd rather it not spill at all than spill somewhere configurable — OutputStreamTarget already exists, which hints the writer may be able to target a ByteArrayOutputStream directly.
• MiMa: good catch that the javap guarantee is currently manual. That's a CI change with its own decisions (which modules, which baseline version, how to whitelist intentional breaks), so it shouldn't ride along here. Worth its own issue if binary compatibility is meant to be a standing promise — happy to file if you want it.

Counts to 5,454 / xl-cats-effect 148. All four gates re-run green.

[claude]

Review — feat(cats-effect): make the streaming writer's spill directory injectable

Read the whole diff plus the surrounding writer code. The shape is right and the reasoning in the description holds up under checking: both spill sites are converted (grepping createTempFile across xl-cats-effect/src leaves only ZipTransformer's output-adjacent temps, which correctly follow the output volume, and XlsxWriter.writeToBytes, deliberately deferred), the constructor really is untouched so spillDir is purely additive, and final on withSpillDir with the subclass-override trap spelled out in the scaladoc is the right call. The docs earn their place too — the permissions caveat on redirected scratch is the kind of note most libraries omit, and it's accurate: Files.createTempFile creates rw------- on POSIX and uses CREATE_NEW, so the residual exposure genuinely is the directory's traversability, exactly as written.

Nothing blocking. Findings below, roughly by weight.

1. createSpillFile reads spillDir twice, so the failure message can name a directory that didn't fail

ExcelIO.scala:80-93: the happy path matches on spillDir, and the catch calls spillDir.fold(...) again to build the message. Because spillDir is an overridable def with no documented arity, a caller whose override isn't constant can get an error naming a different directory than the one that actually threw — which undercuts the whole point of the wrapped message ("names the setting so the cause is not mistaken for a problem with the workbook"). Snapshot it once:

private def createSpillFile(prefix: String): Path =
  val target = spillDir
  try
    target match
      case Some(dir) => JFiles.createTempFile(dir, prefix, ".xml")
      case None      => JFiles.createTempFile(prefix, ".xml")
  catch
    case e: java.io.IOException =>
      val where = target.fold(...)
      ...

This isn't hypothetical in-repo: the new unwind test at ExcelIOSpec.scala:1663 overrides spillDir with a call counter, so it hits spillDir four times for three sheets today. It passes only because the predicate is calls <= 2 and both reads on sheet 3 land past it. Fixing this makes the reads exactly one per sheet and the test's seam honest rather than accidentally consistent.

2. Unwinding a partial acquire can abort itself and swallow the diagnostic

ExcelIO.scala:1142-1145: JFiles.deleteIfExists in the catch is unguarded. If the first delete throws — the same volume trouble that plausibly caused the acquire failure, or a Windows lock — the remaining spills leak and the propagated exception becomes the delete failure instead of the carefully-worded spill-directory message. The unwind is best-effort cleanup, so it should never displace the cause:

case e: Throwable =>
  acquired.foreach { case (_, _, tempFile, _) =>
    try JFiles.deleteIfExists(tempFile)
    catch case d: Throwable => e.addSuppressed(d)
  }
  throw e

Small, but the whole reason this block exists is the pathological-filesystem case, which is precisely where deletes also fail.

3. Test-count numbers in the description disagree with the diff

The body says 5,452 / xl-cats-effect 146; the diff writes 5,454 / 148 into CLAUDE.md and docs/STATUS.md. The diff is the correct one by my count — four tests added (missing directory, partial-acquire unwind, warning-handler carryover, default-is-tmpdir), one removed (assertSpillsCleared names a spill that outlives its budget), net +3 on 145. Just the PR body being stale, but worth correcting before the description becomes the merge commit message.

4. Coverage gap: exists-but-unwritable

The docs sell two failure modes ("missing or unwritable"), and only missing is pinned. The leftover naming in that test gives it away — the output file is dir.resolve("unwritable.xlsx") (ExcelIOSpec.scala:1649) while nothing in the test is unwritable. AccessDeniedException takes a different path through the JDK than NoSuchFileException, so it's a genuinely distinct case, and it's a few lines guarded on POSIX:

val locked = Files.createDirectory(dir.resolve("locked"))
Files.setPosixFilePermissions(locked, PosixFilePermissions.fromString("r-x------"))

(with a teardown that restores rwx so the fixture's Files.walk delete doesn't fail). Either add it, or rename that output file so it stops implying coverage that isn't there.

5. fixtureDirOf's working-directory fallback is now unbounded

ExcelIOSpec.scala:40-41: dropping deleteOnExit in favour of fixture teardown is the right trade, but it changes what the fallback costs. Previously a parentless source meant a temp file the JVM would try to remove; now it means a remap-*.xlsx / stripped-*.xlsx written into the CWD (the repo root under Mill) and never cleaned. Both call sites pass fixture-local paths, so the branch is unreachable — which is the argument for fail(s"expected a fixture-local source, got: $source") over a silent CWD write.

6. Minor / no action needed

• catch case e: Throwable in the unwind is fine given it rethrows, but combined with Add P6 Cell-level Codecs + P31 Ergonomics & Purity Enhancements #2 it's the only place a fatal error could get quietly reshaped. The addSuppressed form above fixes both together.
• withSpillDir returns ExcelIO[F], so the setting is unreachable for code programmed against Excel[F] / ExcelR[F]. Harmless today because neither auto-detect writer is on those traits — just a constraint to remember if one is ever promoted.
• CLI deferral is well-argued, and I agree the mapN / 8-parameter run diff doesn't belong here. Worth capturing in the follow-up that xl import --stream and StreamingWriteCommands can push real volume through /tmp (ImportCommands.scala:235, StreamingWriteCommands.scala:65), so the container-with-a-tiny-/tmp scenario the docs describe is reachable from the CLI too, not only the library.
• spillDir arity — that it's consulted once per sheet rather than once per write is now load-bearing for subclass overrides. One clause in the scaladoc ("consulted once per spilled sheet") would make that contract explicit instead of emergent.

What I checked and liked

The test rewrite is the strongest part. Trading a 2s settle loop against a shared directory for exact assertions in a writer-owned directory is a clear win, and you kept the thing that makes it meaningful — the mid-write sampleDir positive control, without which "empty afterwards" would also pass for a writer that ignored the setting entirely. The default spill directory is java.io.tmpdir test being explicitly positive-only, with the reason in a comment, is the correct handling for an assertion that has to share a directory with sibling worker JVMs. And the fault-injection table in the description is the right evidence to ship with a test-hygiene change — "4 of 5 fail when createSpillFile ignores its argument" is what makes the coverage claim checkable.

One caveat on this review: I could not run the suite in this sandbox (./mill wasn't permitted), so the above is static analysis against the diff and the surrounding code; I'm taking the described green gate run at its word.

[arcaputo3]

Both weighted items taken, and item 1 was a good catch — made better by the fact that my own new test was the thing exposing it. 11a5f47.

#1 — double read of spillDir. Snapshotted once. You're right that it wasn't hypothetical: the unwind test's counter override was hitting spillDir four times for three sheets, passing only because both reads on sheet 3 landed past calls <= 2. Now one read per spill, which makes that seam honest rather than accidentally consistent, and the scaladoc states the arity ("consulted exactly once per spilled sheet") so subclass overrides have a contract instead of an emergent behavior.

#2 — unwind can swallow the diagnostic. Taken in the addSuppressed form. The argument that convinced me is yours: the block exists for the pathological-filesystem case, which is precisely where the deletes also fail, so best-effort cleanup must never displace the cause.

#4 — the test name implied coverage it didn't have. Rather than rename, added the case: a r-x------ directory, which reaches createSpillFile as AccessDeniedException rather than NoSuchFileException. It skips itself on non-POSIX filesystems and when the mode bits don't constrain the user (root), so it can't turn into a CI false failure. Teardown restores rwx before the fixture walk. Renamed the misleading output file too.

#5 — fixtureDirOf fallback. Taken as fail(...). Your reasoning is the deciding factor: dropping deleteOnExit for fixture teardown changed what the fallback costs, from a file the JVM tries to remove to one stranded in the repo root.

#3 — stale counts in the description. Fixed; the body now says 5,455 / xl-cats-effect 149, re-measured from testForked.json after this round.

#6 minors. Added the arity clause (above). Left catch case e: Throwable in the unwind — with addSuppressed it now rethrows the original untouched, which was the concern. Noted the Excel[F] / ExcelR[F] constraint but no action, since neither auto-detect writer is on those traits.

Filed:

• CLI: no way to redirect the streaming spill directory #517 for the CLI flag, carrying your point that xl import --stream (ImportCommands.scala:235) and StreamingWriteCommands.scala:65 push real volume through /tmp, so the scenario is reachable from the CLI too — with the note that -Djava.io.tmpdir doesn't help the native-image build.
• MiMa — still happy to file, but it needs a decision I shouldn't make unilaterally: whether binary compatibility is a standing promise for the published modules, and against which baseline. Say the word and I'll open it.

[claude]

Review: injectable spill directory for the two-pass streaming writers

Strong PR. The problem statement is real (containers with a tiny/ro /tmp; POI's TempFileCreationStrategy is the right precedent), the three shape decisions are each defensible and — unusually — actually justified rather than asserted, and the fault-injection table is the kind of verification that makes a reviewer's job easy. The partial-acquire leak is a genuine find, and the observation that this PR is what makes it reachable is exactly the right framing.

Comments below, roughly by severity. Nothing here is a blocker.

Correctness / robustness

1. createSpillFile catches only IOException — xl-cats-effect/src/com/tjclp/xl/io/ExcelIO.scala:91

The helper exists to guarantee the failure names the setting, but Files.createTempFile can also throw IllegalArgumentException (a prefix/suffix the target filesystem rejects — plausible on a non-default FileSystemProvider) and UnsupportedOperationException (a provider that doesn't support the default PosixFilePermissions attribute). Those escape bare, and the user is back to guessing whether the workbook or the directory is at fault. case NonFatal(e) costs nothing and closes the gap; the getMessage-based message already degrades gracefully for non-IO causes.

2. The unwritable-directory test can turn a skip into a failure on non-POSIX — xl-cats-effect/test/src/com/tjclp/xl/io/ExcelIOSpec.scala:1673-1690

.guarantee(restore) is attached to the whole chain, which is right for the normal path — but if the first assume trips (no posix view), restore still runs setPosixFilePermissions on a filesystem that doesn't support it, throws UnsupportedOperationException, and CE replaces the assumption outcome with a hard failure. Linux CI never sees it, but a contributor on Windows gets a red test with a confusing cause. Either hoist the posix check above Files.createDirectory(locked) / before restore is in play, or make restore best-effort (.attempt.void) — teardown of a permission you may never have set is inherently best-effort anyway.

3. catch case e: Throwable in the acquire unwind — ExcelIO.scala:1148

Behaviour-preserving (you rethrow e unchanged), so low risk, but it does mean an OutOfMemoryError or LinkageError sends you into a deleteIfExists loop and an addSuppressed call. NonFatal is the conventional net here and loses nothing you want to catch — a fatal error mid-acquire is not a case where the orphaned spills are the interesting problem.

API design

4. withSpillDir documents a trap rather than removing one — ExcelIO.scala:72-74

The scaladoc is honest about it: final, returns a plain ExcelIO, so a subclass calling it silently loses its own overrides, and final exists to stop the trap being widened. That reasoning holds, but a protected def copyWith(spill: Option[Path]): ExcelIO[F] seam (overridable by subclasses, called by a non-final withSpillDir) would remove the sharp edge instead of signposting it. Worth it only if subclassing ExcelIO is a use case you want to support — if it isn't, the stronger move is to say so and consider sealing the class in a future major. Fine as shipped either way.

5. Binary-compat guarantee is manual. The javap check on the erased constructor is the right diligence and the writeup is convincing. Since the PR explicitly notes there's no MiMa in CI, that's the follow-up I'd prioritise over #516/#517 — this reasoning is exactly the kind that decays silently on the next person's refactor.

Tests

Genuinely improved: exact assertions against an isolated directory beat a 2s settle loop against shared /tmp, and keeping the positive control ("the spill appeared here first") is what stops the isolation from making the test vacuous. The deliberately positive-only java.io.tmpdir test is the right call for pinning the default.

Gaps I'd consider:

• No multi-sheet happy path with withSpillDir. Both multi-sheet tests are failure paths. The single-sheet test reads the workbook back and checks A1/A100; the multi-sheet assembler is the more intricate of the two (one workbook-global SST, N spills, assembleMultiSheetZip zipping them in order) and has no equivalent "redirecting the scratch file didn't change the output" assertion.
• spillDir pointing at an existing regular file is a plausible user mistake (withSpillDir(someFile)), currently surfacing as a wrapped FileSystemException: Not a directory. Behaviour is fine; it's just unpinned.
• The None branch of the error message ("the default temp directory (java.io.tmpdir)") is unreachable from tests and therefore uncovered. Low value — noting it only so it's a known choice.

Nit — var calls in the partial-acquire override (ExcelIOSpec.scala:1651) makes the test depend on spillDir being consulted in sheet order, single-threaded. True today (the fold is sequential inside one Sync[F].delay), and CLAUDE.md allows var in tests, but an AtomicInteger costs the same and doesn't encode that assumption.

Observation, pre-existing, not this PR's job: the failure tests assert the spill directory is empty, but the two-pass writers open path directly (ExcelIO.scala:867, ExcelIO.scala:1219), so a failed write also leaves a truncated .xlsx at the destination. Given "cleanup on failure" is this PR's theme, that asymmetry might deserve its own issue alongside #516/#517.

Performance

acquired :+ (...) in the fold (ExcelIO.scala:1141) is O(n²) in sheet count on a Seq. Utterly dominated by file I/O at any realistic sheet count — mentioning only because a ListBuffer or reverse-prepend-then-.reverse is free and the unwind loop already wants a mutable-ish accumulator shape.

Docs / security

The performance-guide note is a good addition and the permissions claim is accurate — Files.createTempFile is rw------- on POSIX, and warning that the directory's mode becomes the caller's problem is the right emphasis for a setting whose natural targets are shared scratch mounts.

One thing I'd add to it: the spill is not registered with deleteOnExit (correctly — bracket handles it, and deleteOnExit leaks in long-lived JVMs). The consequence is that a SIGKILL/OOM-kill mid-write strands the worksheet body. On java.io.tmpdir that's usually swept by the OS or a reboot; on the persistent fast volume this feature exists to point at, it survives indefinitely. Callers redirecting the spill have quietly taken on sweeping orphans — worth one sentence next to the permissions paragraph.

Doc drift (mostly pre-existing): the test count now lives in four places and only two are updated here — docs/reference/testing-guide.md:222 still says xl-cats-effect 111, and docs/plan/roadmap.md:13 says 5,448. Not introduced by this PR, but since you're touching two of the four, either fix the other two or drop the per-module duplication.

On #517

For the CLI, a system-property default consulted by spillDir (xl.spillDir) would fix xl import --stream without touching the 8-parameter run or the mapN chain — worth weighing against a proper --spill-dir flag, which is more discoverable but much more diff. Reading ambient config in a library defaulting path does cut against the determinism charter, so it may be the wrong trade; flagging it as an option rather than a recommendation.

Nice work — the writeup alone made this a fast review.

[arcaputo3]

Last two taken. 7910a30.

The orphan-on-SIGKILL trade. Good catch, and the right frame: the consequence isn't the same on both paths. On java.io.tmpdir the OS sweeps it; on the persistent volume this feature points at, it stays. The guide now says that next to the permissions note, along with why deleteOnExit isn't used (it leaks in long-lived JVMs), so nobody "fixes" the absence.

Doc drift. Fixed all four rather than two. testing-guide.md was the worst of them — a 4,085 total with per-module rows to match, stale by roughly 1,370 tests — and roadmap.md still said 5,448. All four now carry the same measured numbers (5,455) and every per-module table sums to its own headline. Agreed the duplication is the underlying problem; consolidating it is a docs-pass call rather than something to decide here.

On #517: added your xl.spillDir system-property idea to the issue, with the determinism caveat you flagged and one suggestion — if it goes that way, read the property at the CLI entry point and call withSpillDir explicitly, so the ambient read lives in the application layer and the library defaulting path stays deterministic.

Gates re-run green.

[claude]

Review: injectable spill directory

Nice piece of work. The problem is real (POI's TempFileCreationStrategy is the right prior art), the binary-compat reasoning behind def spillDir over a constructor parameter is sound and worth the extra prose, and the test rewrite is a genuine improvement — trading a 2s settle loop against shared /tmp for exact assertions on a fixture-owned directory is exactly the right move. The Files.walk(...).sorted(reverseOrder) teardown already handles the new spill/ subdirectory, and the fault-injection table is the kind of verification I wish more PRs carried.

Findings below, roughly in descending order of how much I would care.

---

1. Consistency: the rest of the codebase already colocates scratch with output

ExcelIO.scala:54 defaults to java.io.tmpdir, but the two other scratch-file sites in this module deliberately do not:

• ZipTransformer.scala:377-378 — Option(output.getParent).getOrElse(Path.of("."))
• Excel.scala:420-421 — Option(targetPath.getParent).getOrElse(Paths.get("."))

So after this PR the library has two different answers to "where does scratch go", and the streaming writer — the one that moves the most bytes — is the one that picks the small tmpfs. Keeping the default unchanged is defensible for compat, but it is worth stating in the scaladoc why the two-pass spill differs from its neighbours, or reconsidering the default: the output's parent is already known at both spill sites, is on the same volume as the thing being written, and is by construction writable if the write is going to succeed at all.

Concretely, this also shrinks 517 to one line. ImportCommands.scala:235 and StreamingWriteCommands.scala:65 build ExcelIO.instance[IO] inline, so today:

ExcelIO
  .withSpillDir[IO](Option(outputPath.getParent).getOrElse(Path.of(".")))
  .writeStreamWithAutoDetect(outputPath, sheetName)

That fixes the motivating case named in the PR body (xl import --stream pushing real volume through /tmp) without touching the CLI's mapN chain or the 8-parameter run — a --spill-dir flag then becomes a genuine nice-to-have rather than the thing standing between users and a working import. Mind the null parent for a bare filename, per the two precedents above.

---

2. catch case d: Throwable around the unwind deletes (ExcelIO.scala:1152-1153)

The outer catch case e: Throwable (line 1148) I would defend — you really do want the spills unwound regardless of what failed, and it rethrows, so nothing is swallowed. The inner one is different: a VirtualMachineError raised by deleteIfExists gets demoted to a suppressed exception on e and the loop carries on. scala.util.control.NonFatal(d) expresses the intent ("a delete that fails must not mask the cause") without also absorbing errors that should terminate the JVM.

---

3. createSpillFile only wraps IOException (ExcelIO.scala:91)

NoSuchFileException, AccessDeniedException and NotDirectoryException all land here, so the cases you tested are covered. But Files.createTempFile can also raise SecurityException (under a security manager) and, for a spillDir whose provider rejects the resolved name, InvalidPathException/IllegalArgumentException — those escape without the "names the setting" framing that is the whole point of the wrapper. A case NonFatal(e) here, still rethrowing as IOException, would make the guarantee unconditional.

---

4. foldLeft + :+ on Seq.empty (ExcelIO.scala:1138-1141)

Seq.empty is List, so each :+ copies the accumulator — quadratic in sheet count. Bounded by the number of sheets, so genuinely negligible at runtime; flagging it only because that accumulator is also what the unwind iterates, and a Vector (or a ListBuffer inside what is already an impure delay block) reads no worse and does not invite the question.

---

5. "Consulted exactly once per spilled sheet" is now public contract (ExcelIO.scala:51-52)

The partial-acquire test (ExcelIOSpec.scala:1722-1725) leans on that arity with a var counter, which makes the read count a compatibility surface: a future change that pre-validates spillDir before the fold — a plausible improvement, since it would move missing-directory detection ahead of any file creation — becomes a doc change plus a test rewrite rather than a refactor. If the intent is just "the multi-sheet acquire unwinds what it took", an alternative seam is an override keyed on something stable rather than on call count. Judgment call, and the PR is explicit about it, but I would rather the scaladoc promised "read per spilled sheet, not cached across writes" and left the exact count to the implementation.

---

6. withSpillDir silently discards subclass overrides (ExcelIO.scala:72-74)

Well documented, and final correctly stops the trap from widening. Worth noting that nothing in the repo subclasses ExcelIO — only the anonymous instances at ExcelIO.scala:73, :1393, :1397 and the test at ExcelIOSpec.scala:1723 — so if the class does not need to be open, final class ExcelIO in a future major would delete the sharp edge entirely rather than documenting it.

---

Test coverage

Strong overall; the positive controls are the right shape, and pinning the default with a deliberately positive-only test is a good call. Gaps I would close:

• The permissions claim is now documented but untested. performance-guide.md asserts the spill is rw------- on POSIX, and that is the security-relevant half of "the directory's permissions become your call". The sampling harness already captures the exact path mid-write, so this is a two-line Files.getPosixFilePermissions assertion behind the same supportedFileAttributeViews guard you already use.
• No success-path test for multi-sheet plus custom spill dir. writeStreamsSeqWithAutoDetect is covered for mid-stream failure and for partial acquire, but not for "redirect the spill, get a correct two-sheet workbook out". The single-sheet test added exactly that check (wb.sheets.head(ref"A1")), and the multi-sheet path, with its shared workbook-global SST, is the one where routing could plausibly interact with correctness.
• spillDir pointing at an existing regular file reaches createSpillFile as a different IOException subtype than either case you cover, and is a likely user typo (--spill-dir /tmp/foo.xlsx).

Test nits

• ExcelIOSpec.scala:1683-1687 — restore calls setPosixFilePermissions unconditionally, but it runs under .guarantee even when the assume at line 1673 short-circuits because the filesystem has no POSIX view. On such a platform the finalizer raises UnsupportedOperationException and can override the assumption violation, turning a clean skip into an error. Guard restore with the same check, or capture a posix: Boolean once and gate both.
• ExcelIOSpec.scala:1691-1692 — fs2.Stream.emits(rows.compile.toVector) round-trips a Stream[Pure, RowData] through a Vector for no effect; the sibling tests pass rows straight to .through(...), and Pure widens to IO fine.
• spillDirUnder (line 1539) is bypassed by two of the five tests that need a spill directory (lines 1720 and 1741 call Files.createDirectory inline). Understandable given those two are not inside an IO for-comprehension at that point, but the helper reads as the way to get one.
• var calls (line 1722) is fine in practice — all three reads happen sequentially inside one Sync.delay on one thread — but an AtomicInteger or Ref costs nothing and removes the need for that argument.

Docs

The performance-guide section is the best part of the docs change; naming both inherited consequences (directory permissions, and the SIGKILL case where no deleteOnExit means no sweep on a persistent volume) is more honest than most libraries manage. Test counts are internally consistent (145 minus 1 removed plus 5 added = 149; 5,451 to 5,455), and roadmap.md picks up a stale 5,448 along the way.

---

Note on verification: I reviewed this statically. ./mill xl-cats-effect.test was not available to me in this environment, so I am taking the green release gates in the PR body at face value rather than confirming them independently.

None of the above is blocking. Item 1 is the one I would most like to see addressed, and it is the cheapest.