fix(cli): freeze sourcemap process selection across inject and upload - #91191
fix(cli): freeze sourcemap process selection across inject and upload#91191cat-ph wants to merge 2 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Prompt To Fix All With AI### Issue 1
cli/src/sourcemaps/args.rs:139-144
**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.Reviews (1): Last reviewed commit: "fix(cli): bound the injecting-selection ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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() { |
There was a problem hiding this comment.
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.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b19c12f32a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bail!("No files found in {resolved}"); | ||
| } | ||
| Ok(Self { | ||
| directory: files, |
There was a problem hiding this comment.
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 👍 / 👎.
| .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.
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 👍 / 👎.
| Ok(Self { | ||
| directory: files, | ||
| stdin: false, | ||
| include: Vec::new(), | ||
| exclude: Vec::new(), |
There was a problem hiding this comment.
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 👍 / 👎.
|
I think #91427 is a better approach actually |
1 participant
Problem
posthog-cli sourcemap processcan abort a user's production build withChunk ID not found(exit 1) when something writes into the scanned directory while the command runs. Next.js 16.3+ users hit this on every build: Turbopack's filesystem cache (on by default there) keeps flushing files into.nextin the background while@posthog/nextjs-configruns the CLI, reported in posthog-js#4667.The cause is a time-of-check/time-of-use race inside
process: the inject pass and the upload pass each re-walk the directory roots independently. A chunk pair that materializes between the walks reaches upload without an injected chunk ID, and one such pair fails the entire upload.Changes
sourcemap processnow expands the file selection into a concrete file list once (stdin resolved, include/exclude globs applied) and hands the identical list to inject and upload, so a build no longer fails when files appear mid-run. Late files are consistently outside the run instead of half-processed.Chunk ID not founderror now names the offending file.injecting selectionlog line is bounded by entry count and byte budget instead of printing every selected path. A materialized selection used to render as one ~1 MB line, which killed the CLI with EAGAIN when stderr was a non-blocking pipe (e.g. spawned from Node.js with inherited stdio), failing the build.ProcessArgs::resolve_stdinbecameProcessArgs::materialize; standalonesourcemap inject/sourcemap uploadkeep their existing selection behavior.Companion fix on the plugin side: posthog-js#4681 makes
@posthog/nextjs-configpass an explicit file snapshot, which fixes the issue for already-shipped CLI versions.How did you test this code?
materialize_freezes_the_file_set: materializes a selection, writes a new file into the directory, and asserts the selection still resolves to the original set — this reproduces the regression class (it fails against the old per-pass walk).materialize_fails_on_empty_selectioncovers the empty-directory error path.display_is_bounded_for_large_selections/display_is_bounded_for_long_pathspin the log-line bound for 10k paths and for long path names.process_materialization_keeps_every_filepins that the frozen selection keeps every file: asourceMappingURLcan point at any filename, and upload's--delete-aftercleanup guard derives its allowed roots from the parents of the selected files, so filtering to "candidates" would silently shrink the guard's coverage.cargo testfor the CLI crate,cargo clippy --all-targets, andcargo fmtare green locally.next buildagainst a live PostHog instance with this CLI build.Automatic notifications
Docs update
Sampo changeset included (
cli/.sampo/changesets/sourcemap-process-frozen-selection.md); no docs describe the two-pass internals.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
runAfterProductionCompileruns right after compile while the Turbopack shutdown/cache-flush promise is awaited only at the end of the build, so background writes overlap the hook; unchanged on canary.