Skip to content

Clean all temporary test files in all circumstances #246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented May 28, 2025

User description

Codeflash CLI currently generates some test files for establishing code behavior and performance. In certain cases, these files remain in the filesystem which shows up in git status. We should be deleting all such files no matter how the code exited.


PR Type

Documentation


Description

  • Insert explanatory comment for trace cleanup

Changes walkthrough 📝

Relevant files
Documentation
optimizer.py
Document trace file cleanup                                                           

codeflash/optimization/optimizer.py

  • Added comment explaining trace file unlink behavior
+1/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Race Condition

    The code checks for the trace file with exists() before calling unlink(), which can lead to a TOCTOU race condition. Consider using Path.unlink(missing_ok=True) or catching FileNotFoundError directly.

    trace_file = Path(self.args.benchmarks_root) / "benchmarks.trace"
    if trace_file.exists():
        #Trace file not deleted all the times
        trace_file.unlink()
    Error Handling

    If an exception is thrown before unlink(), the trace file might not be deleted. It may be better to place cleanup in a finally block to guarantee deletion in all exit scenarios.

    try:
        instrument_codeflash_trace_decorator(file_to_funcs_to_optimize)
        trace_file = Path(self.args.benchmarks_root) / "benchmarks.trace"
        if trace_file.exists():
            #Trace file not deleted all the times
            trace_file.unlink()
    Comment Style

    The newly added comment #Trace file not deleted all the times lacks a leading space and could be clarified. Follow the project's comment style and improve the grammar for clarity.

    #Trace file not deleted all the times

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use TemporaryDirectory for auto cleanup

    Switch to tempfile.TemporaryDirectory to ensure the temporary directory is
    automatically cleaned up even if exceptions occur. Assign the manager to an instance
    attribute so it remains alive for the needed scope.

    codeflash/optimization/optimizer.py [118-119]

    -self.replay_tests_dir = Path(
    -    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=self.args.benchmarks_root)
    -)
    +self._temp_dir_manager = tempfile.TemporaryDirectory(prefix="codeflash_replay_tests_", dir=self.args.benchmarks_root)
    +self.replay_tests_dir = Path(self._temp_dir_manager.name)
    Suggestion importance[1-10]: 7

    __

    Why: Switching to TemporaryDirectory ensures the temp dir is cleaned up automatically, preventing resource leaks while maintaining the same behavior.

    Medium
    Possible issue
    Make trace_file deletion robust

    Replace the manual existence check and unlink with a single unlink(missing_ok=True)
    call wrapped in a try/except to avoid race conditions and suppress any OSError if
    deletion fails.

    codeflash/optimization/optimizer.py [114-116]

    -if trace_file.exists():
    -    trace_file.unlink()
    +try:
    +    trace_file.unlink(missing_ok=True)
    +except OSError:
    +    pass
    Suggestion importance[1-10]: 6

    __

    Why: Using unlink(missing_ok=True) and catching OSError removes the race condition and safely ignores deletion failures, improving robustness without major functional changes.

    Low

    Copy link

    openhands-ai bot commented Jun 3, 2025

    Looks like there are a few issues preventing this PR from being merged!

    • GitHub Actions are failing:
      • Lint

    If you'd like me to help, just leave a comment, like

    @OpenHands please fix the failing actions on PR #246

    Feel free to include any additional details that might help me get this PR into a better state.

    You can manage your notification settings

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant