Add fingerprint-based module caching - #3576
Conversation
|
@codex review |
GitHub Actions
Failure
![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: 453e043945
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add fingerprint-based module caching (#3576)
Overall this is a well-designed feature. It reuses the existing IModuleResultRepository extension point cleanly rather than inventing a parallel mechanism, the fingerprint composition (module identity + inputs + key parts + env vars + dependency results) is thorough, the filesystem/S3/Redis stores all use atomic write patterns (temp file + rename, or write-chunks-then-publish-metadata), and the test suite covers the tricky cases well (fingerprint captured pre-execution even when the module mutates its own input, dependency value changes invalidating dependents, glob file-count limits). Docs are clear and the "correctness rules" section sets the right expectations for users.
A few things worth addressing before merge:
1. _fingerprints dictionary leaks entries whenever a cached module doesn't finish Successful (src/ModularPipelines/Caching/ModuleCacheResultRepository.cs:61, 109, 127, 169)
GetResultAsync stores the computed fingerprint in _fingerprints[module] on a cache miss (line 127) so SaveResultAsync can reuse it later instead of re-hashing inputs. But SaveResultAsync only removes that entry inside the try/finally at line 109 — and it early-returns at line 61 before reaching that finally whenever moduleResult.ModuleStatus != Status.Successful. Looking at the callers in ModuleExecutionPipeline: SaveToHistory is invoked for Successful and IgnoredFailure results, but for a plain Failed result it's never called at all. So for any cache-enabled module that fails (with or without an ignore-failure condition), the dictionary keeps a strong reference to that IModule for the remainder of the repository's lifetime.
Why this matters: it's not just a style nit — it's the guard clause silently defeating the cleanup the finally block was written to guarantee. In a typical one-shot CLI pipeline run the blast radius is small (process exits anyway), but it will bite in any host that reuses the same DI container/repository across multiple pipeline executions (e.g. a persistent build server), and it's the kind of latent bug that's easy to reintroduce again if someone refactors this method without noticing the ordering dependency.
Suggestion: wrap the whole method body in try/finally (or use _fingerprints.TryRemove unconditionally in a finally at the top of the method) so cleanup doesn't depend on which branch returns. That also removes the implicit coupling between "when does this get called" in ModuleExecutionPipeline and "when does this get cleaned up" in the repository.
2. Fingerprint validation is copy-pasted across all three store implementations
FileSystemModuleCache.GetEntryPath (src/ModularPipelines/Caching/FileSystemModuleCache.cs:66), S3ModuleCache.ValidateFingerprint (src/ModularPipelines.Distributed.Artifacts.S3/Caching/S3ModuleCache.cs:109-114), and RedisModuleCache.ValidateFingerprint (src/ModularPipelines.Distributed.Redis/Caching/RedisModuleCache.cs:191-196) all contain the identical fingerprint.Length != 64 || fingerprint.Any(c => !Uri.IsHexDigit(c)) check and the same error message.
Why this matters: these three packages already share IModuleCacheStore from ModularPipelines.Caching. Since the S3 and Redis projects already reference that assembly, a single internal static class ModuleCacheFingerprint { public static void Validate(string fingerprint) } (or make it public since it's a useful guard for third-party IModuleCacheStore implementations too) in the core package would let all three call the same method. Right now, fixing or extending the validation rule (e.g. supporting a future non-SHA-256 hash, or improving the exception message) requires three synchronized edits, and it's easy for one to drift.
3. Undocumented trust in mtime+size for skipping re-hashes (src/ModularPipelines/Caching/ModuleCacheFileHasher.cs:42-43)
HashAsync skips re-hashing a file when its Length and LastWriteTimeUtc.Ticks match the persisted index — a reasonable, common optimization (make/ccache/incremental-build tools all do this), but it means the fingerprint doesn't strictly reflect current file content as the docs (docs/docs/how-to/module-caching.md:44-49) state — it reflects content-as-of-last-observed-mtime-change. A tool that rewrites a file in place while preserving both size and timestamp (some editors, some VCS checkout operations, or filesystems with coarse timestamp resolution) would produce a silent stale cache hit rather than a cache miss, which is the worse failure mode for a caching feature explicitly built around a "Correctness rules" section.
Suggestion: not necessarily a blocker, but the "Correctness rules" doc section should call this trade-off out explicitly (the same way build tools that make this trade-off document it), so users know why touching a file without changing bytes is safe but some external tools that preserve stat metadata across writes are not.
4. Duplicate/garbled XML doc summaries (multiple files)
Several new constructors have both the British and American spelling of the summary line, e.g.:
src/ModularPipelines/Caching/FileSystemModuleCache.cs:13-14src/ModularPipelines.Distributed.Artifacts.S3/Caching/S3ModuleCache.cs:20-21src/ModularPipelines.Distributed.Redis/Caching/RedisModuleCache.cs:20-21src/ModularPipelines/Attributes/CacheInputsAttribute.cs:19-20
/// Initialises a new instance of the <see cref="FileSystemModuleCache"/> class.
/// Initializes a new instance of the <see cref="FileSystemModuleCache"/> class.Looks like leftover from an automated spelling pass that appended rather than replaced. Worth a quick cleanup pass before merge since these are public API docs.
Nice addition overall — the store abstraction is clean and the S3/Redis backends correctly avoid coupling cache keys to distributed run identifiers so they're safely shareable across concurrent runs. Items 1 and 2 are the ones I'd actually want fixed before merge; 3 and 4 are lower priority polish.
|
Addressed all review findings in 92e89aa: guaranteed fingerprint cleanup after failed/ignored modules, centralized SHA-256 validation, documented file hash-index metadata tradeoff, removed duplicate XML docs, and replaced reflection-based hash-index JSON with source-generated metadata for analyzer/AOT safety. Validation: core Release build (0 errors), ModuleCacheTests 6/6, ModuleExecutionPipelineTests 2/2, S3 cache tests 3/3, Redis cache tests 3/3. The prior Ubuntu pipeline failure was unrelated flaky CommandLoggerTests.Fast_Command_Logs_Complete_Output_When_Result_Is_Truncated; this update reruns it. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9250bfa1da
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Follow-up review: fingerprint-based module caching (#3576)
Re-reviewed after the fix(caching): harden cache correctness (b772837) and Merge remote-tracking branch 'origin/main' (8a857f6) commits, checking against the four items raised in my prior review on commit 453e043945.
Previously flagged items — status
-
_fingerprintsdictionary leak on failed modules — ✅ Fixed.ModuleCacheResultRepository.SaveResultAsyncnow wraps its whole body intry/finallywithDiscardFingerprint(module)in thefinally(src/ModularPipelines/Caching/ModuleCacheResultRepository.cs:63-120), andModuleExecutionPipelinealso unconditionally calls_cacheResultRepository?.DiscardFingerprint(module)in its own outerfinally(src/ModularPipelines/Engine/ModuleExecutionPipeline.cs:202) regardless of module outcome. Cleanup no longer depends on which branch returns. -
Duplicated fingerprint validation across stores — ✅ Fixed. All three stores (
FileSystemModuleCache,S3ModuleCache,RedisModuleCache) now call the sharedModuleCacheFingerprint.Validate(...)(src/ModularPipelines/Caching/ModuleCacheFingerprint.cs). -
Undocumented mtime+size trust for skipping re-hashes — ✅ Resolved, and better than just documenting it: the hash-index shortcut was removed entirely.
ModuleCacheFileHasher.HashAsyncnow always hashes actual file content and re-hashes if size/mtime changed during hashing, to detect concurrent modification (src/ModularPipelines/Caching/ModuleCacheFileHasher.cs:29-42). The new "Correctness rules" section states this explicitly ("Input files are content-hashed on every fingerprint calculation. File size and timestamps are not treated as proof that content is unchanged."). -
Duplicate British/American XML doc summary lines — Partially cleaned up (S3ModuleCache/RedisModuleCache constructors now have a single summary line), but
FileSystemModuleCache.cs:13-14andCacheInputsAttribute.cs:19-20still have both lines. Not a blocker — the same double-line pattern already exists throughout the pre-existing codebase (CommandBuilderBase.cs,DelegateRequirement.cs,DownloadFileOptions.cs, etc.), so this reads as an established repo convention rather than a defect introduced by this PR.
New finding
Stale doc paragraph contradicts the new "Correctness rules" section it sits above (docs/docs/how-to/module-caching.md:80):
"The file limit prevents unexpectedly broad globs. File hashes use a persistent modification-time and size index; changed files are hashed concurrently."
This describes the old mtime/size hash-index design that was removed in the "harden cache correctness" commit (item 3 above) — there's no HashIndexPath/persisted index anywhere in ModuleCacheOptions or ModuleCacheFileHasher anymore. It directly contradicts the "Correctness rules" bullet 23 lines below it, which correctly states files are always content-hashed. This is exactly the kind of doc/code drift the "Correctness rules" section is meant to prevent trust in — worth a one-line fix (e.g. "The file limit prevents unexpectedly broad globs. Changed files are hashed concurrently.") before merge so readers don't walk away with the wrong mental model of cache correctness.
Other observations (non-blocking)
AddModuleCache<TStore>composing viaReplaceforIModuleCacheStore+TryAddSingletonfor everything else (src/ModularPipelines/Extensions/PipelineBuilderExtensions.cs:262-278) is a clean way to make registration order-independent and to let a user-suppliedIModuleCacheResultRepositorywin over the default — nice touch, matches the PR description's "regardless of registration order" claim.RedisModuleCachechunked writes under a freshgenerationGUID per write, with the metadata key published last, so concurrent readers never observe a half-written entry; old-generation chunks simply expire via TTL rather than being explicitly deleted — a reasonable simplicity/storage tradeoff for a cache.ModuleCacheFileResolver's glob-to-regex conversion compiles with a 1-secondmatchTimeout, which is a good defensive touch against pathological user-supplied patterns.
Overall this is in good shape — the substantive correctness issues from the last pass are resolved. I'd only ask for the one doc line above to be fixed before merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a857f618c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73f478a4d7
ℹ️ 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".
There was a problem hiding this comment.
Follow-up review: fingerprint-based module caching (#3576)
Re-reviewed after fix(caching): preserve artifact semantics (73f478a), checking the current HEAD against every item raised across my two prior review rounds and Codex's eight inline findings.
Status of all previously flagged items
From my review on 453e043945 and follow-up on 8a857f618c:
_fingerprintsdictionary leak on failed modules — ✅ still fixed (SaveResultAsync'stry/finally+ModuleExecutionPipeline's outerfinallyatModuleExecutionPipeline.cs:205).- Duplicated fingerprint validation across stores — ✅ still fixed (shared
ModuleCacheFingerprint.Validate). - Undocumented mtime+size trust for skipping re-hashes — ✅ still fixed (removed entirely; files are always content-hashed).
- Stale doc paragraph describing the old hash-index — resolved in the doc pass.
From Codex's eight P1/P2 findings on 8a857f618c, the three called out explicitly in this PR's description are now correctly implemented and covered by new tests in ModuleCacheTests.cs:
- Cache artifacts after the after-execute hook:
ModuleExecutionPipeline.cs:148-162now runsInvokeAfterExecuteAsyncbeforeSaveResults, and only setsCompletionSourceafter both complete (ModuleExecutionPipeline.cs:164-165).CacheCapturesArtifactsAfterAfterExecuteHookverifies the archived artifact reflects the post-hook file content. - Remove stale outputs before restoring cached artifacts:
RestoreArtifactsAsyncnow callsClearArtifacts(deleting everything matching the module'sProducesArtifactpatterns) before extracting the selected entry (ModuleCacheResultRepository.cs:342).CacheRestoreRemovesArtifactsAbsentFromSelectedEntryconfirmsb.txtfrom a stale run is gone after restoring theaentry. - Preserve executable modes when archiving artifacts: Unix mode is now captured into
ZipArchiveEntry.ExternalAttributeson write and reapplied viaRestoreUnixFileModeon restore (ModuleCacheResultRepository.cs:313-317,401-419), verified byCacheRestorePreservesUnixExecutableMode.
The remaining five Codex findings, which weren't mentioned in the PR description, also check out against the current code:
- Exclude cache directory from input expansion:
ComputeFingerprintAsyncandAddArtifactsAsync/ClearArtifactsall pass_options.CacheDirectoryasexcludedDirectoryintoModuleCacheFileResolver.ResolveFiles, which skips it (ModuleCacheFileResolver.cs:110-120). - Preserve existing result repositories when enabling caching:
AddModuleCachenever touchesIModuleResultRepository; it registers a separateIModuleCacheResultRepository, andModuleExecutionPipelineconsumes both independently (ModuleExecutionPipeline.cs:34-48). Registration order betweenAddResultsRepositoryandAddModuleCacheno longer matters because they're disjoint DI service types. - Pass pipeline cancellation to cache operations: the token now threads end-to-end —
TryGetCachedResult/SaveResultspassexecutionContext.ModuleCancellationTokenSource.Tokenthrough toIModuleCacheResultRepository, andFileSystemModuleCache/S3ModuleCache/RedisModuleCacheall forward it into the underlying I/O calls (GetObjectAsync,PutObjectAsync,StringGetAsync/StringSetAsyncviaWaitAsync(cancellationToken)). - Isolate Redis cache options from distributed services:
AddRedisModuleCacheuses a private keyedIConnectionMultiplexer(ModuleCacheConnectionKey) and buildsRedisModuleCachefrom locally-scoped option instances rather than registeringRedisDistributedOptions/ArtifactOptionsas unkeyed DI singletons, so it no longer collides withAddRedisDistributedCoordinator/AddRedisDistributedArtifactStore. - Isolate S3 cache configuration from artifact storage:
AddS3ModuleCachelikewise capturesS3ArtifactOptionsin a closure forS3ModuleCacheinstead of registering it in DI, so it can't collide withAddS3DistributedArtifactStore's separate registration.
I also did a clean local build of the touched files: core ModularPipelines.sln -c Release builds with 0 warnings / 0 errors on this branch.
One thing worth a sanity check, not a blocker
Moving module.CompletionSource.TrySetResult(...) to after SaveResults (ModuleExecutionPipeline.cs:157-165) is the right call for cache correctness — dependents must never observe a module as "done" before its cache/history write captures the post-hook artifact state. But SaveResults also drives the plain IModuleResultRepository history path, so this now inserts a synchronization point between completion and dependent start for every module using history persistence, not just cache-enabled ones. That's probably fine (SaveResultAsync failures are caught/logged rather than propagated, so it won't hang), but it's a behavior change with a blast radius slightly wider than the caching feature itself — worth a one-line callout in the PR description or changelog so it doesn't read as a silent latency change to existing history-repository users.
Overall
This is a clean, well-tested piece of work. The store abstraction (IModuleCacheStore) stays uniform across filesystem/S3/Redis, the fingerprint composition is thorough (module identity + inputs + key parts + env vars + dependency results/exceptions/skip-reasons), and every substantive correctness issue raised across three rounds of review has been addressed with a corresponding regression test rather than just a fix. Nothing outstanding blocks merge from my side.
|
Fixed both cache reviews: configured/dependency skips now win before cache lookup, and artifact snapshots record/restore empty directories while clearing stale empty directories. ModuleCacheTests: 15/15. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1843f68ce7
ℹ️ 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".
There was a problem hiding this comment.
Follow-up review: fingerprint-based module caching (#3576)
Re-reviewed the final commit, dbca45a081 ("fix(cache): honor skips and empty directories") plus the trailing merge (1843f68ce7, HEAD), which weren't covered by my prior two review rounds on 453e043945 / 8a857f618c / 73f478a4d7. This commit addresses the two remaining open Codex findings from that round.
Verified: the two remaining Codex findings are correctly fixed
-
"Evaluate skip conditions before accepting cache hits" — Fixed by simple reordering in
ModuleExecutionPipeline.ExecuteAsync(src/ModularPipelines/Engine/ModuleExecutionPipeline.cs:75-102): the fluent/dependency skip-decision block now runs before theconfig.CacheEnabledcache-lookup block (previously it ran after). AWithSkipWhengate now always wins over a cache hit, matching normal (non-cached) module semantics. Confirmed by the newFluentSkipConditionTakesPrecedenceOverCacheHittest, which asserts the second run reportsStatus.Skippedwith reason"gate closed"rather than reusing the cached artifact, and that the module only executed once. -
"Preserve empty directories in artifact snapshots" — Fixed via a new
ModuleCacheFileResolver.ResolveDirectories(mirrorsResolveFiles) that walks declaredProducesArtifactpatterns for directories, not just files.AddArtifactsAsyncnow writes explicit trailing-/ZIP directory entries for each resolved directory;RestoreArtifactsAsyncrecreates those directories before extracting file entries; andClearArtifactsnow also deletes directories that are empty after stale files are removed (ordered deepest-first viaOrderByDescending(path => path.Length), which is a valid proxy for depth since a child path is always longer than its parent). Confirmed by the newCacheRestorePreservesEmptyArtifactDirectoriestest — deletes the artifact tree, re-runs, and asserts the nested empty directory reappears viaUsedHistoryrestoration without re-executing the module.
Both fixes are minimal, targeted, and each ships with a regression test that would fail without the fix. I also did a clean local build and test run to double-check:
ModularPipelines.sln -c Release: 0 warnings, 0 errorsModuleCacheTests: 15/15 passedModuleExecutionPipelineTests: 3/3 passed
One non-blocking observation (new, not raised before)
ModuleCacheFileResolver.ResolveDirectories duplicates almost all of ResolveFiles (src/ModularPipelines/Caching/ModuleCacheFileResolver.cs:8-116) — pattern normalization, glob-regex construction, root-containment checks, and the limit-enforcing Add* helper are all copy-pasted with only the filesystem predicate (File.Exists/Directory.Exists) and enumeration call (EnumerateFiles/EnumerateDirectories) differing. This is the same shape of duplication flagged (and fixed, via ModuleCacheFingerprint.Validate) earlier in this PR for the three cache-store implementations. A shared private helper parameterized on Func<string,bool> exists + Func<string,IEnumerable<string>> enumerate would collapse both methods to a few lines each and remove the risk of the two traversal implementations drifting apart (e.g. if the containment-escape check ever needs a fix, someone has to remember there are two copies). Not a blocker — the two copies are currently in sync — but worth cleaning up given the precedent already set in this same PR.
Overall
This closes out every substantive issue raised across three review rounds (mine and Codex's), each with a corresponding test. Nothing outstanding blocks merge from my side; the duplication note above is a nice-to-have for a follow-up, not this PR.
|
Addressed both latest reviews at e31f4fd: successful results complete before self-awaiting after hooks while cache snapshots remain post-hook; cached directory ZIP entries now preserve Unix type/mode and restore deepest-first after extraction. ModuleCacheTests pass 15/15; DirectModuleHooksTests pass 9/9 (literal self-await regression included). @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e31f4fdbc8
ℹ️ 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".
There was a problem hiding this comment.
Follow-up review: fingerprint-based module caching (#3576)
Re-reviewed after the latest commit, e31f4fdb ("fix(cache): preserve hook and directory modes"), which addresses my prior P1 finding about await this deadlocking in a self-awaiting OnAfterExecuteAsync hook, plus the Unix directory-mode preservation follow-up. The directory-mode fix looks solid: modes are now applied to directories deepest-first (OrderByDescending(artifact => artifact.Destination.Length)) so a restrictive parent mode can never block traversal into a child before its mode is restored.
However, the deadlock fix itself introduces a new correctness regression:
OnAfterExecuteAsync result transformations are no longer visible to dependent modules
In ModuleExecutionPipeline.cs (around line 147-166), the fix now does:
moduleResult = ModuleResult<T>.CreateSuccess(result, executionContext);
executionContext.SetTypedResult(moduleResult);
module.CompletionSource.TrySetResult(moduleResult); // <-- published with PRE-hook result
afterHookInvoked = true;
moduleResult = await InvokeAfterExecuteAsync(...) // <-- hook may return a transformed result
.ConfigureAwait(false);
await SaveResults(...).ConfigureAwait(false);
executionContext.SetTypedResult(moduleResult); // <-- executionContext gets POST-hook result
// module.CompletionSource is NOT updated again
return moduleResult;Module<T>.GetAwaiter() resolves via CompletionSource.Task (src/ModularPipelines/Modules/Module.cs:271), and this is exactly what dependent modules consume — e.g. await context.GetModule<PackProjectsModule>() in PackagePathsParserModule.cs. Since TrySetResult only succeeds once, CompletionSource is now permanently pinned to the pre-hook result, while executionContext's typed result (used for history/cache saving and the pipeline summary) reflects the post-hook result. OnAfterExecuteAsync is explicitly documented as being able to "add cleanup or result transformation" and return a modified result — but any such transformation will now silently never reach a dependent module that awaits the module directly, even though the cached/saved/reported result is the transformed one. This is a real behavioral divergence between what gets persisted/reported and what dependents observe, not just an edge case.
The existing test added for this fix (OnAfterExecuteAsync_Can_Await_Its_Own_Module) doesn't catch this because the hook just does return await this; (returning the same value already published), and the older ResultModifyingModule test explicitly notes "We can't easily modify the internal result here, so we just verify the hook was called" — so there's no coverage asserting a genuinely transformed result propagates to a dependent module.
Suggested fix: keep publishing the pre-hook result early to unblock self-awaiting hooks (that part is correct), but if the hook actually returns a different result, republish it — e.g. reset/replace CompletionSource's value isn't possible on TaskCompletionSource once set, so this likely needs a different mechanism: either (a) only pre-publish when the hook is detected as self-awaiting (hard to detect generically), or (b) have dependents read the result through executionContext's typed-result task instead of module.CompletionSource so there's a single source of truth that's updated once, after the hook completes, with CompletionSource reserved only for unblocking a self-await. Option (b) is preferable because it keeps a single authoritative result path (cache/history/summary/dependents all read the same post-hook value) rather than two diverging result channels.
Everything else from earlier rounds (fingerprint cleanup ordering, skip-before-cache-lookup, empty-directory artifact handling, executable bit preservation) still looks correctly addressed at this commit.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e52a73f19
ℹ️ 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".
Review: Add fingerprint-based module caching (#3576)Reviewed the full diff ( OverviewThis adds opt-in, content-addressed module caching ( Strengths
Findings1. Dependents can observe a different dependency value on a fresh run vs. a cache/history-hit run, if In moduleResult = ModuleResult<T>.CreateSuccess(result, executionContext);
executionContext.SetTypedResult(moduleResult);
module.CompletionSource.TrySetResult(moduleResult); // dependents unblock HERE
afterHookInvoked = true;
moduleResult = await InvokeAfterExecuteAsync(...); // can replace moduleResult.Value
await SaveResults(module, moduleResult, ...); // cache/history persist the POST-hook value
The consequence: if a module's Suggested approach: capture the value used for 2. Fingerprint only hashes direct dependency values, not the full transitive graph — worth documenting explicitly
Minor
Test coverageCoverage is strong for the storage/fingerprinting mechanics (glob limits, timestamp-vs-content hashing, Unix mode/symlink round-trips, stale-artifact clearing, empty-directory preservation, registration-order independence with the history repository). I didn't see a test exercising finding #1 (a module with a value-mutating Overall this is a well-engineered, appropriately opt-in feature with careful attention to path safety, atomicity, and cross-platform artifact fidelity. The one thing I'd want resolved or explicitly documented before merge is the pre/post-hook value visibility asymmetry in finding #1, since it's the kind of thing that will be very hard to debug in a real pipeline if it bites someone. |
Code reviewNo new commits touch the caching feature since the previous review — the only commits added since then ( That means the two findings from the earlier review are still open and unaddressed in the current code:
Neither is a blocker for architecture soundness elsewhere in the PR — the store implementations (filesystem/S3/Redis), atomic-write handling, and path/glob safety in |
|
Addressed the latest review findings in 99326ef: artifact expansion no longer follows directory reparse points or accepts exact paths through linked directories; post- |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01b330b51e
ℹ️ 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".
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2b00a01d5
ℹ️ 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".
|
@codex review |
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 287143b765
ℹ️ 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".
|
Addressed the latest artifact restoration findings in d534181: validate every archive destination against declared ProducesArtifact scopes before mutation, reject nesting beneath all archived symlinks regardless of directory metadata, and temporarily restore Unix parent-directory writability for file-only artifacts while preserving original modes. Validation: ModuleCacheTests 32/32; core Release build 0 errors; changed production files compile warning-free. @codex review |
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d534181543
ℹ️ 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".
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2920afabdd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c5205dfa5
ℹ️ 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".
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e543fe29fe
ℹ️ 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".
GitHub Actions
Failure
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7128668c0
ℹ️ 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".
|
@claude review |
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff367ff3d4
ℹ️ 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".
| foreach (var file in files) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| File.Delete(file); |
There was a problem hiding this comment.
On Windows, when a module produces a declared file with FileAttributes.ReadOnly, File.Delete throws UnauthorizedAccessException. The first cache hit therefore fails while clearing the artifact, rollback repeats the same failure, and normal execution starts with the stale read-only output still present; clear the read-only attribute before deletion, as the repository's other recursive deletion helpers already do.
Useful? React with 👍 / 👎.
| var linkedPath = GetLinkedComponent(root, searchRoot) | ||
| ?? (Directory.Exists(searchRoot) | ||
| ? EnumerateLinksWithoutFollowing(searchRoot).FirstOrDefault() | ||
| : null); |
There was a problem hiding this comment.
For a non-recursive or otherwise narrow glob, this rejects the cache merely because any link exists anywhere below the literal search root, even when that link cannot match the pattern—for example, src/*.cs is disabled by an unrelated src/assets/logo symlink. Both cache reads and writes then fall back on every run. Fresh evidence beyond the descendant-link rejection fix is that this unconditional FirstOrDefault never tests the discovered link or its reachable paths against the actual glob.
Useful? React with 👍 / 👎.
1 participant
Closes #3536.
Adds opt-in, fingerprint-based persistent caching for modules through
[CacheInputs]and[ProducesArtifact], with pluggable stores, local filesystem persistence, automatic result/artifact restoration, input hashing, and cache registration APIs.Review follow-ups ensure:
OnAfterExecuteAsyncand before dependents observe completion;Validation:
ModuleCacheTests: 13/13 passedModuleExecutionPipelineTests: 3/3 passedModularPipelines.slnRelease build: 0 warnings, 0 errorsThe Unix permission assertion runs on Unix CI; it is intentionally skipped on Windows.
Cache skip and empty-directory follow-up
ModuleCacheTests: 15/15.