Skip to content

fix(local-env): use context manager in _update_cwd to prevent fd leak - #8112

Closed
chinadbo wants to merge 2 commits into
NousResearch:mainfrom
chinadbo:fix/local-env-file-handle-leak
Closed

fix(local-env): use context manager in _update_cwd to prevent fd leak#8112
chinadbo wants to merge 2 commits into
NousResearch:mainfrom
chinadbo:fix/local-env-file-handle-leak

Conversation

@chinadbo

Copy link
Copy Markdown
Contributor

Summary

  • open(self._cwd_file).read().strip() in _update_cwd leaks the file descriptor because the handle is never explicitly closed
  • Replace with a with statement context manager to ensure prompt cleanup after each read

Test plan

  • test_update_cwd_uses_context_manager — verifies open() is used with __enter__/__exit__ (context manager)
  • test_update_cwd_closes_file_handle — verifies no fd leak over 50 repeated calls
  • Existing test_local_env_blocklist.py and test_local_tempdir.py still pass (21/21)

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have backend/local Local shell execution labels Apr 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #15552 — same root cause: bare open().read() fd leak in LocalEnvironment._update_cwd.

@chinadbo
chinadbo force-pushed the fix/local-env-file-handle-leak branch from 5093488 to 8b54bcc Compare April 27, 2026 13:18
open(self._cwd_file).read().strip() leaks the file descriptor because
the handle is never closed. Replace with a context manager to ensure
prompt cleanup.
- Assert __exit__ is called in context manager test
- Add test verifying fd is closed when f.read() raises UnicodeDecodeError
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for this contribution, @chinadbo! This is an automated hermes-sweeper review.

The fix you proposed has already landed on main via a separate commit that addresses the same root cause:

Closing as implemented on main. The additional tests in this PR (test_local_env_file_handle.py) could still be worth salvaging as a separate contribution if desired.

@teknium1 teknium1 closed this Jun 10, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/local Local shell execution P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants