fix(cmplog): stop() closes the FIFO drain - #27
Merged
Merged
Conversation
stop() unlinked the FIFO but never closed _FifoDrain, leaving its thread polling and both fds open (FIFO sink is the default). Link-order test fixture now requests monkeypatch so stop() tears down first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWiz6zR3X7BQXuQT2m45G4
Reviewer's GuideFix cmplog FIFO resource leaks by closing the drain during collector shutdown, stopping its thread, releasing both FIFO descriptors, and clearing shutdown state; add targeted regression coverage for cleanup, idempotency, file sinks, and fixture teardown ordering. Sequence diagram for cmplog collector shutdown cleanupsequenceDiagram
participant Collector as CmplogCollector
participant Drain as _FifoDrain
participant FIFO as FIFO resources
participant OS as OS
Collector->>Collector: stop()
alt FIFO sink configured
Collector->>Drain: close()
Drain->>Drain: stop drain thread
Drain->>FIFO: close both file descriptors
Drain->>OS: unlink FIFO
Collector->>Collector: clear _fifo
Collector->>Collector: clear log_path
else File sink configured
Collector->>OS: exists(log_path)
Collector->>OS: unlink(log_path)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues remain.
Review effort: Lite
Findings: None
What changed in this PR
Fixes cmplog FIFO resource leaks during collector shutdown.
Changes:
- Closes FIFO drain threads and file descriptors in
stop(). - Clears FIFO paths safely.
- Adds cleanup, idempotency, and file-sink regression tests.
- Corrects fixture teardown ordering.
| File | Description |
|---|---|
tests/test_regression_cmplog_fifo_stop.py |
Covers FIFO cleanup and repeated stopping. |
tests/test_regression_cmplog_asan_link_order.py |
Ensures teardown precedes monkeypatch restoration. |
src/fuzzer_tool/core/cmplog.py |
Releases FIFO drain resources during shutdown. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Follow-up to #26 review.
CmplogCollector.stop()unlinked the FIFO but never called_FifoDrain.close(), so the drain thread kept polling and both FIFO fds stayed open. FIFO sink is the CLI default, so every run leaked this.stop()now closes the drain and clearslog_path(close already unlinks, so the old exists-then-clear path no longer fired). Newtests/test_regression_cmplog_fifo_stop.py: regression (fails before, passes after), adversarial double-stop, falsification for file sink.collectorfixture now requestsmonkeypatch, sostop()tears down before monkeypatch undoes. Cross-test leakage did not reproduce (autouse_env_isolationin conftest resets env), but order was wrong within the test; verified with--setup-show.cmplog tests: 199 passed. ruff clean; lizard clean on the new file.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QWiz6zR3X7BQXuQT2m45G4
Generated by Claude Code
Summary by Sourcery
Close the cmplog FIFO drain during collector shutdown to prevent resource leaks.
Bug Fixes:
Tests: