fix(patterns): patterns loader temp dir leak - #2192
Open
scottidler wants to merge 1 commit into
Open
Conversation
`PatternsLoader.configure()` ran `os.MkdirTemp` on every plugin registry construction, so every fabric invocation left behind an empty `fabric-patterns-*` directory that nothing ever removed. Only `PopulateDB()` and the helpers it calls use that folder, and it is already removed on the success path by `movePatterns`. Move creation into `ensureTempPatternsFolder()`, called from `PopulateDB()`, and remove the folder in a deferred cleanup so the error paths are covered too and the loader stays reusable. A plain `defer os.RemoveAll(tempDir)` in `configure()` would delete the folder before `gitCloneAndCopy` populates it, so lazy creation is the correct seam. Adds regression coverage: `configure()` must not create the folder, and `ensureTempPatternsFolder()` must be lazy and idempotent. closes danielmiessler#2190
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this Pull Request (PR) does
Stops
PatternsLoaderfrom leaking one empty temp directory per fabric invocation.configure()calledos.MkdirTempdirectly. It is wired as the plugin'sConfigureCustom, so it runs on everyNewPluginRegistryconstruction, which meansevery fabric invocation created an empty
fabric-patterns-*directory that nothingremoved. Details and measurements are in #2190.
Only
PopulateDB()and the helpers it calls readtempPatternsFolder, and thesuccess path already removes the folder in
movePatterns. So this moves creation intoensureTempPatternsFolder(), called fromPopulateDB(), plus a deferred cleanup thatalso covers the error paths and leaves the loader reusable.
Worth noting for review: a plain
defer os.RemoveAll(tempDir)insideconfigure()looks like the obvious one-line fix but is wrong. It deletes the folder before
gitCloneAndCopypopulates it. Lazy creation is the correct seam.Verification
fabric -lagainst a configured~/.config/fabric/.env:20 leaked directories before, 0 after.
fabric -Ubehaves identically before and after: 255 patterns downloaded,257 entries installed,
loadedmarker written, 0 leftover temp directories.go test ./internal/tools/... ./internal/core/... ./internal/plugins/...passes,25 packages, exit 0.
gofmtandgo vetclean.rather than just passing alongside it.
mainat338b89c.Related issues
closes #2190
Screenshots
N/A, no user-visible output changes.