improving update template + empty folder edge case #6573
Conversation
Mzack9999
added
the
Type: Bug
|
Warning Rate limit exceeded@Mzack9999 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe template manager now returns a map of absolute written template paths from Changes
Sequence Diagram(s)sequenceDiagram
participant Updater as TemplateManager
participant Writer as writeTemplatesToDisk
participant Cleaner as cleanupOrphanedTemplates
participant Metadata as regenerateTemplateMetadata
participant FS as FileSystem
Updater->>Writer: download & write templates
activate Writer
Note right of Writer #D6EAF8: track each written absolute path\nreturn writtenPaths map
Writer->>FS: write/update files
FS-->>Writer: write results
Writer-->>Updater: writtenPaths, err?
deactivate Writer
alt write succeeded
Updater->>Cleaner: cleanup(dir, writtenPaths)
activate Cleaner
Cleaner->>FS: walk templates dir
FS-->>Cleaner: file entries
Note right of Cleaner #F9E79F: filter non-templates\nexclude custom dirs\nidentify orphaned files
Cleaner->>FS: delete orphaned files
Cleaner-->>Updater: nil or error
deactivate Cleaner
Updater->>Metadata: regenerateTemplateMetadata(dir)
activate Metadata
Metadata->>FS: ensure dirs, purge empties, rewrite index, checksum
FS-->>Metadata: success
Metadata-->>Updater: nil or error
deactivate Metadata
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/installer/template.go(9 hunks)pkg/installer/template_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/installer/template_test.gopkg/installer/template.go
🧬 Code graph analysis (2)
pkg/installer/template_test.go (2)
pkg/installer/template.go (2)
HideProgressBar(32-32)TemplateManager(70-74)pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)
pkg/installer/template.go (2)
pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)pkg/catalog/config/template.go (1)
IsTemplate(68-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Release test
- GitHub Check: Integration tests (windows-latest)
- GitHub Check: Functional tests (windows-latest)
- GitHub Check: Integration tests (ubuntu-latest)
- GitHub Check: Integration tests (macOS-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/installer/template.go (1)
204-209: Consider removing redundant deletion logic.After
cleanupOrphanedTemplates(line 180) removes orphaned templates andregenerateTemplateMetadata(line 187) rebuilds the checksum, the templates inresults.deletionshave already been removed by either:
- Line 340 (for renamed templates with the same ID)
cleanupOrphanedTemplates(for deleted templates)This deletion loop attempts to remove already-deleted files. While the
os.IsNotExistcheck makes it harmless, removing this block would simplify the logic and eliminate confusion.Consider removing lines 204-209 or adding a comment explaining why this fallback exists (e.g., if there are edge cases where cleanup doesn't catch all deletions).
pkg/installer/template_test.go (1)
440-514: Minor: Clarify test comment.Line 513 comment states "index should not contain template that was deleted but not cleaned", but
template2was actually cleaned up bycleanupOrphanedTemplates(it wasn't inwrittenPaths). The assertion is correct, but the comment could be clearer.Consider revising the comment to:
require.NotContains(t, index, "test-template-2", "index should not contain orphaned template that was cleaned up")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/installer/template.go(9 hunks)pkg/installer/template_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/installer/template_test.gopkg/installer/template.go
🧠 Learnings (1)
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
Repo: projectdiscovery/nuclei PR: 6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Applied to files:
pkg/installer/template.go
🧬 Code graph analysis (2)
pkg/installer/template_test.go (3)
pkg/installer/template.go (2)
HideProgressBar(32-32)TemplateManager(70-74)pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)pkg/catalog/config/template.go (1)
GetNucleiTemplatesIndex(114-158)
pkg/installer/template.go (3)
pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)pkg/catalog/config/template.go (2)
IsTemplate(68-82)GetNucleiTemplatesIndex(114-158)pkg/installer/util.go (1)
PurgeEmptyDirectories(75-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (7)
pkg/installer/template.go (5)
138-141: LGTM: writtenPaths appropriately discarded for fresh installs.Fresh installations don't have orphaned templates to clean up, so ignoring the returned
writtenPathsis correct.
174-192: Excellent: Metadata regeneration after cleanup addresses previous review feedback.This implementation correctly addresses the past review comment by regenerating the template index and checksum after
cleanupOrphanedTemplatesruns, ensuring metadata accurately reflects the cleaned template tree. The error handling appropriately logs warnings without failing the entire update since the core template write and cleanup operations have already succeeded.
300-408: LGTM: writeTemplatesToDisk correctly tracks written paths with proper thread safety.The signature change and implementation are sound:
SyncLockMapprovides thread safety for concurrent callback invocations- All successfully written paths are tracked (lines 338, 352)
- Error handling consistently returns
(nil, err)- Success path returns
(writtenPaths, nil)The returned map enables downstream cleanup of orphaned templates.
410-504: LGTM: cleanupOrphanedTemplates implementation is thorough and robust.The implementation correctly:
- Normalizes all paths to absolute for consistent comparison (lines 414-431)
- Excludes custom template directories (lines 434-440, 463-467)
- Uses
config.IsTemplateto identify template files (line 470)- Handles edge cases gracefully (non-existent directory, empty directory)
- Provides appropriate logging for removed files
506-537: LGTM: regenerateTemplateMetadata correctly rebuilds metadata from current disk state.The implementation properly:
- Purges empty directories before regeneration (line 510)
- Handles edge case where templates directory is purged by recreating it (lines 513-517)
- Rebuilds index by scanning current templates on disk (lines 520-529)
- Regenerates checksum file to match current state (lines 532-534)
This ensures metadata files accurately reflect the cleaned template tree.
pkg/installer/template_test.go (2)
104-374: LGTM: Comprehensive test coverage for cleanupOrphanedTemplates.The test suite thoroughly validates:
- Core functionality: orphaned template removal while preserving referenced templates
- Custom template preservation across nested directory structures
- Non-template file preservation (README, config files, checksums)
- Edge cases: empty
writtenPaths, empty directories, non-existent directories- Path handling: relative vs absolute path normalization
All scenarios include proper setup, execution, and assertions.
376-736: LGTM: Excellent test coverage for regenerateTemplateMetadata.The test suite comprehensively validates:
- Index and checksum file generation
- Integration with cleanup (ensures deleted templates excluded from metadata)
- Empty directory handling and recreation
- Directory purging behavior
- End-to-end integration test simulating the complete update flow (lines 574-662)
The integration tests effectively verify that metadata accurately reflects the cleaned template tree, which is critical for the PR's objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/installer/template.go (1)
204-209: Consider removing redundant deletion loop.The
cleanupOrphanedTemplatescall at line 180 already removes templates that aren't in the new release. This deletion loop will attempt to remove files that have already been deleted, making it effectively a no-op. While theos.IsNotExistcheck prevents errors, this code is now redundant and could be removed for clarity.If you choose to remove it, apply this diff:
// summarize all changes results := t.summarizeChanges(oldchecksums, newchecksums) - // remove deleted templates - for _, deletion := range results.deletions { - if err := os.Remove(deletion); err != nil && !os.IsNotExist(err) { - gologger.Warning().Msgf("failed to remove deleted template %s: %s", deletion, err) - } - } - // print summary
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/installer/template.go(9 hunks)pkg/installer/template_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/installer/template_test.gopkg/installer/template.go
🧠 Learnings (1)
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
Repo: projectdiscovery/nuclei PR: 6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Applied to files:
pkg/installer/template.go
🧬 Code graph analysis (2)
pkg/installer/template_test.go (3)
pkg/installer/template.go (2)
HideProgressBar(32-32)TemplateManager(70-74)pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)pkg/catalog/config/template.go (1)
GetNucleiTemplatesIndex(114-158)
pkg/installer/template.go (3)
pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)pkg/catalog/config/template.go (2)
IsTemplate(68-82)GetNucleiTemplatesIndex(114-158)pkg/installer/util.go (1)
PurgeEmptyDirectories(75-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (ubuntu-latest)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
🔇 Additional comments (8)
pkg/installer/template_test.go (2)
10-11: LGTM! Imports support new test functionality.The new imports for
fileutilandmapsutilare properly aliased and necessary for the new cleanup and metadata tests.
380-748: Excellent test coverage for metadata regeneration.The test suite comprehensively validates:
- Index and checksum file generation
- Cleanup integration with metadata updates
- Edge cases (empty directories, deleted templates)
- Path normalization consistency
The integration test (lines 583-674) effectively validates the complete cleanup and metadata regeneration flow.
pkg/installer/template.go (6)
22-22: LGTM! Import supports synchronized path tracking.The
mapsutilimport is necessary for theSyncLockMapused to track written template paths during updates.
138-141: Correct handling for fresh installations.Ignoring
writtenPathsduring fresh installation is appropriate since there are no pre-existing templates to clean up. The cleanup logic is only needed during updates.
174-192: Well-structured update flow with proper metadata refresh.The implementation correctly:
- Captures written paths from the template write operation
- Cleans up orphaned templates not present in the new release
- Regenerates metadata (index and checksum) after cleanup to ensure consistency
The non-fatal error handling for cleanup and metadata regeneration is appropriate, allowing the update to succeed even if these operations encounter issues.
Based on past review comments.
300-408: Well-implemented path tracking in template writes.The changes correctly:
- Return a synchronized map of written paths for downstream cleanup
- Track paths only after successful writes (lines 338, 352)
- Handle errors appropriately by returning
(nil, err)- Update all call sites to accommodate the new signature
The thread-safe
SyncLockMapis appropriate for the callback-based download mechanism.
410-512: Robust orphaned template cleanup implementation.The implementation handles:
- Path normalization for cross-platform compatibility (using
filepath.Clean)- Non-existent directories gracefully
- Exclusion of custom template directories
- Filtering to only process template files via
config.IsTemplate- Proper error handling with debug logging for removed files
The function correctly identifies and removes templates that exist locally but weren't part of the new release.
514-545: Properly implemented metadata regeneration.The function correctly:
- Purges empty directories left after cleanup
- Ensures the templates directory exists (handles the edge case where all templates were removed)
- Removes the old index file and regenerates it from current disk state
- Regenerates the checksum file
This ensures the index and checksum files accurately reflect the templates on disk after cleanup operations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/installer/template.go(9 hunks)pkg/installer/template_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/installer/template_test.gopkg/installer/template.go
🧠 Learnings (1)
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
Repo: projectdiscovery/nuclei PR: 6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Applied to files:
pkg/installer/template.go
🧬 Code graph analysis (2)
pkg/installer/template_test.go (3)
pkg/installer/template.go (2)
HideProgressBar(32-32)TemplateManager(70-74)pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)pkg/catalog/config/template.go (1)
GetNucleiTemplatesIndex(114-158)
pkg/installer/template.go (2)
pkg/catalog/config/template.go (2)
IsTemplate(68-82)GetNucleiTemplatesIndex(114-158)pkg/installer/util.go (1)
PurgeEmptyDirectories(75-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/installer/template.go (1)
517-525: Critical: PurgeEmptyDirectories will panic if directory doesn't exist.Calling
PurgeEmptyDirectories(dir)before ensuring the directory exists can cause a panic. Whenfilepath.WalkDirencounters a missing root, it passesnilfor theDirEntry, and the callback inutil.goattemptsd.IsDir(), triggering a nil pointer dereference.Apply this fix to ensure the directory exists before purging:
func (t *TemplateManager) regenerateTemplateMetadata(dir string) error { - // Purge empty directories that may have been left after cleanup - PurgeEmptyDirectories(dir) - - // Ensure templates directory exists (it may have been purged if empty) + // Ensure the directory exists before purging; WalkDir panics on a missing root. if !fileutil.FolderExists(dir) { if err := os.MkdirAll(dir, 0755); err != nil { - return errkit.Wrapf(err, "failed to recreate templates directory %s after purge", dir) + return errkit.Wrapf(err, "failed to create templates directory %s", dir) } } + + // Purge empty directories that may have been left after cleanup. + PurgeEmptyDirectories(dir) + + // Purge may remove the root if it became empty; recreate it for metadata outputs. + if !fileutil.FolderExists(dir) { + if err := os.MkdirAll(dir, 0755); err != nil { + return errkit.Wrapf(err, "failed to recreate templates directory %s after purge", dir) + } + }
🧹 Nitpick comments (1)
pkg/installer/template.go (1)
205-209: Consider verifying if this deletion loop is still necessary.With the new
cleanupOrphanedTemplateslogic at line 180, orphaned templates are already removed. However,cleanupOrphanedTemplatesonly processes template files (viaconfig.IsTemplatecheck), whileresults.deletionsmay include non-template files. Please verify whether this code is still needed for non-template files or if it can be removed to simplify the logic.If this code is no longer needed, consider removing it:
- // remove deleted templates - for _, deletion := range results.deletions { - if err := os.Remove(deletion); err != nil && !os.IsNotExist(err) { - gologger.Warning().Msgf("failed to remove deleted template %s: %s", deletion, err) - } - } -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/installer/template.go(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/installer/template.go
🧠 Learnings (1)
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
Repo: projectdiscovery/nuclei PR: 6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Applied to files:
pkg/installer/template.go
🧬 Code graph analysis (1)
pkg/installer/template.go (3)
pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)pkg/catalog/config/template.go (2)
IsTemplate(68-82)GetNucleiTemplatesIndex(114-158)pkg/installer/util.go (1)
PurgeEmptyDirectories(75-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests (ubuntu-latest)
- GitHub Check: Tests (windows-latest)
🔇 Additional comments (5)
pkg/installer/template.go (5)
22-22: LGTM: Signature changes and return value handling are correct.The updated
writeTemplatesToDisksignature and caller adaptations are appropriate. Discarding the written paths map ininstallTemplatesAtis fine since no cleanup is needed for fresh installs, while capturing it inupdateTemplatesAtenables the new orphan cleanup logic.Also applies to: 138-141, 174-177, 300-302
312-312: LGTM: Written paths tracking is comprehensive.The implementation correctly tracks all successfully written templates, including both relocated templates and new/updated templates. The error handling properly returns
nilfor the map on failures and the populatedwrittenPathson success.Also applies to: 337-338, 348-353, 357-407
410-512: LGTM: Orphan cleanup logic is solid.The implementation correctly identifies and removes templates that exist locally but were not part of the new release. Path normalization is consistent, custom directories are properly excluded, and error handling is appropriate.
179-192: LGTM: Metadata regeneration after cleanup addresses previous concern.The new flow correctly regenerates template index and checksum files after orphan cleanup, ensuring metadata accurately reflects the cleaned template tree. This properly addresses the issue raised in the first previous review comment.
540-550: LGTM: Index filtering adds robustness.Filtering the regenerated index to only include paths that actually exist on disk is good defensive programming, especially for handling Windows file deletion timing issues.
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
Proposed changes
Closes #6467
Checklist
Summary by CodeRabbit
New Features
Tests