Local Build scratch directory should be created in /tmp/ instead of in the project root#10541
Local Build scratch directory should be created in /tmp/ instead of in the project root#10541falahat wants to merge 4 commits into
Conversation
…ng local builds. This now creates the files in /tmp/ rather than the workspace folder. This also deletes the folder if it exists already (due to an interrupted attempt) instead of blocking and failing.
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
There was a problem hiding this comment.
Code Review
This pull request transitions the App Hosting local build scratch directory to the system's temporary directory, utilizing a path-based hash to ensure isolation between builds. It also modifies the preparation logic to automatically remove existing scratch directories rather than failing. Review feedback highlights that the tests currently rely on hardcoded hashes which are environment-dependent and suggests providing an explicit project root in tests for better portability. Additionally, it is recommended to move loop-invariant path and hash calculations outside of the deployment loop to improve efficiency.
| expect(context.backendLocalBuilds["foo"].localBuildScratchDir).to.equal( | ||
| "/tmp/apphosting-local-build-foo-e1feae81", | ||
| ); |
There was a problem hiding this comment.
The hardcoded hash e1feae81 is brittle because it is derived from process.cwd() (since options.projectRoot is not explicitly set in the test opts). This will cause the tests to fail in environments with different directory structures, such as CI/CD pipelines or other developers' local machines. To ensure consistency, consider explicitly setting a projectRoot in the opts object (e.g., projectRoot: '/my/project') or calculating the expected hash dynamically within the test suite.
| fooLocalBuild: { | ||
| outputFiles: ["./nextjs/standalone"], | ||
| localBuildScratchDir: path.join(process.cwd(), `${LOCAL_BUILD_DIR_NAME}_fooLocalBuild`), | ||
| localBuildScratchDir: "/tmp/apphosting-local-build-fooLocalBuild-e1feae81", |
There was a problem hiding this comment.
The hardcoded hash e1feae81 in the scratch directory path is brittle as it depends on the current working directory of the environment running the tests. This is likely to cause failures in CI environments. It is recommended to set an explicit projectRoot in the test configuration to guarantee a predictable hash across all environments.
| const rootDir = options.projectRoot || process.cwd(); | ||
| const localBuildScratchDir = path.join(rootDir, `${LOCAL_BUILD_DIR_NAME}_${cfg.backendId}`); | ||
| // Generate a static 8-character hash of the Workspace Root directory path to guarantee | ||
| // 100% sibling folder build isolation on the same machine, while preserving predictability. | ||
| const pathHash = crypto.createHash("md5").update(rootDir).digest("hex").substring(0, 8); |
There was a problem hiding this comment.
The rootDir and pathHash variables are loop-invariant as they only depend on the options object. Calculating them inside the for loop (which starts at line 185) is redundant. Moving these definitions outside the loop would improve efficiency and code clarity. Additionally, consider using sha256 instead of md5 to avoid potential flags from security scanners, even though cryptographic security is not a requirement for this specific use case.
Description
This was causing a really weird bug where nextjs was "escaping out of" the local build scratch folder and detecting the project folder itself as the build folder, instead of the .local_build folder inside that project folder. Apparently, this is nextjs' default root project detection for monorepos: it crawls up as long as the parent folder has a package-lock.json file.
By doing the local build in a temporary scratch folder, we avoid nextjs accidentally detecting the parent root folder.
Also, by using a scratch temp folder, we can easily delete the folder before a run starts if it wasn't cleaned up. We can be more confident that it isn't sensitive info that the user doesn't want deleted (because it is unlikely the user created a file with the same exact name + random hash in the tmp folder)
Scenarios Tested
Sample Commands