feat(lib): reuse metadata cache across thread-safe scans - #7608
Conversation
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Keep one engine-owned metadata index across SDK calls and persist it at shutdown instead of loading and saving a new index for each execution. Track dirty state and parser validation so compatible metadata can skip redundant filtering parses w/o trusting lax entries in strict mode. Defensively copy cached values, invalidate stale entries synchronously, and remove the unused ephemeral engine allocation. Signed-off-by: Dwi Siswanto <git@dw1.io>
WalkthroughThe change adds validation-aware metadata caching, defensive index persistence, shared metadata indexes for loaders, and lazy thread-safe engine integration. Execution setup reuses the engine index, while new tests and a benchmark cover cache correctness, parser compatibility, concurrency, and repeated execution. ChangesMetadata cache and execution integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant NucleiEngine
participant LoaderStore
participant MetadataIndex
Client->>NucleiEngine: ExecuteNucleiWithOptsCtx
NucleiEngine->>MetadataIndex: getMetadataIndex
NucleiEngine->>LoaderStore: create with MetadataIndex
LoaderStore->>MetadataIndex: reuse or update template metadata
LoaderStore-->>NucleiEngine: loaded templates
NucleiEngine-->>Client: execution result
Client->>NucleiEngine: Close
NucleiEngine->>MetadataIndex: Save
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
pkg/catalog/index/index.go (1)
320-333: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
GetAll/FilterFuncdon't invalidate stale entries likeGetnow does.
Getsynchronously drops entries that failIsValid()(lines 117-129), butGetAll(andFilterFunc, which is built onGetAll) return cached metadata without any staleness check, so bulk reads can surface metadata for files whoseModTimehas changed on disk. This creates an inconsistency between per-key and bulk access paths introduced by this PR's freshness guarantee.♻️ Possible fix: route GetAll through the same validity check as Get
func (i *Index) GetAll() map[string]*Metadata { i.mu.RLock() defer i.mu.RUnlock() result := make(map[string]*Metadata, i.cache.EstimatedSize()) for path, metadata := range i.cache.All() { - result[path] = metadata.clone() + if metadata.IsValid() { + result[path] = metadata.clone() + } } return result }Also applies to: 349-359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/catalog/index/index.go` around lines 320 - 333, Update GetAll to apply the same IsValid() freshness check and remove stale entries synchronously, matching Get’s behavior; ensure FilterFunc continues using the corrected GetAll result so both bulk access paths exclude metadata whose files have changed on disk.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/sdk.go`:
- Around line 268-272: Update closeInternal to obtain the metadata index through
the existing synchronized getMetadataIndex() accessor before saving it, rather
than reading e.metadataIndex directly. Preserve the current nil check and
warning behavior while ensuring the read uses the same synchronization as
metadataIndexOnce.Do.
---
Nitpick comments:
In `@pkg/catalog/index/index.go`:
- Around line 320-333: Update GetAll to apply the same IsValid() freshness check
and remove stale entries synchronously, matching Get’s behavior; ensure
FilterFunc continues using the corrected GetAll result so both bulk access paths
exclude metadata whose files have changed on disk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d44e96f-53a1-4c7f-8df0-9afcf6c292ed
📒 Files selected for processing (8)
lib/multi.golib/multi_bench_test.golib/sdk.gopkg/catalog/index/index.gopkg/catalog/index/index_test.gopkg/catalog/index/metadata.gopkg/catalog/loader/loader.gopkg/catalog/loader/loader_test.go
| if e.metadataIndex != nil { | ||
| if err := e.metadataIndex.Save(); err != nil { | ||
| e.Logger.Warning().Msgf("Could not save metadata cache: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unsynchronized read of e.metadataIndex races with concurrent getMetadataIndex() writers.
e.metadataIndex is written inside metadataIndexOnce.Do(...) (lines 107-123), but closeInternal reads the raw field directly instead of going through the same sync.Once-guarded getter. If Close() runs concurrently with an in-flight ExecuteNucleiWithOptsCtx call that's still initializing the index (a realistic scenario for a "thread-safe" engine), this is a data race per the Go memory model.
🔒 Proposed fix: read through the synchronized getter
- if e.metadataIndex != nil {
- if err := e.metadataIndex.Save(); err != nil {
+ if metadataIndex := e.getMetadataIndex(); metadataIndex != nil {
+ if err := metadataIndex.Save(); err != nil {
e.Logger.Warning().Msgf("Could not save metadata cache: %v", err)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if e.metadataIndex != nil { | |
| if err := e.metadataIndex.Save(); err != nil { | |
| e.Logger.Warning().Msgf("Could not save metadata cache: %v", err) | |
| } | |
| } | |
| if metadataIndex := e.getMetadataIndex(); metadataIndex != nil { | |
| if err := metadataIndex.Save(); err != nil { | |
| e.Logger.Warning().Msgf("Could not save metadata cache: %v", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/sdk.go` around lines 268 - 272, Update closeInternal to obtain the
metadata index through the existing synchronized getMetadataIndex() accessor
before saving it, rather than reading e.metadataIndex directly. Preserve the
current nil check and warning behavior while ensuring the read uses the same
synchronization as metadataIndexOnce.Do.
Mzack9999
added
the
Type: Bug
2 participants
Proposed changes
Keep one engine-owned metadata index across SDK
calls and persist it at shutdown instead of
loading and saving a new index for each execution.
Track dirty state and parser validation so
compatible metadata can skip redundant filtering
parses w/o trusting lax entries in strict mode.
Defensively copy cached values, invalidate stale
entries synchronously, and remove the unused
ephemeral engine allocation.
Fixes #7569
Proof
The patched version of
ExecuteNucleiWithOptsCtxis dramatically faster and more efficient than thedevversion:devpatchThe patch reduces runtime, memory usage, and allocations by roughly three orders of magnitude (≈1000× better) with high statistical significance (p=0.000).
Checklist
Summary by CodeRabbit
Performance
Reliability
Compatibility