Skip to content

fix: guard fcntl.flock(LOCK_UN) against OSError in cleanup blocks - #35793

Open
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/flock-unlock-oserror-guard
Open

fix: guard fcntl.flock(LOCK_UN) against OSError in cleanup blocks#35793
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/flock-unlock-oserror-guard

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Fix: Guard fcntl.flock(LOCK_UN) against OSError in cleanup blocks

Two locations had unguarded fcntl.flock(LOCK_UN) calls that could raise OSError (e.g. EBADF if the fd was already closed), preventing proper cleanup:

agent/google_oauth.py:229

OSError from flock() was not caught — the except only handled ImportError (for platforms without fcntl). An OSError would skip the msvcrt fallback entirely and propagate up.

Fix: Added OSError to the existing except ImportError clause.

hermes_cli/kanban_db.py:1171

OSError from flock() would skip the handle.close() in the outer finally block, leaking the file descriptor.

Fix: Wrapped in try/except OSError: pass.

Already-guarded locations (no change needed)

5 other flock(LOCK_UN) locations already had proper try/except (OSError, IOError): pass guards:

  • agent/shell_hooks.py:636
  • tools/skill_usage.py:91
  • tools/memory_tool.py:234
  • tools/environments/file_sync.py:293
  • hermes_cli/auth.py:1012

Two locations had unguarded fcntl.flock(LOCK_UN) calls that could
raise OSError (e.g. EBADF), preventing proper cleanup:

- agent/google_oauth.py:229 — OSError from flock was not caught,
  skipping the msvcrt fallback on Windows and propagating up.
  Added OSError to the existing except clause.

- hermes_cli/kanban_db.py:1171 — OSError from flock skipped the
  handle.close() in the outer finally block, leaking the file
  descriptor. Wrapped in try/except OSError: pass.

Five other flock(LOCK_UN) locations already had proper guards
(shell_hooks.py, skill_usage.py, memory_tool.py, file_sync.py, auth.py).
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #35204 — covers the same two remaining unguarded flock(LOCK_UN) sites (google_oauth.py + kanban_db.py). Also overlaps with #31275 (google_oauth only) and the broader family rooted at #16274.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary — PR #35793

Verdict: Comment (Duplicate label — may conflict with another fix)

Author: annguyenNous | Type: bugfix | Files: agent/google_oauth.py, hermes_cli/kanban_db.py

Findings

  • Clean, targeted fix: adds OSError to the existing except ImportError in google_oauth.py:229, and wraps kanban_db.py:1171 in try/except OSError: pass.
  • The PR correctly identifies 5 other locations that already have proper guards — no scope creep.
  • ⚠️ Note: This PR is labeled duplicate. The fix is correct but may overlap with another PR. If this one lands, the code is correct as written. Verify it's not duplicating an already-merged fix before merging.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cleanup. The original two-hunk patch is stale, but one related cleanup path remains worth salvaging.

Problems

  • agent/google_oauth.py no longer exists on current main: commit 7130d60861a9243301514bff611a9381830d59d8 removed the Google OAuth provider, so that hunk has no target.
  • The Kanban code was refactored. Current main still has a bare POSIX unlock at hermes_cli/kanban_db.py:1410 in _cross_process_init_lock(), but the PR changes obsolete context. Its nested finally still closes the handle at hermes_cli/kanban_db.py:1411-1412; an unlock failure propagates, but does not skip close().
  • Existing Kanban coverage asserts normal Windows lock release (tests/hermes_cli/test_kanban_db.py:81-106), not an OSError from the current POSIX unlock.

Suggested changes

  • Re-scope the guard to hermes_cli/kanban_db.py:1410 and add a regression test that injects an unlock OSError.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants