Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Pull request overview
This PR significantly improves test build performance by implementing in-process proposal execution and comprehensive build caching. The changes refactor the code behind agoric run to execute proposal builder scripts in-process (rather than via shell) and add a sophisticated dependency-aware caching layer that persists across test runs.
Changes:
- Introduced cross-process directory locking with stale lock recovery for build caches
- Added in-process proposal building mode with fallback to shell execution for compatibility
- Implemented dependency-aware caching with fingerprint-based invalidation
- Added opt-in Chrome Trace Event format profiling for boot tests
- Consolidated atomic file write utilities and bundle cache tooling
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/internal/src/build-cache.js | New directory locking and atomic write utilities for cross-process coordination |
| packages/internal/src/build-cache-types.ts | TypeScript types for build cache events and powers |
| packages/internal/test/build-cache.test.js | Comprehensive tests for directory locking and atomic writes |
| packages/agoric-cli/src/proposals.js | New in-process proposal builder with shell fallback |
| packages/deploy-script-support/src/writeCoreEvalParts.js | Added onWriteCoreEval callback for in-process material capture |
| packages/deploy-script-support/src/helpers.js | Wired through new endowments for onWriteCoreEval, log, writeFile |
| packages/deploy-script-support/src/cachedBundleSpec.js | Updated to use writeFileAtomic |
| packages/boot/tools/supports.ts | Major refactoring with proposal cache, profiling, and build modes |
| packages/boot/test/tools/proposal-extractor-cache.test.ts | Tests for proposal cache hit/miss and invalidation |
| packages/boot/README.md | Documentation for test profiling feature |
| packages/SwingSet/tools/bundleTool.js | Refactored to use shared directory locking and event system |
| packages/SwingSet/test/bundleTool.test.ts | Updated tests for new API |
| packages/agoric-cli/src/lib/bundles.js | Added null safety for optional fileName |
| packages/agoric-cli/package.json | Exported new proposals.js module |
| packages/cosmic-swingset/test/inquisitor.test.ts | Updated to use new bundleTool API |
| packages/boot/test/configs.test.js | Updated to use new bundleTool API |
| packages/SwingSet/src/controller/initializeSwingset.js | Updated to use makeNodeBundleCache directly |
| packages/SwingSet/package.json | Added TypeScript support for AVA tests |
| PLAN.md | Implementation plan document |
| @@ -0,0 +1,59 @@ | |||
| # @aglocal/boot | |||
There was a problem hiding this comment.
The README has a typo in the package name. It should be "@agoric/boot" not "@aglocal/boot". This typo in the header appears to be a simple error and should be corrected to match the actual package name used throughout the codebase.
| let atomicWriteSequence = 0; | ||
|
|
||
| /** | ||
| * @param {{ | ||
| * fs: Pick<import('node:fs/promises'), 'rename' | 'writeFile'>; | ||
| * filePath: string; | ||
| * data: string; | ||
| * now: () => number; | ||
| * pid: number; | ||
| * }} options | ||
| */ | ||
| export const writeFileAtomic = async ({ fs, filePath, data, now, pid }) => { | ||
| atomicWriteSequence += 1; | ||
| const tempPath = `${filePath}.${pid}.${now()}.${atomicWriteSequence}.tmp`; |
There was a problem hiding this comment.
The atomicWriteSequence is a module-level mutable variable that is not safe for concurrent use across multiple imports or module instances. If multiple parts of the codebase import this module, they will share the same sequence counter, which could lead to collisions in temporary file names despite the sequence being incremented. Consider making this a WeakMap keyed by the fs object, or better yet, use crypto.randomBytes or a UUID for uniqueness instead of a sequence number to avoid any potential race conditions.
| const queueWrite = () => { | ||
| session.writeQueue = session.writeQueue | ||
| .catch(() => {}) | ||
| .then(async () => { | ||
| const metadataEvents: BootProfileMetadataEvent[] = [ | ||
| { | ||
| name: 'process_name', | ||
| ph: 'M', | ||
| pid, | ||
| tid: 0, | ||
| ts: 0, | ||
| args: { name: 'agoric-boot-tests' }, | ||
| }, | ||
| { | ||
| name: 'thread_name', | ||
| ph: 'M', | ||
| pid, | ||
| tid: 0, | ||
| ts: 0, | ||
| args: { name: 'main' }, | ||
| }, | ||
| ]; | ||
| const traceFile: BootProfileTraceFile = { | ||
| displayTimeUnit: 'ms', | ||
| traceEvents: [...metadataEvents, ...session.events], | ||
| }; | ||
| await fs.mkdir(dirname(profilePath), { recursive: true }); | ||
| await fs.writeFile( | ||
| `${profilePath}`, | ||
| `${JSON.stringify(traceFile, null, 2)}\n`, | ||
| 'utf8', | ||
| ); | ||
| }) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
The profiler writes are queued sequentially using a promise chain pattern (session.writeQueue.catch().then()...), but errors are silently caught at lines 364 and 395. This means if a write fails (e.g., due to disk full or permissions), the error is swallowed and subsequent writes might also fail silently. While this is acceptable for optional profiling data, it would be helpful to at least log errors to console.error so developers are aware when profiling is not working correctly. Consider adding minimal error logging before the catch returns.
| const getTargetNames = (canonicalRootPath, targetName, options0) => { | ||
| const requestedTargetName = | ||
| targetName || path.basename(canonicalRootPath, '.js'); | ||
| const optionsKey = optionsKeyFor(options0); | ||
| const hashKey = createHash('sha256') | ||
| .update(`${canonicalRootPath}\n${optionsKey}`) | ||
| .digest('hex') | ||
| .slice(0, 12); | ||
| const cacheTargetName = `${sanitizeName(requestedTargetName)}-${hashKey}`; | ||
| return harden({ requestedTargetName, cacheTargetName, optionsKey }); | ||
| }; |
There was a problem hiding this comment.
The getTargetNames function computes a hash-based cache target name that includes the options (line 224-228). However, this means that bundles with different build options will have different cache target names and won't share cached bundles even if the source is identical. While this is technically correct for cache invalidation, it could lead to unnecessary bundle duplication when the same source is built with different metadata-only options that don't affect the bundle content. Consider documenting this behavior or optimizing the hash key to only include options that actually affect bundle content.
1 participant
refs: #12381
Description
This saves several seconds per
buildAndExtractin boot/RunUtils tests. It does this through refactoring the code behindagoric runto run in-process and to broaden the caching capabilities. It also refactors build caching to consolidate some of the logic. It also adds instrumentation and cleans up some of the earlier time logging.It should be reviewable by commit.
Security Considerations
Bundles under test come from cache. Some filesystem and access as the source code.
Scaling Considerations
n/a
Documentation Considerations
Should be transparent
Testing Considerations
CI. Also run locally and have confirmed no rebuilding on subsequent runs.
Upgrade Considerations
n/a