Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cli/.sampo/changesets/sourcemap-process-frozen-selection.md
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).
2 changes: 1 addition & 1 deletion cli/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
139 changes: 138 additions & 1 deletion cli/src/sourcemaps/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
}
}
}

Expand Down Expand Up @@ -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() {
Comment on lines +139 to +144

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Path snapshot leaves content race

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 process to abort with Chunk ID not found.

Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/sourcemaps/args.rs
Line: 139-144

Comment:
**Path snapshot leaves content race**

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 `process` to abort with `Chunk ID not found`.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

bail!("No files found in {resolved}");
}
Ok(Self {
directory: files,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Stop requiring every snapshotted file to survive

When any file disappears after materialize() returns—particularly while release resolution performs network work—the materialized vector is passed into injection, whose validation now requires every path to still exist and aborts the command otherwise. Because the snapshot includes every regular file, an unrelated temporary or cache file removed by a concurrently running bundler can now fail sourcemap process; previously only the user-provided root was validated and WalkDir silently skipped vanished children. Preserve the original roots for validation or do not treat snapshot members as required inputs.

Useful? React with 👍 / 👎.

stdin: false,
include: Vec::new(),
exclude: Vec::new(),
Comment on lines +147 to +151

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the original cleanup roots

With --delete-after and include/exclude filters that omit map files, replacing a directory root with only matched files narrows CSS cleanup authorization. For example, a selected dist/css/app.css referencing dist/maps/app.css.map was previously authorized by root dist, but after materialization the only relevant root is dist/css; upload succeeds while the cleanup safety check rejects the map and leaves both it and the CSS reference behind. Retain the original roots separately for cleanup validation.

Useful? React with 👍 / 👎.

})
}
}

/// How exceptions get associated with a release.
Expand Down Expand Up @@ -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),
Expand Down
42 changes: 39 additions & 3 deletions cli/src/sourcemaps/plain/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
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> {
self.file_selection = self.file_selection.materialize()?;
Ok(self)
}
}
Expand Down Expand Up @@ -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([
Expand Down
9 changes: 6 additions & 3 deletions cli/src/sourcemaps/source_pairs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add a recovery action to the chunk-ID error

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 sourcemap inject first or use sourcemap process.

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)?;
Expand Down
Loading