Better dumper cache - #254
Conversation
|
![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: c7e154514d
ℹ️ 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.
🟡 Changes recommended
The new cache publish/install logic can incorrectly accept or reuse corrupted cache entries (including wrong-type destinations) without validation, which can break resource initialization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors the clang-dumper resource cache to be more robust and shareable across runs/processes, introducing a maintenance lock and staging/publish workflow to avoid partial/corrupt cache artifacts while enabling shared include extraction by SHA-256.
Changes:
- Introduces
CacheFilesto centralize cache locking, staging directories, atomic publishing, stale cleanup, and SHA-256 verification for downloaded artifacts. - Reworks
ClangResourcesto use areleases/layout and a shared includes cache keyed by includes archive SHA-256, with maintenance-locked “touch” usage tracking and stale cleanup. - Updates/expands
ClangResourcesTestto cover shared includes extraction, maintenance-lock behavior across JVMs, staging cleanup, and cache reuse semantics.
File summaries
| File | Description |
|---|---|
| ClangAstParser/test/pt/up/fe/specs/clang/ClangResourcesTest.java | Replaces old cache-lock tests with broader coverage for maintenance lock, shared includes-by-SHA, staging cleanup, and reuse behaviors. |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangResources.java | Switches to releases/includes cache layout, introduces shared includes resolution and maintenance-locked usage tracking + cleanup. |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangAstWebResource.java | Uses CacheFiles.installFile() for manifest caching under the new cache-root layout. |
| ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java | New cache utility implementing maintenance lock, staging/publish, checksum validation, and stale/staging cleanup. |
Review details
Suppressed comments (1)
ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java:188
publish()returns immediately if the destination exists, even if it has the wrong type (e.g., a directory where a file is expected, or vice-versa). This can cause callers likeinstallFile()to proceed with an unusable cache entry and hide the underlying corruption.
static Path publish(Path staging, Path destination) {
try {
Files.createDirectories(destination.getParent());
if (Files.exists(destination)) {
return destination;
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
c7e1545 to
2dee23b
Compare
|
No description provided.