diff --git a/cli/.sampo/changesets/sourcemap-process-frozen-selection.md b/cli/.sampo/changesets/sourcemap-process-frozen-selection.md new file mode 100644 index 000000000000..85291340c076 --- /dev/null +++ b/cli/.sampo/changesets/sourcemap-process-frozen-selection.md @@ -0,0 +1,5 @@ +--- +cargo/posthog-cli: patch +--- + +Fix a race in `sourcemap process`: the file selection is now expanded into a concrete file list once and shared by the inject and upload passes. Previously each pass re-walked directory roots, so a bundler writing into the scanned directory mid-run (e.g. Turbopack's background filesystem-cache flush on Next.js 16.3+) could hand the upload pass chunks the inject pass never stamped, aborting the whole run with "Chunk ID not found". That error now also names the offending file. The "injecting selection" log line is now bounded instead of printing every selected path — a large selection used to produce a log line big enough to kill the CLI when stderr was a non-blocking pipe (e.g. spawned from Node.js). diff --git a/cli/src/commands.rs b/cli/src/commands.rs index 92f7e39ebd55..27aee25d9f26 100644 --- a/cli/src/commands.rs +++ b/cli/src/commands.rs @@ -413,7 +413,7 @@ impl Cli { crate::sourcemaps::plain::upload::upload(&upload_args, None)?; } SourcemapCommand::Process(args) => { - let (inject_args, upload_args) = args.resolve_stdin()?.into(); + let (inject_args, upload_args) = args.materialize()?.into(); let cwd = std::env::current_dir().context("Failed to determine current directory")?; let release = crate::sourcemaps::inject::get_release_for_maps( diff --git a/cli/src/sourcemaps/args.rs b/cli/src/sourcemaps/args.rs index c2708fcf4450..67b8ed77fa7d 100644 --- a/cli/src/sourcemaps/args.rs +++ b/cli/src/sourcemaps/args.rs @@ -54,9 +54,38 @@ impl TryFrom for FileSelection { } } +/// Caps on the selection roots Display renders. A materialized selection can hold +/// thousands of paths, and printing them all produces a log line larger than the pipe +/// buffer (16-64 KiB depending on the platform), which kills the process with EAGAIN +/// when stderr is a non-blocking pipe (e.g. inherited from a Node.js parent, as +/// `@posthog/nextjs-config` does). The count cap keeps the line readable; the byte +/// budget bounds it even when the individual paths are long. +const MAX_DISPLAYED_ROOTS: usize = 8; +const MAX_DISPLAY_BYTES: usize = 1024; + impl Display for FileSelectionArgs { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{:?}", self.directory) + let mut rendered = String::from("["); + let mut shown = 0usize; + for path in self.directory.iter().take(MAX_DISPLAYED_ROOTS) { + let formatted = format!("{path:?}"); + // Always render the first path (a single path is bounded by PATH_MAX). + if shown > 0 && rendered.len() + formatted.len() + 2 > MAX_DISPLAY_BYTES { + break; + } + if shown > 0 { + rendered.push_str(", "); + } + rendered.push_str(&formatted); + shown += 1; + } + rendered.push(']'); + let omitted = self.directory.len() - shown; + if omitted > 0 { + write!(f, "{rendered} … and {omitted} more") + } else { + write!(f, "{rendered}") + } } } @@ -91,6 +120,37 @@ impl FileSelectionArgs { } Ok(self) } + + /// Expand every root into the concrete files it currently contains, applying the + /// include/exclude globs, so repeated consumers see an identical set. `process` needs + /// this: its inject and upload passes each re-walk directory roots, and a bundler + /// writing into the directory between the passes (e.g. Turbopack's background + /// filesystem-cache flush on Next.js 16.3+) would otherwise hand upload chunks that + /// inject never stamped, failing the run with "Chunk ID not found". + /// See https://github.com/PostHog/posthog-js/issues/4667 + /// + /// Every file is kept, not just chunk candidates: a `sourceMappingURL` can point at + /// any filename, and upload's `--delete-after` cleanup guard authorizes deletions by + /// the parent directories of the selected files, so dropping "irrelevant" files here + /// would silently shrink what that guard covers. + pub fn materialize(self) -> Result { + let resolved = self.resolve_stdin()?; + resolved.validate()?; + let files = FileSelection::try_from(resolved.clone())? + .into_iter() + .filter(|entry| entry.file_type().is_file()) + .map(|entry| entry.into_path()) + .collect::>(); + if files.is_empty() { + bail!("No files found in {resolved}"); + } + Ok(Self { + directory: files, + stdin: false, + include: Vec::new(), + exclude: Vec::new(), + }) + } } /// How exceptions get associated with a release. @@ -262,6 +322,83 @@ mod tests { parsed } + #[test] + fn materialize_freezes_the_file_set() { + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("chunk.js"), "content").unwrap(); + + let args = FileSelectionArgs { + directory: vec![dir.path().to_path_buf()], + stdin: false, + include: Vec::new(), + exclude: Vec::new(), + }; + let materialized = args.materialize().unwrap(); + + // A chunk written after materialization (a bundler flushing files in the + // background between the inject and upload passes) must not enter the selection. + fs::write(dir.path().join("late.js"), "content").unwrap(); + + let selected: Vec<_> = FileSelection::try_from(materialized) + .unwrap() + .into_iter() + .map(|entry| entry.into_path()) + .collect(); + assert_eq!(selected, vec![dir.path().join("chunk.js")]); + } + + #[test] + fn materialize_fails_on_empty_selection() { + let dir = tempfile::tempdir().unwrap(); + let args = FileSelectionArgs { + directory: vec![dir.path().to_path_buf()], + stdin: false, + include: Vec::new(), + exclude: Vec::new(), + }; + assert!(args.materialize().is_err()); + } + + #[test] + fn display_is_bounded_for_large_selections() { + let args = FileSelectionArgs { + directory: (0..10_000) + .map(|i| PathBuf::from(format!("static/chunks/chunk-{i}.js"))) + .collect(), + stdin: false, + include: Vec::new(), + exclude: Vec::new(), + }; + + let rendered = args.to_string(); + + // Keep the rendered selection far under the smallest pipe buffer (16 KiB): a + // single oversized log line kills the CLI when stderr is a non-blocking pipe. + assert!(rendered.len() < 2048, "rendered {} bytes", rendered.len()); + assert!( + rendered.ends_with("… and 9992 more"), + "rendered: {rendered}" + ); + } + + #[test] + fn display_is_bounded_for_long_paths() { + let long_segment = "a".repeat(300); + let args = FileSelectionArgs { + directory: (0..10_000) + .map(|i| PathBuf::from(format!("{long_segment}/chunk-{i}.js"))) + .collect(), + stdin: false, + include: Vec::new(), + exclude: Vec::new(), + }; + + let rendered = args.to_string(); + + assert!(rendered.len() < 2048, "rendered {} bytes", rendered.len()); + assert!(rendered.ends_with("more"), "rendered: {rendered}"); + } + fn make_args(name: Option<&str>, version: Option<&str>, build: Option<&str>) -> ReleaseArgs { ReleaseArgs { name: name.map(String::from), diff --git a/cli/src/sourcemaps/plain/mod.rs b/cli/src/sourcemaps/plain/mod.rs index cfe039ef8c37..574613263494 100644 --- a/cli/src/sourcemaps/plain/mod.rs +++ b/cli/src/sourcemaps/plain/mod.rs @@ -65,9 +65,11 @@ pub struct ProcessArgs { } impl ProcessArgs { - /// Resolve stdin paths once so they can be shared between inject and upload. - pub fn resolve_stdin(mut self) -> Result { - self.file_selection = self.file_selection.resolve_stdin()?; + /// Freeze the file selection into a concrete file list once, so inject and upload + /// operate on the exact same set even when the scanned directory keeps changing + /// underneath us (e.g. Turbopack's background cache flush on Next.js 16.3+). + pub fn materialize(mut self) -> Result { + self.file_selection = self.file_selection.materialize()?; Ok(self) } } @@ -125,6 +127,40 @@ mod tests { assert_eq!(args.upload_concurrency.concurrency.get(), 18); } + #[test] + fn process_materialization_keeps_every_file() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("chunk.js"), "content").unwrap(); + std::fs::write(dir.path().join("app.css"), "content").unwrap(); + std::fs::create_dir(dir.path().join("maps")).unwrap(); + // A sourceMappingURL can point at any filename, and upload's `--delete-after` + // cleanup guard is derived from the parents of the selected files — filtering + // the frozen selection down to "candidates" would shrink what it covers. + std::fs::write(dir.path().join("maps/app.custommap"), "{}").unwrap(); + + let parsed = SourcemapCli::try_parse_from([ + "test", + "process", + "--directory", + dir.path().to_str().unwrap(), + ]) + .expect("process args should parse"); + let SourcemapCommand::Process(args) = parsed.command else { + panic!("expected process command"); + }; + + let mut selected = args.materialize().unwrap().file_selection.directory; + selected.sort(); + assert_eq!( + selected, + vec![ + dir.path().join("app.css"), + dir.path().join("chunk.js"), + dir.path().join("maps/app.custommap"), + ] + ); + } + #[test] fn upload_accepts_concurrency_override() { let parsed = SourcemapCli::try_parse_from([ diff --git a/cli/src/sourcemaps/source_pairs.rs b/cli/src/sourcemaps/source_pairs.rs index e66fbeaa5c2b..740ef343fed6 100644 --- a/cli/src/sourcemaps/source_pairs.rs +++ b/cli/src/sourcemaps/source_pairs.rs @@ -158,9 +158,12 @@ impl SourcePair { /// In symbol-set mode no hash is set and the upload layer hashes the raw payload, matching /// the hashes the server already stores for previous uploads. pub fn into_upload(mut self, release_mode: ReleaseMode) -> Result { - let chunk_id = self - .get_chunk_id() - .ok_or_else(|| anyhow!("Chunk ID not found"))?; + let chunk_id = self.get_chunk_id().ok_or_else(|| { + anyhow!( + "Chunk ID not found in {} — the file was not injected before upload", + self.source.inner.path.display() + ) + })?; let release_id = self.sourcemap.get_release_id(); let source_content = self.source.inner.content.clone(); let sourcemap_content = serde_json::to_string(&self.sourcemap.inner.content)?;