fix(cron): close SQLite connections in executions ledger to prevent FD exhaustion (#69567) - #70917
Closed
webtecnica wants to merge 2 commits into
Closed
fix(cron): close SQLite connections in executions ledger to prevent FD exhaustion (#69567)#70917webtecnica wants to merge 2 commits into
webtecnica wants to merge 2 commits into
Conversation
…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.
alt-glitch
added
type/bug
Collaborator
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #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 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 so PRAGMA/DDL failures after a successful connect still close the connection.
Key design:
Added regression test test_every_ledger_call_closes_sqlite_connection that runs 10 full lifecycles through every ledger function and asserts /proc/self/fd count for executions.db does not grow.
18 passed