Skip to content

fix(cron): close sqlite connections deterministically in execution ledger - #69594

Closed
JoaoMarcos44 wants to merge 2 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/cron-sqlite-fd-leak-69567
Closed

JoaoMarcos44 wants to merge 2 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/cron-sqlite-fd-leak-69567

Conversation

@JoaoMarcos44

@JoaoMarcos44 JoaoMarcos44 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Closes #69567

🚨 The Problem (SQLite Connection & File Descriptor Leak)

The cron execution ledger (cron/executions.py) opens SQLite connections on multiple operations during a normal job run cycle:

  1. create_execution() (connection 1)
  2. mark_execution_running() (connection 2)
  3. finish_execution() (connection 3)

The code implicitly assumed that using with _lock, _connect() as conn: would close the connection upon exiting the block. However, Python's sqlite3.Connection acts only as a transactional context manager (handling commit/rollback on exit, but never closing the connection).

In Linux environments running SQLite in WAL mode, each of these 3 unclosed connections keeps active file descriptors for executions.db and executions.db-wal (6 file descriptors per run). At a rate of ~60 cron executions per hour, the gateway process would hit the soft limit of 1,024 file descriptors (EMFILE) in less than 3 hours, leading to severe and cascading system-wide crashes (e.g. failing config writes, session database access, socket binds, etc.).


🛡️ The Solution (Root Cause Fixed)

We centralized and decoupled connection lifecycle, transaction management, and locking into a dedicated context manager named _transaction() in cron/executions.py:

  • Deterministic Closing: The context manager guarantees that conn.close() is unconditionally called in a finally block.
  • DDL/PRAGMA Setup Safety: Database initialization (_initialize_schema()) is now run inside the try/finally block. If initialization fails (e.g., DDL or PRAGMA errors) after a successful connect(), the connection is still closed instead of leaked.
  • Preserved Transaction Invariants: The inner transactional block keeps its original behavior (with conn:) to ensure commits on success and rollbacks on exceptions.

🧪 Automated Test Coverage

We added four new exhaustive and deterministic tests in test_execution_ledger.py to prevent regressions:

  1. test_ledger_operations_close_every_connection: Tracks the connection lifecycle to verify that every single ledger call closes its connection.
  2. test_early_return_still_closes_connection: Ensures connections are closed even during premature early returns (such as mark_execution_running exiting early on invalid status transitions).
  3. test_exception_during_operation_still_closes_connection: Verifies rollback followed by connection closing when statement execution fails.
  4. test_schema_init_failure_still_closes_connection: Verifies that a connection is closed if DDL schema creation raises an error.

All 21 ledger tests are now passing successfully (21 passed).


🎨 Infographic: Cron Ledger FD Leak Safeguard

Cron Ledger FD Leak Safeguard Infographic

@egilewski

Copy link
Copy Markdown
Contributor

suggesting changes

The deterministic-close change works on this branch, but its current-main replay is broken. Current main added apply_wal_with_fallback as a function-local import in _connect() after this branch point. The patch moves the helper call into _initialize_schema(), where that name is not visible, so every ledger operation raises NameError before a transaction starts. The focused ledger suite on the replay returns 16 failed, 5 passed (the unmodified PR head returns 21 passed).

Please rebase and keep apply_wal_with_fallback visible to _initialize_schema()—for example, by importing it at module scope or inside that function—while preserving the helper rather than reverting to the branch's raw PRAGMA journal_mode=WAL.

Security evidence:

  • trust boundary: cron scheduler and CLI execution-ledger operations.
  • source/sink/invariant: every ledger API reaches _transaction() / _initialize_schema(); schema setup must remain callable and every opened connection must close.
  • current-main reproduction: the close-tracking probe reports opened=1 explicit_closed=0.
  • PR-head or patch-replay validation: PR head reports opened=1 explicit_closed=1; the current-main replay raises NameError: apply_wal_with_fallback is not defined.
  • positive/negative cases: PR-head normal, early-return, operation-error, and schema-error close tests pass; replayed normal create/update/list/recovery paths fail before use.
  • residual bypass search: current main's WAL fallback and WAL-reset corruption guard must remain in the rebased implementation.
  • reviewer validation: tests/cron/test_execution_ledger.py is 21 passed on PR head and 16 failed, 5 passed on the current-main replay.

Signed: GPT-5.6-sol-xhigh in Codex

teknium1 pushed a commit that referenced this pull request Jul 24, 2026
…, delegation, and verification ledgers

Three durable ledgers used `with _connect() as conn:` where the sqlite3
connection context manager commits/rolls back but never closes, leaking the
db/-wal/-shm file descriptors on every call. On a long-running gateway this
exhausts RLIMIT_NOFILE and fails unrelated components with
`[Errno 24] Too many open files`. Same bug class as the cron execution ledger
(#69567 / PR #69594), which the connection helpers here are modeled on.

Fix: route every ledger operation through a `_transaction()` context manager
that guarantees `conn.close()` on exit. `_connect()` keeps its
schema-on-connect contract (several tests call it directly) and now self-closes
if schema init fails.

Adds per-module regression tests asserting every opened connection is closed,
including the no-op-update and exception-mid-transaction paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
teknium1 pushed a commit that referenced this pull request Jul 24, 2026
…, delegation, and verification ledgers

Three durable ledgers used `with _connect() as conn:` where the sqlite3
connection context manager commits/rolls back but never closes, leaking the
db/-wal/-shm file descriptors on every call. On a long-running gateway this
exhausts RLIMIT_NOFILE and fails unrelated components with
`[Errno 24] Too many open files`. Same bug class as the cron execution ledger
(#69567 / PR #69594), which the connection helpers here are modeled on.

Fix: route every ledger operation through a `_transaction()` context manager
that guarantees `conn.close()` on exit. `_connect()` keeps its
schema-on-connect contract (several tests call it directly) and now self-closes
if schema init fails.

Adds per-module regression tests asserting every opened connection is closed,
including the no-op-update and exception-mid-transaction paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@teknium1

Copy link
Copy Markdown
Collaborator

Merged via #70985 with your authorship preserved on the cron-ledger commit — clean deterministic-close fix, and your regression tests shipped with it. Consolidated with the 3-ledger fix from #69681 since it's the same bug class. Fixes #69567.

@teknium1 teknium1 closed this Jul 24, 2026
webtecnica added a commit to webtecnica/hermes-agent that referenced this pull request Jul 26, 2026
…D exhaustion (NousResearch#69567)

The cron execution ledger was leaking SQLite connections because `sqlite3.Connection` as a context manager only commits/rolls back -- it does NOT close the connection. Without explicit `conn.close()`, every ledger call (create, mark_running, finish, recover, list, latest) left an open connection and its WAL/SHM file descriptors.

Created a `_transaction()` context manager that wraps lock acquisition, connection open, schema initialization, transaction commit/rollback, and deterministic connection close in a `finally` block. Schema init runs inside the `try` too so PRAGMA/DDL failures after a successful connect still close the connection.

Key design:
- `_connect()` now only opens the connection (no schema)
- `_initialize_schema(conn)` handles PRAGMAs, DDL, and WAL setup
- `_transaction()` combines lock + connect + init + commit/rollback + close
- `apply_wal_with_fallback` is preserved (unlike PR NousResearch#69594 which replaced it with raw PRAGMA, losing NFS/SMB fallback)

All 7 call sites migrated from `with _lock, _connect() as conn:` to `with _transaction() as conn:`.

Added regression test `test_every_ledger_call_closes_sqlite_connection` that repeatedly calls all ledger functions and asserts the /proc/self/fd count for executions.db doesn't grow.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…, delegation, and verification ledgers

Three durable ledgers used `with _connect() as conn:` where the sqlite3
connection context manager commits/rolls back but never closes, leaking the
db/-wal/-shm file descriptors on every call. On a long-running gateway this
exhausts RLIMIT_NOFILE and fails unrelated components with
`[Errno 24] Too many open files`. Same bug class as the cron execution ledger
(NousResearch#69567 / PR NousResearch#69594), which the connection helpers here are modeled on.

Fix: route every ledger operation through a `_transaction()` context manager
that guarantees `conn.close()` on exit. `_connect()` keeps its
schema-on-connect contract (several tests call it directly) and now self-closes
if schema init fails.

Adds per-module regression tests asserting every opened connection is closed,
including the no-op-update and exception-mid-transaction paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
prmartinow pushed a commit to prmartinow/hermes-agent that referenced this pull request Aug 26, 2026
…, delegation, and verification ledgers

Three durable ledgers used `with _connect() as conn:` where the sqlite3
connection context manager commits/rolls back but never closes, leaking the
db/-wal/-shm file descriptors on every call. On a long-running gateway this
exhausts RLIMIT_NOFILE and fails unrelated components with
`[Errno 24] Too many open files`. Same bug class as the cron execution ledger
(NousResearch#69567 / PR NousResearch#69594), which the connection helpers here are modeled on.

Fix: route every ledger operation through a `_transaction()` context manager
that guarantees `conn.close()` on exit. `_connect()` keeps its
schema-on-connect contract (several tests call it directly) and now self-closes
if schema init fails.

Adds per-module regression tests asserting every opened connection is closed,
including the no-op-update and exception-mid-transaction paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
…, delegation, and verification ledgers

Three durable ledgers used `with _connect() as conn:` where the sqlite3
connection context manager commits/rolls back but never closes, leaking the
db/-wal/-shm file descriptors on every call. On a long-running gateway this
exhausts RLIMIT_NOFILE and fails unrelated components with
`[Errno 24] Too many open files`. Same bug class as the cron execution ledger
(NousResearch#69567 / PR NousResearch#69594), which the connection helpers here are modeled on.

Fix: route every ledger operation through a `_transaction()` context manager
that guarantees `conn.close()` on exit. `_connect()` keeps its
schema-on-connect contract (several tests call it directly) and now self-closes
if schema init fails.

Adds per-module regression tests asserting every opened connection is closed,
including the no-op-update and exception-mid-transaction paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P1 High — major feature broken, no workaround type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: cron execution ledger leaks three SQLite connections per run, exhausting file descriptors

4 participants