fix(bundle): produce output for bundle --check /dev/stdin - #36264
fix(bundle): produce output for bundle --check /dev/stdin#36264nathanwhitbot wants to merge 4 commits into
bundle --check /dev/stdin#36264Conversation
…d#36162) `deno bundle --check /dev/stdin` silently produced no output while the same command without `--check` worked. `/dev/stdin` is a pipe that can be read only once, but `--check` read it twice across two independent `CliFactory` instances: first when building the type-check module graph, then again when esbuild's loader fetched the entrypoint. The second read hit EOF, so esbuild received empty content and emitted nothing. Additionally, on Linux `canonicalize("/dev/stdin")` errors (realpath resolves to a now-nonexistent `/proc/PID/fd/pipe:[inode]`), so the bundle command errored at resolution time before the double-read even mattered. Fix: - `resolve_url_or_path_absolute`: when `canonicalize` fails but the path exists and is a non-regular file, fall back to the uncanonicalized `file://` URL. Genuinely-missing paths still error as before. - Read non-regular entrypoints (e.g. `/dev/stdin`) exactly once into memory and register the `File`s in both the type-check factory's and the bundle factory's file fetchers (which consult in-memory files before touching the filesystem), so neither path re-reads the drained pipe. Adds a unix-gated spec test asserting `bundle --check /dev/stdin` and the no-`--check` variant emit identical bundled output. Closes denoland#36162
…gression tests (denoland#36162) Resolve a bundle entrypoint that is a non-regular file (FIFO/char device/socket, e.g. /dev/stdin when stdin is a pipe, or a symlink to a FIFO) to its uncanonicalized file:// URL instead of canonicalizing it. canonicalize both errors on Linux /dev/stdin (realpath -> /proc/PID/fd/pipe:[inode]) and *rewrites* the path elsewhere (macOS /dev/fd/0, a symlink to its target). Any rewrite makes the bundle flow's specifier diverge from the --check flow's collect_specifiers specifier, so the shared in-memory file is missed and the already-drained pipe is reopened, hanging or emitting nothing. Keeping one stable URL fixes both. Add tests/integration/bundle_tests.rs: mkfifo + a symlink alias + a background writer exercise 'bundle --check' of a symlink-to-FIFO, proving the piped source is read once (no hang) and that the type-check graph sees it (a type error surfaces). Closes denoland#36162
…#36162) `read_nonregular_entrypoints` read every non-regular entrypoint (e.g. `/dev/stdin` when stdin is a pipe, or a symlink to a FIFO) once per occurrence, without deduplicating by resolved URL. Passing the same non-regular entrypoint more than once therefore drained the pipe twice: the second EOF read overwrote the first source in the in-memory files, so `deno bundle --check --outdir=<tmp> /dev/stdin /dev/stdin` exited 0 but wrote a 0-byte bundle; a duplicated symlink-to-FIFO blocked forever on the second reopen. Duplicate entrypoints are valid input (they collapse to one bundle), so this must be handled, not rejected. Track a `HashSet` of resolved file-scheme URLs and skip any entrypoint whose URL was already seen, before the `std::fs::read`, so each distinct non-regular entrypoint is read at most once. Insert regular files into the set too, so a mixed duplicate list can't slip a second read through. Add a bounded `#[cfg(unix)]` regression test that passes the same symlink-to-FIFO entrypoint twice with a single background writer: the fix reads once (rendezvous → success, non-empty bundle), while a regression reopens the drained FIFO and the 30s timeout fails fast instead of hanging CI.
bartlomieju
left a comment
There was a problem hiding this comment.
Nice fix, and the writeup is excellent. The core mechanism checks out: shared interior-mutable MemoryFiles, memory-first lookup on esbuild's permissioned on_load fetch, cheap Arc-backed File clone, and on native targets the check flow's collect_specifiers URL is byte-identical to the pre-read key (deno_path_util::url_from_file_path delegates to Url::from_file_path, same as ModuleSpecifier::from_file_path). A few things worth addressing, roughly in severity order:
1. --check /dev/stdin doesn't actually type-check (silent gap). The pre-read File for the extensionless /dev/stdin is built with maybe_headers: None, so its media type resolves to Unknown and the type-checker doesn't treat it as TypeScript. So printf 'const x: number = "s";' | deno bundle --check /dev/stdin produces the bundle but does not surface TS2322 — which defeats the point of --check for the exact input this PR targets. This is why it slips past the tests: bundle_check_symlink_to_fifo_type_error_from_check uses a .ts alias (has an extension, so it works), and the /dev/stdin spec test uses plain JS with no type error. Consider stamping a TypeScript content-type header on the pre-read File for the extensionless case (and adding a --check /dev/stdin test with a type error).
2. Missing entrypoint under --check now errors early with no path context. read_nonregular_entrypoints calls resolve_url_or_path_absolute(entrypoint)?, which falls through to canonicalize_path and errors before the check graph runs. deno bundle --check ./missing.ts now prints No such file or directory (os error 2) instead of the previous clearer "Module not found ./missing.ts". Minor, but a user-facing regression scoped to the --check path.
3. Bundle flow keys on_load by resolver.resolve() output, but the memory File is keyed pre-resolve. read_nonregular_entrypoints inserts under resolve_url_or_path_absolute(entrypoint), while resolve_entrypoints feeds resolver.resolve(...) into the bundle graph and on_load fetches by that. If an import map / workspace remaps the entrypoint, the lookup misses and esbuild reopens the drained pipe. The check flow is unaffected (it bypasses resolver.resolve). Narrow, but currently untested.
4. Dedup is by resolved URL, not by the underlying fd/inode. Two distinct specifiers that alias the same pipe — deno bundle --check /dev/stdin /dev/fd/0, or a symlink passed alongside its target — produce different URLs, so seen never collides and both get std::fs::read; the second hits EOF (or blocks). Same class as #36162, left partially open.
5. A swallowed metadata() error silently reintroduces the double-read. In read_nonregular_entrypoints, match std::fs::metadata(&path) { ... _ => {} } means a transient EINTR/EACCES on a real FIFO produces no memory File, and both factories then read the pipe independently → empty output, no diagnostic.
6. The new integration tests can flake/hang CI. Each builds TestContextBuilder::for_npm() (registry + esbuild download) under a fixed 30s timeout that a loaded runner can exceed; the background writer parks in open() forever on the regression path (recv_timeout bounds the wait but never joins/kills the thread, leaking a FIFO fd for the rest of the run); and they hard-depend on an external mkfifo binary, panicking rather than skipping where it's absent.
7. Duplicated classification + redundant stats (cleanup). !is_file() && !is_dir() is copy-pasted in read_nonregular_entrypoints and resolve_url_or_path_absolute, and the same path is stat'd 2–3× per entrypoint (the former calls the latter, which stats, then stats again; resolve_entrypoints stats a third time). A single is_nonregular(&Path) helper would DRY both up.
8. Altitude. The memory-file threading + the non-canonicalize special case are really working around the two independent CliFactory instances (--check builds the graph twice, each with its own fetcher). Sharing a single graph/fetcher between the check and bundle passes would make both mechanisms unnecessary — probably out of scope here, but worth noting as the deeper fix.
1 and 2 are the ones I'd want resolved before merge (1 is a silent correctness hole; 2 is a regression); 3–5 are narrow robustness edges; 6–8 are test/cleanup/altitude.
…nd#36162) Review follow-up for `bundle --check /dev/stdin`: 1. The pre-read `File` for an extensionless entrypoint had no media type, so deno_graph assumed JavaScript for the root and `--check` silently skipped type-checking piped TypeScript. Stamp an `application/typescript` content type when the entrypoint carries no extension of its own, matching `deno run -` (`$deno$stdin.mts`). The pre-read now happens for the non-`--check` flow too, so an extensionless entrypoint gets the same media type either way. 2. `read_nonregular_entrypoints` no longer resolves through `resolve_url_or_path_absolute`, so a missing entrypoint under `--check` reports `TS2307 Cannot find module <path>` again instead of a bare `No such file or directory (os error 2)`. 3. Register the pre-read content under `resolver.resolve()`'s output as well (when it still names the same file), so an import map that remaps the entrypoint doesn't send esbuild back to the drained pipe. 4. Dedupe by underlying file identity (device + inode) rather than by resolved URL, so `/dev/stdin /dev/fd/0` — or a symlink passed alongside its target — reads the pipe once and shares the bytes with every alias. 5. Surface non-`NotFound` `metadata()` errors instead of swallowing them (swallowing quietly restored the read-the-pipe-twice path). 6. Tests: create the FIFO with `nix::unistd::mkfifo` instead of shelling out to a `mkfifo` binary, raise the bound to 180s so a cold/loaded runner doesn't trip it, and always unpark and join the writer thread (opening the read end with `O_NONBLOCK`) so no thread or fd leaks. Adds coverage for a type error via `--check /dev/stdin`, TypeScript via `/dev/stdin` without `--check`, two aliases of one pipe, and an import-map-remapped FIFO entrypoint. 7. Factor the `!is_file() && !is_dir()` classification into `is_nonregular` and drop a redundant `stat` per entrypoint.
|
Thanks — pushed a commit addressing 1–7. 1 (silent gap). Confirmed: the pre-read 2 (regression). 3. The pre-read content is now also registered under 4. Dedup is now by device+inode (path on non-unix) rather than resolved URL, and the bytes are shared with every aliasing URL. 5. Non- 6. FIFO creation uses 7. Single 8. Agreed, out of scope here — the two Verified locally: 69 |
Fixes #36162.
Problem
cat src/main.ts | deno bundle --check /dev/stdinsilently produced no output, while dropping--checkworked./dev/stdinis a pipe that can be read only once, but with--checkdeno reads it twice, across two independentCliFactoryinstances incli/tools/bundle/mod.rs:--checkblock builds a type-check module graph, which fetches/dev/stdinand drains the pipe.bundle_init()then creates a second factory, and esbuild'son_loadre-fetches/dev/stdin→ hits EOF → gets empty content → esbuild emits nothing.There was also a platform wrinkle:
resolve_url_or_path_absolutecanonicalizes entrypoints. On Linuxcanonicalize("/dev/stdin")errors (realpath → nonexistent/proc/PID/fd/pipe:[inode]); on macOS it rewrites/dev/stdin→/dev/fd/0; a symlink rewrites to its target. Any of these makes the bundle flow's specifier diverge from the--checkflow'scollect_specifiers(which never canonicalizes), so the two flows read the pipe under different keys.Fix
read_nonregular_entrypoints()reads any entrypoint that resolves to a non-regular local file (fifo/char device/socket, e.g./dev/stdinwhen stdin is a pipe) exactly once into aVec<File>, deduping by resolved URL so a duplicated entrypoint (/dev/stdin /dev/stdin) is still read only once. The--checkblock registers these in the check factory's file fetcher and threads the same vec intobundle_init(), which registers them in the bundle factory's fetcher.deno_cache_dir's fetcher consults its in-memory files before touching the filesystem, so neither path re-reads the drained pipe. Otherbundle_initcallers (compile /provider.rs) pass&[].resolve_url_or_path_absolutenow checksmetadatafirst and returns the uncanonicalizedfile://URL for any non-regular, non-directory entry (metadata follows symlinks, so a symlink→FIFO is caught too), only canonicalizing regular files/dirs. This keeps a single stable specifier across the check flow, the bundle flow, and the in-memory key. Genuinely-missing paths still error as before.Tests
tests/specs/bundle/stdin_check/pipes source via stdin and assertsbundle --check /dev/stdinandbundle /dev/stdinemit the same bundle.tests/integration/bundle_tests.rs,#[cfg(unix)]) exercise the canonicalize-rewrite path via a symlink→FIFO with a bounded timeout (a regression fails fast instead of hanging CI): one assertsbundle --check <symlink>produces the bundle without re-reading the drained FIFO; one asserts a duplicated symlink→FIFO entrypoint is read only once; and one pipes a type error and assertsbundle --check=allfails with TS2322, proving the type-checker actually reads the piped content (an EOF/empty module would pass).Existing
bundle/checkspecs still pass.tools/format.jsandtools/lint.js(full Rust clippy) are clean.Before:
printf 'const x=5;\nconsole.log(x);\n' | deno bundle --check /dev/stdin→ silent/empty (andNo such file or directoryon Linux).After: emits the bundled JS identically with and without
--check.