Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Validation can still delete concurrently updated cache data, and repeated tryLock() calls can leak a held lock.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves mirror reliability by preventing concurrent cache writes, eliminating duplicate cron execution, and fixing container log forwarding.
Changes:
- Adds Java and shell locking with tests.
- Corrects dcron configuration to prevent duplicate jobs.
- Replaces direct stdout access with a FIFO log drain.
File summaries
| File | Description |
|---|---|
Dockerfile |
Adds shared helpers and FIFO logging. |
src/docker/apache/mirror.conf |
Blocks HTTP access to the lock file. |
src/docker/crontab/mirror |
Routes cron output through the FIFO. |
src/docker/scripts/common.sh |
Implements shared flock locking. |
src/docker/scripts/epss.sh |
Adopts shared locking. |
src/docker/scripts/kev.sh |
Adopts shared locking. |
src/docker/scripts/mirror.sh |
Replaces stale-file locking. |
src/docker/scripts/validate.sh |
Documents an unresolved validation race. |
src/docker/supervisor/supervisord.conf |
Adds FIFO draining and corrects dcron paths. |
src/main/java/io/github/jeremylong/vulnz/cli/cache/DirectoryLock.java |
Adds cache-directory locking. |
src/main/java/io/github/jeremylong/vulnz/cli/services/NvdMirrorService.java |
Locks mirror processing. |
src/test/java/io/github/jeremylong/vulnz/cli/cache/DirectoryLockTest.java |
Tests lock lifecycle behavior. |
src/test/java/io/github/jeremylong/vulnz/cli/services/NvdMirrorServiceTest.java |
Tests lock-conflict exit handling. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@jeremylong I can review this when I return to the office tomorrow. |
|
I'm still working through the Copilot review. The validate.sh TODO turned into a thing. I don't like that hiccups blast the entire cache, and also figured that proper validation should fix #405 . But that problem turned out to be worse than stated - a year file can be partially recreated and will remain so unnoticed. Keeping track of year files being proper requires storing hashes, which then requires updates to be transaction-like so that incomplete writes don't invalidate the hash. But NVD can be very slow, laggy and times often, so caching should at least "commit" a single file once that's downloaded, or else it may never complete. At this point this is still too mooshy, I'm trying to get through tying it all up. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
All right, the validation is now re-implemented in the main mirror code. The original method was really blunt, but that wasn't the primary reason for complicating things like this change does, really fixing #405 is. The new implementation now also stages all changes, protecting the cache from crashes, incomplete writes, etc. Year files that fail validation (because it's corrupt in any way), are re-downloaded, but only the affected files are. As an unfortunate side-effect, all year files will be re-downloaded when this change is pushed on any existing deployments. However, because of #405, the validity of these files can not truly be established. The rest of the change, like weeding out "impossible year files", or dealing with potentially 0 vulnerabilities in a year are just to shore up the stability, even if those are far-fetched. One thing I'm not particularly happy about is the lack of logging, the container does't say about what it's doing on updates, but I generally followed what the existing code did. |
There was a problem hiding this comment.
🟡 Changes recommended
Lock-channel handling and staged metadata merging can defeat exclusion or publish inconsistent cache claims.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 3
- Review effort level: Balanced
| } catch (OverlappingFileLockException exception) { | ||
| // another thread or object in this JVM already holds it | ||
| lock = null; | ||
| } catch (IOException exception) { |
| try (OutputStreamWriter osw = new OutputStreamWriter(output, StandardCharsets.UTF_8); | ||
| PrintWriter writer = new PrintWriter(osw)) { |
| for (String key : properties.stringPropertyNames()) { | ||
| stagedProperties.setProperty(key, properties.getProperty(key)); |
|
@veselov I really appreciate your work on this. Thank you... Any opinion on the latest copilot review? |
Apologies, I've been on the road, let me take a look at those. |
Resolves #430 and #432
The main culprit was the duplicate cron jobs because of the way dcron was configured. Logging output permissions obscured that heavily. I ended up clearly seeing the duplicate runs from a set up BPF filter.
The fix: