Skip to content

fix: guard flock(LOCK_UN) against OSError in kanban_db and google_oauth - #35204

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

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

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Problem

Two files had unguarded fcntl.flock(LOCK_UN) calls in finally blocks that could raise OSError and propagate as unhandled exceptions.

  • hermes_cli/kanban_db.py:1075: flock(LOCK_UN) in finally block without try/except — if unlock fails, the exception propagates even though handle.close() still runs in the outer finally.
  • agent/google_oauth.py:229: flock(LOCK_UN) inside try/except ImportError (for msvcrt fallback) but OSError was not caught. The msvcrt path already had proper OSError guarding, but the fcntl path did not.

Fix

  • kanban_db.py: Wrap flock(LOCK_UN) in try/except OSError: pass
  • google_oauth.py: Change except ImportError to except (ImportError, OSError) so both import failures and lock failures are handled

Before vs After

File Before After
kanban_db.py flock(UN) bare in finally Wrapped in try/except OSError
google_oauth.py except ImportError only except (ImportError, OSError)

Pattern Consistency

Both fixes follow the existing pattern already used in:

  • tools/skill_usage.py:91except (OSError, IOError): pass
  • tools/memory_tool.py:234except (OSError, IOError): pass
  • hermes_cli/auth.py:1012except (OSError, IOError): pass
  • cron/scheduler.py:2027except Exception: pass
  • gateway/status.py:416except OSError: pass

The two files in this PR were the remaining unguarded sites across the codebase.

Two files had unguarded fcntl.flock(LOCK_UN) calls in finally blocks
that could raise OSError and propagate as unhandled exceptions.

- hermes_cli/kanban_db.py: flock(LOCK_UN) in finally block without
  try/except — if unlock fails, the exception propagates even though
  handle.close() still runs in the outer finally.

- agent/google_oauth.py: flock(LOCK_UN) inside try/except ImportError
  (for msvcrt fallback) but OSError was not caught. The msvcrt path
  already had proper OSError guarding, but the fcntl path did not.

Both fixes follow the existing pattern used in tools/skill_usage.py,
tools/memory_tool.py, and hermes_cli/auth.py where flock(LOCK_UN) is
already wrapped in try/except (OSError, IOError): pass.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels May 30, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

The google_oauth.py fix is a duplicate of #31275 and #30276. The kanban_db.py fix is new — last remaining unguarded flock(LOCK_UN) site. Part of the broader fcntl guard family: #16274, #23873, #20529.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the defensive cleanup. The Kanban issue is still present on current main, but this branch needs a focused salvage rather than a direct cherry-pick.

Problems

  • agent/google_oauth.py was deleted by 7130d60861a9243301514bff611a9381830d59d8; the OAuth hunk is obsolete. This also aligns with the member note that this portion duplicated earlier work.
  • The relevant Kanban cleanup has moved: current main still has the unguarded POSIX unlock at hermes_cli/kanban_db.py:1410, while the submitted hunk targets the older location.
  • Current tests cover normal lock release (tests/hermes_cli/test_kanban_db.py:81) and bounded acquisition (tests/hermes_cli/test_kanban_init_lock_bounded.py:75), but not an OSError from POSIX unlock.

Suggested changes

  • Reapply only the Kanban guard at hermes_cli/kanban_db.py:1410, preserving its outer close-finally.
  • Add an OSError-on-unlock regression test; do not revive the removed OAuth module.

Automated hermes-sweeper review.

Comment thread agent/google_oauth.py

fcntl.flock(fd, fcntl.LOCK_UN)
except ImportError:
except (ImportError, OSError):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

agent/google_oauth.py was deleted on current main by 7130d60861a9243301514bff611a9381830d59d8; omit this hunk when salvaging the still-relevant Kanban fix.

Comment thread hermes_cli/kanban_db.py
fcntl.flock(handle.fileno(), fcntl.LOCK_UN)
try:
fcntl.flock(handle.fileno(), fcntl.LOCK_UN)
except OSError:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The same cleanup concern remains on current main, but this hunk's context is stale: apply the guard to the current POSIX unlock at hermes_cli/kanban_db.py:1410 and add a regression test that makes that unlock raise OSError.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants