fix: denylist reload now removes stale entries and closes file handles#1895
Open
LautaroPetaccio wants to merge 5 commits into
Open
fix: denylist reload now removes stale entries and closes file handles#1895LautaroPetaccio wants to merge 5 commits into
LautaroPetaccio wants to merge 5 commits into
Conversation
loadDenylists() only added to the deniedContentIdentifiers Set and never cleared it, so entries removed from the denylist file or URL remained denied until server restart. The fix builds a new Set during reload and swaps it atomically after all sources load successfully, preserving the old set if loading fails. The readline interface created from createInterface() was also never closed after processing, leaking a file descriptor every reload cycle. Also fixes the NodeJS.Timer type to use ReturnType<typeof setInterval> to resolve the TS2769 error with clearInterval.
- Use the already-trimmed cid variable instead of re-trimming the original line - Add test verifying that entries removed from the denylist file are no longer reported as denylisted after reload
…land/catalyst into fix/denylist-reload-and-cleanup
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.
Summary
Two issues in the denylist reload mechanism:
Stale entries never removed (
ports/denylist.ts):loadDenylists()only added to thedeniedContentIdentifiersSet, never cleared it. If a content hash was removed from the denylist file or remote URL, it remained denied until the server restarted. The fix builds a new Set during each reload cycle and swaps it in atomically after all sources load successfully. If loading fails (outer try/catch), the old set is preserved to avoid briefly un-denying everything.Readline interface never closed: The
createInterface()call created a readline wrapper around the file read stream but never closed it after processing. SinceloadDenylists()runs every 2 minutes, this leaked a file descriptor each cycle. Addedlines.close()after processing.Also fixes the
NodeJS.Timertype toReturnType<typeof setInterval>to resolve a TS2769 error withclearInterval.Test plan
cd content && npx jest test/unit/ports/denylist.spec.ts --no-coverage