-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(cli): freeze sourcemap process selection across inject and upload #91191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,9 +54,38 @@ impl TryFrom<FileSelectionArgs> 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<Self> { | ||
| 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::<Vec<_>>(); | ||
| if files.is_empty() { | ||
| bail!("No files found in {resolved}"); | ||
| } | ||
| Ok(Self { | ||
| directory: files, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When any file disappears after Useful? React with 👍 / 👎. |
||
| stdin: false, | ||
| include: Vec::new(), | ||
| exclude: Vec::new(), | ||
|
Comment on lines
+147
to
+151
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With Useful? React with 👍 / 👎. |
||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// 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), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<SymbolSetUpload> { | ||
| 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", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When standalone upload encounters an uninjected source-map pair, this new error states the cause but does not tell the user how to recover, and it uses the explicitly disallowed em dash. Use direct sentences that instruct the user to run AGENTS.md reference: AGENTS.md:L239-L242 Useful? React with 👍 / 👎. |
||
| 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)?; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a selected chunk is overwritten or its sourcemap is written after injection but before upload, the two passes read different source-pair states at the same frozen path, causing
processto abort withChunk ID not found.Prompt To Fix With AI