Skip to content

fix(neo4j): reuse one bounded driver across per-session graph stores - #91

Merged
Salil Das (sadlilas) merged 5 commits into
mainfrom
fix/neo4j-bounded-shared-driver
Sep 1, 2026
Merged

fix(neo4j): reuse one bounded driver across per-session graph stores#91
Salil Das (sadlilas) merged 5 commits into
mainfrom
fix/neo4j-bounded-shared-driver

Conversation

@colombod

@colombod Diego Colombo (colombod) commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Problem — bolt-driver resource exhaustion

The drain path built a new AsyncGraphDatabase driver for every session_id. Each of those drivers ran its own connection pool at the neo4j-python default of 100, so the pool was bounded but the number of pools was not. Per-session drivers are closed only on clean finalize, cancel, or the 5-day idle reap — none of them reachable while Neo4j is under load — so bolt connections accumulated until the server's bolt thread pool starved and ingest backpressured to clients.

This is a standalone, ship-first extraction of the resource-exhaustion fix, off main; the rest of the hardening stack rebases on top of it.

Fix

  • SessionRegistry builds one shared, pool-bounded Neo4j driver (lazily, on first session) and injects it into every Neo4jGraphStore it constructs.
  • Neo4jGraphStore accepts an optional pre-built driver and tracks owns_driver; close() only closes a driver it owns — so a per-session finalize can never tear down the driver other live sessions are still using.
  • The shared driver is closed exactly once, at lifespan shutdown, and only after the drain workers have been quiesced (see below).
  • Driver kwargs live in one helper (build_bounded_neo4j_driver in neo4j_store.py) so the admin driver, the query driver, the doctor CLI, and the registry's shared driver can never diverge.
  • New setting: neo4j_max_connection_pool_size (default 50, validated > 0). The neo4j driver's own default is 100; 50 is a deliberate reduction, well under the server's default bolt thread-pool size, now that every session shares one pool instead of holding a private one.

There is deliberately no max_connection_lifetime setting: the driver already recycles pooled connections at 3600 s by default, so a knob whose default equalled the library default would change nothing.

Shutdown: quiesce the drainers before closing the shared driver

Sharing the driver makes its lifetime a cross-session concern, and that turns process shutdown into a correctness problem it never was before.

A drain worker that meets a closed shared driver fails its batch, spends its max_delivery_attempts budget in ~250 ms (5 attempts × the 50 ms _DRAIN_POLL_INTERVAL backoff), and lands in _handle_exhausted_batch — which dead-letters each line and commits the queue offset past it. Those are healthy events that merely happened to be queued when the process stopped, and once dead-lettered they never replay.

SessionRegistry.shutdown_workers() cancels and awaits every drain worker, and the lifespan finally calls it before any driver closes. Cancellation routes each drainer through its existing CancelledError handler → _safe_close(worker) → a final flush on a driver that is still open; anything left uncommitted stays in the durable queue and replays on the next boot.

Measured against a live Neo4j, 400 events queued to one session with the drainer running:

shutdown sequence events dead-lettered
driver left open (control) 0 of 400
close the shared driver with drainers still live 100–400, in 8 of 8 trials (one trial: all 400)
quiesce, then close (this PR) 0 of 400, in 6 of 6 trials

Driver-kwarg parity

Routing per-session driver construction through the shared helper must not quietly change how those drivers behave. build_bounded_neo4j_driver carries over both kwargs the per-session driver set:

kwarg old per-session driver shared driver lifespan admin/query
max_connection_pool_size 100 (library default) 50 50
connection_acquisition_timeout 30.0 (neo4j_lock_timeout) 30.0 60.0 (library default, unchanged)
max_transaction_retry_time 30.0 30.0 30.0

connection_acquisition_timeout matters more after this change, not less: one bounded pool is now shared by every session, so waiting on a free connection is reachable in a way it was not when each session held a private 100-slot pool. The lifespan admin and query drivers pass no acquisition timeout, preserving exactly what they did before they were routed through the helper.

Evidence (live Neo4j)

The proving test drives 30 concurrent sessions through get_or_create against a real Neo4j container and counts actual Python bolt connections on the shared driver:

[driver-leak evidence] sessions=30 pool_size=8 baseline=0 during_load=8 after_close=0
  • Bound to the pool, not one-per-session: during_load (8) < SESSION_COUNT (30) and <= pool_size + 2. One shared driver served all 30 sessions.
  • No hanging drivers / full reclaim: after_close == 0 — every bolt connection released once the shared driver is closed.
  • Per-session close is safe: test_session_a_close_does_not_disrupt_session_b proves closing one session leaves the shared driver working for others.
  • Shutdown discards nothing: test_shutdown_quiesce_then_close_deadletters_nothing dead-letters 0 events; removing the quiesce makes it fail with 200 events dead-lettered and error="Driver closed".

Each regression guard was verified to actually fail without its fix: deleting the driver injection makes the live leak test report 30 open connections against a pool bound of 8, and makes 5 of the CI-run unit tests in tests/test_neo4j_driver_sharing.py fail.

Test results

  • tests/neo4j/ (live container) — 92 passed
  • Full -m "not neo4j" suite — 1945 passed, 8 skipped
  • ruff check clean; ruff format --check clean on every changed file

Notes

  • +998 / −56 across 11 files. Merged with main after the durable-queue hardening work landed; re-verified against the reworked drain path.
  • Extracted from stack commits 75e2fdd + f4ef6b5; incidental edits to test_boot_safety.py / test_writer_lease.py were dropped because those files do not exist on main.

🤖 Generated with Amplifier

The drain path built a new AsyncGraphDatabase driver for every session_id
with no pool bound, so bolt connections accumulated without limit until the
server's bolt thread pool starved and ingest backpressured to clients.

SessionRegistry now builds one shared, pool-bounded driver (lazily, on first
session) and hands it to every Neo4jGraphStore it constructs. Neo4jGraphStore
accepts an optional pre-built driver and tracks whether it owns it; close()
only closes a driver it owns, so a per-session finalize can never take down
the driver other live sessions are still using. The shared driver is closed
exactly once, at lifespan shutdown.

The pool-bounding kwargs (max_connection_pool_size, max_connection_lifetime)
live in one helper in neo4j_store.py so the lifespan admin driver, the query
driver, the doctor CLI, and the registry's shared driver can never diverge.
Config gains neo4j_max_connection_pool_size (default 50) and
neo4j_max_connection_lifetime (default 3600s). A live-Neo4j test proves the
pool stays bounded across 30 sessions and returns to zero after close.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Diego Colombo (colombod) added a commit that referenced this pull request Aug 27, 2026
Extracts the queue-hardening slice from the squashed harden-durable-ingestion
branch (PR #78), on top of the shared-driver base (PR #91). Writer-lease
enforcement is a separate, later slice and is excluded entirely.

- queue_manager.py / idempotency.py: taken wholesale from the source branch
  (near-rewrites: per-session file-lock record framing, dual-format
  committed-offset reader, continuous committed-prefix compaction, boot
  reclamation of provably-drained logs, per-key async idempotency lock).
- registry.py: supervised drain loop with poison-batch isolation that
  finalizes correctly when a batch carries the terminal record, finalize
  retry on a non-drained delete, and the shared Neo4j driver wiring from
  PR #91 preserved unchanged.
- main.py: boot backgrounded into schema -> heal -> reclaim -> expire ->
  reconcile -> seed -> topup -> sweep, each phase timeout-bounded, so a
  Neo4j-unreachable schema init no longer blocks first request; ingest's
  idempotency check/append/store sequence is serialized per key by an
  async lock (no lease involved); crash-recovery topup falls back to a
  session's byte-0 line when its head is unparseable.
- config.py: queue self-bounding settings only (compaction thresholds,
  dead-letter retention, boot-phase timeout); no writer-lease settings.
- status.py: BootState surfaced on /status (reclaim/resume/defer counters
  and boot phase); no writer-lease field.
- handlers/data_layer_2/session.py: session:end no longer self-flushes --
  the drainer's gated flush barrier is now the sole trigger; the ended-node
  upsert is seeded with the labels already read so it can't shadow a
  persisted terminal type.
- tests: ported the concurrent-append stress suite (multi-session and
  single-session contention, >1 MiB records) and the queue adverse-state
  suites (boot safety/reclaim, drain supervision, finalize-delete ordering,
  idempotency-store-on-success, large-event tail drop, steady-state
  reclaim, drain lifecycle logging), all with writer-lease-specific cases
  removed.

No writer_lease references remain anywhere in shipped code or tests.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Salil Das (sadlilas) and others added 4 commits September 1, 2026 06:33
- Quiesce drain workers before closing driver in lifespan shutdown to prevent data loss
- Restore connection_acquisition_timeout and max_transaction_retry_time to shared driver
- Remove no-op neo4j_max_connection_lifetime setting

Fixes data loss (dead-lettering of queued events) at shutdown. Adds regression tests
for shutdown ordering, driver kwarg parity, and live-Neo4j shutdown behavior.

Hardens PR #91.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Issues are disabled on this repository, so the bare "#489" in the
shared-driver test docstring could only resolve somewhere else -- a
dangling pointer for anyone reading the code. Replaced with a
self-describing statement of the property under test.

Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…session driver

The topology section said the server uses two drivers and that the admin
driver "carries the ingest path -- the drainer's batch flushes". Neither
is true now: the drainer's flushes go through the registry's shared
session driver, and /status probes only the admin and cypher_query
drivers, so neo4j_connected: true no longer implies a healthy ingest
path. An operator debugging ingest backpressure -- the exact failure this
change fixes -- would have been pointed at the wrong driver.

Documents all three drivers, which one carries ingest, the shared
driver's lazy build and bounded pool, and the shutdown ordering
requirement.

Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
main advanced with the durable-queue hardening work, which reworked the
drain path this change's shutdown fix interacts with. Conflict was a
single comment block in SessionRegistry.__init__ (kept the shared-driver
field, took main's shortened counter comment).

Re-verified against the merged drain path: closing the shared driver
under live drainers still dead-letters healthy queued events (188 of 400
in a live run), so the shutdown_workers() quiesce is still required and
still correct. The new supervision callback ignores cancelled tasks, so
it does not fight the quiesce.

Merged verification: 1945 passed / 8 skipped (not neo4j), 92 live-Neo4j
tests passed, ruff clean. All four regression guards re-confirmed to fail
when their fix is removed.

Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@sadlilas
Salil Das (sadlilas) merged commit b5428d3 into main Sep 1, 2026
3 checks passed
@sadlilas
Salil Das (sadlilas) deleted the fix/neo4j-bounded-shared-driver branch September 1, 2026 14:04
Diego Colombo (colombod) added a commit that referenced this pull request Sep 9, 2026
This release ships production bug fixes (Neo4j query-plan fixes that resolved
ingest stalls in #91, #94, #95, #96). The version bump restores per-build
identifiability: four prior fix PRs merged without bumping, causing 6.7.1 to
identify five distinct builds. /version and /status.server_version now correctly
distinguish this deploy.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Diego Colombo (colombod) added a commit that referenced this pull request Sep 9, 2026
This release ships production bug fixes (Neo4j query-plan fixes that resolved
ingest stalls in #91, #94, #95, #96). The version bump restores per-build
identifiability: four prior fix PRs merged without bumping, causing 6.7.1 to
identify five distinct builds. /version and /status.server_version now correctly
distinguish this deploy.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants