Skip to content
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

[V3] Fix build indeterminism by sorting entries #319

Merged
merged 11 commits into from
Feb 7, 2025

Conversation

mattcompiles
Copy link
Contributor

@mattcompiles mattcompiles commented Jan 31, 2025

Motivation

When building with multiple entries, builds could produce indeterministic outputs as the entries were attached to the graph in random order (due to concurrency). While entry order is not really an issue, it could cause Assets shared between entries to be visited earlier/later when running a DFS. This caused bundles to be changed between builds in some cases, for example manual shared bundle Asset order.

To resolve this, entries are now attached to the graph after the AssetGraph is built and in a sorted order (alphabetical by entry name).

Changes

  • Attach entries to the AssetGraph after it's complete and in a sorted order
  • Ensure we don't add duplicate edges to the graph when handling pending Assets
  • Add tracing to serialize_asset_graph

Checklist

  • Existing or new tests cover this change

@@ -131,6 +133,15 @@ impl AssetGraphBuilder {
}
}

// Connect the entries to the root node in the graph. We do this in
// alphabetical order so it's consistent between builds.
self
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem isn't on this graph, is on the other parts of the codebase which are depending on the order these edges and nodes exist in.

There is one known place which does this added in https://github.com/parcel-bundler/parcel/pull/9706/files

Here, instead of assetGraph.dfsFast which will depend on the order of nodes on the graph, we should be sorting them by some preset value (like ID or path). This is tracked by AFB-406 since that change was made.

I'd rather we try to fix that first. You would need to sort in that part instead of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I attempted to fix the individual places we rely on edge order but it became a risky and complicated fix that will take time to get right.

I've added a comment to explain the situation and we can look to remove the ordering code once the bundler has been replaced.

@mattcompiles mattcompiles enabled auto-merge (squash) February 7, 2025 04:12
@mattcompiles mattcompiles merged commit 91eaa8d into main Feb 7, 2025
18 checks passed
@mattcompiles mattcompiles deleted the matt/fix-native-determinism branch February 7, 2025 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants