Skip to content

fix(gateway,tools,agent): close leaked SQLite connections in delivery… - #69681

Closed
dhruvraajeev wants to merge 1 commit into
NousResearch:mainfrom
dhruvraajeev:fix/sqlite-fd-leak-ledgers
Closed

fix(gateway,tools,agent): close leaked SQLite connections in delivery…#69681
dhruvraajeev wants to merge 1 commit into
NousResearch:mainfrom
dhruvraajeev:fix/sqlite-fd-leak-ledgers

Conversation

@dhruvraajeev

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a file-descriptor leak in three durable SQLite ledgers. Each used
with _connect() as conn:, where the sqlite3.Connection context manager
commits/rolls back but never closes the connection — so every operation leaked
the connection and its -wal/-shm descriptors, deferring the close to GC. On
a long-running gateway this drifts toward RLIMIT_NOFILE and fails unrelated
components with [Errno 24] Too many open files.

This is the same bug class as the cron execution ledger (#69567), fixed in
PR #69594; these three ledgers share the identical _connect() pattern and
were not covered by that fix. gateway/delivery_ledger.py runs on every
outbound final response, so it is the highest-frequency leaker.

Related Issue

Fixes #69678

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/delivery_ledger.py — added a _transaction() context manager that
    always close()s; routed all 5 sites through it. The lock-free _prune()
    site stays lock-free.
  • tools/async_delegation.py — same _transaction() wrapper; routed all 13
    durable-ledger sites through it.
  • agent/verification_evidence.py — same _transaction() wrapper; routed all
    3 sites through it.
  • In all three, _connect() keeps its schema-on-connect contract (several
    tests call it directly) and now self-closes if schema init fails.
  • Added regression tests:
    tests/gateway/test_delivery_ledger_fd_leak.py,
    tests/tools/test_async_delegation_fd_leak.py,
    tests/agent/test_verification_evidence_fd_leak.py.

Locking is unchanged: _transaction() never acquires the module lock, so each
call site keeps its exact existing _DB_LOCK behavior (no new lock nesting,
no deadlock risk).

How to Test

  1. pytest tests/gateway/test_delivery_ledger_fd_leak.py tests/tools/test_async_delegation_fd_leak.py tests/agent/test_verification_evidence_fd_leak.py -q
    → all pass. Each wraps sqlite3.connect to count opens vs closes and
    asserts every opened connection is closed (happy path, no-op update,
    exception mid-transaction, and schema-init failure).
  2. Reverting the close() makes the happy-path/early-return/exception tests
    fail with len(closed) == 0, confirming they catch the regression.
  3. Existing suites for all three modules pass:
    pytest tests/gateway/test_delivery_ledger.py tests/gateway/test_delivery_ledger_producer.py tests/tools/test_async_delegation.py tests/agent/test_verification_evidence.py -q

Checklist

  • I've read the Contributing Guide
  • Conventional Commits (fix(...))
  • Searched existing PRs to avoid duplicates
  • PR contains only changes related to this fix
  • Ran the affected suites and they pass
  • Added tests for the fix
  • Tested on my platform: macOS (Python 3.11)
  • Docs/config/tool-schema updates — N/A (internal resource-cleanup fix)

…, 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>
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/delegate Subagent delegation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 22, 2026
@egilewski

Copy link
Copy Markdown
Contributor

looks mergeable

The close-on-exit invariant is implemented consistently across all three SQLite ledgers, including no-op returns and failures during both statements and schema initialization. I reviewed the submitted head and a run-owned local semantic replay against current GitHub main; the focused regression suite failed all 11 cases on current main and passed all 11 on both review trees, with the replay retaining the newer WAL-fallback and durable-session schema changes.

Security evidence:

  • trust boundary: long-running gateway and agent processes repeatedly open SQLite-backed durable-state resources that must not outlive one operation.
  • source/sink/invariant: every ledger operation acquires through _transaction(); commit or rollback completes before the connection is deterministically closed, and _connect() also closes after initialization failures.
  • current-main reproduction: the focused suite failed 11/11; tracked public operations opened 15 connections across the three ledgers and closed none.
  • PR-head or patch-replay validation: the same suite passed 11/11 on the submitted head and 11/11 on a run-owned local semantic replay against current GitHub main.
  • positive/negative cases: covered successful operations, no-op updates, statement exceptions, rollback, and schema-initialization exceptions.
  • residual bypass search: no operational with _connect() call site remains in the three affected modules.
  • reviewer validation: CodeRabbit's suggested explicit rollback-state assertion would strengthen the regression test, but _transaction() delegates exception rollback to sqlite3.Connection.__exit__ before deterministic closure, so it does not change the production correctness conclusion.

Signed: GPT-5.6-sol-xhigh in Codex

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #70985 with your authorship preserved on the 3-ledger fix commit — thank you for the thorough fd-leak analysis and the test files, which shipped as-is. The salvage adds the cron executions ledger (#69594) and a fifth leak in the gateway readiness probe on top. Fixes #69678.

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/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/delegate Subagent delegation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQLite connections leaked (no close) in delivery_ledger, async_delegation, and verification_evidence ledgers — fd exhaustion, same class as #69567

4 participants