Give each request and worker its own database session

[new-usemame]

Addresses #1121. Also reproduces the mechanism behind #1048, which #1121 had recorded as a hypothesis.

Why

CalibreDB.session_factory is a scoped_session, which exists to hand each thread its own Session. init_session() then did:

self.session = self.session_factory()

storing that thread's Session on a single shared attribute of a module-level singleton. Whichever thread called init_session() most recently published its Session there, and all ~270 calibre_db.session call sites read that one. The thread-locality the wrapper exists to provide was discarded by the assignment.

This matters more here than in a typical Flask app because CWNG runs gevent without monkey.patch_all(), so WorkerThread is a real OS thread rather than a greenlet — two genuinely concurrent users of one Session.

Root cause

A single shared attribute standing in for per-thread state. Not the background tasks themselves: every one of them inherits the defect, so the symptom reappears in unrelated places with unrelated-looking tracebacks.

Reproduction

Two levels, both committed.

The mechanism (test_1121_session_is_thread_local.py, from #1122, previously quarantined as RED-by-design). Against main:

AssertionError: the request thread's session changed underneath it: it read
4490054112 at the start and 4490054208 after the worker ran, which is the
worker's (4490054208).

The symptom (test_1121_worker_close_during_request_fetch.py, new). #1121 asked for this specifically — "queue a manual scan while a slow paginated browse is streaming, and assert the browse completes" — and noted two earlier attempts had failed because closing and re-using a Session sequentially is harmless (a Session re-acquires a connection). This builds the genuine interleave: the reader iterates yield_per so rows arrive from a live cursor, pauses mid-stream, and only then lets the worker run duplicate_scan's teardown shape (ensure_session() … session.close()). Against main the reader dies:

InvalidRequestError: Object <_Row> cannot be converted to 'persistent' state,
as this identity map is no longer valid. Has the owning Session been closed?

That is the reported mechanism — a background task tearing down the session a request is streaming from. Honest scope: the reporter's traceback on #1048 surfaced as sqlite3.ProgrammingError from _raw_all_rows rather than this error. What is reproduced here is the mechanism; which error surfaces depends on how far into the fetch the close lands. I am not claiming the reporter's exact frame is reproduced.

Fix

session becomes a property that resolves through the scoped_session registry on every read, so thread-locality survives. The setter is kept because callers assign to it — cps/editbooks.py:1072 drops a poisoned session with calibre_db.session = None, and tests substitute stubs — but those assignments are now scoped to the assigning thread instead of leaking to every other one.

Three call sites needed care, each found by the test suite rather than by inspection:

• __init__ no longer assigns self.session = None. That would run the setter and tear down the calling thread's session, and CalibreDB() is constructed inside request handlers (cps/web.py:1373, magic_shelf.py).
• The thread-local slot is created on demand, not only in __init__: several tests build instances with CalibreDB.__new__(CalibreDB) to avoid needing a Flask app, and a property that raises AttributeError on a partially constructed instance is a worse contract than the plain attribute it replaced.
• session needs a deleter. mock.patch.object(calibre_db, "session", ...) records that the attribute was absent from __dict__ and restores by deleting it, so without one every such patch raises on exit.

dispose() reads a new _peek_session() rather than the property, so it does not create a Session for the disposing thread purely to close it.

One incidental improvement: threads now read the current session_factory on every access, so a factory rebuilt by setup_db reaches every thread rather than only the one that ran the loop.

What this does NOT fix

Worth stating plainly, because the title could be read as "calibre_db is now thread-safe" and it is not.

notes/fix-udf-gil-deadlock-DESIGN.md records that StaticPool is architecturally required here: the calibre engine is sqlite:// with per-connection ATTACH of calibre.db + app.db, and per-thread connections (QueuePool/NullPool) were considered and rejected as a much larger change. So every thread still shares one DBAPI connection, and transaction boundaries on it remain process-global — a commit() on one thread still commits another thread's in-flight writes.

That is not a regression: with a single shared Session the same was true, plus the identity map was shared and could be closed underneath a reader. This change removes the Session sharing, which is what the reported crashes come from. The residual connection-level sharing is worth a follow-up issue, not a blocker.

Validation

• Both test files: RED against main, GREEN on this branch (evidence quoted above).
• Full tests/unit + tests/smoke: 4172 passed, 8 failed — the 8 are byte-identical to main's pre-existing baseline, so zero regressions. Baseline captured by running the same suite on main in the same venv.
• The blast-radius pass earned its keep: the first version of this change caused 38 regressions, the second 25. Both were contract gaps in the property (missing lazy init, missing deleter), not in the design, and both were fixed rather than worked around.

One caveat recorded rather than hidden: during an intermediate state (deleter not yet added, so patch.object teardown was raising) one suite run stalled for ~10 minutes at 0.6% CPU without pytest-timeout firing — the signature of the known GIL↔sqlite-mutex class in that design note. It did not recur: the final code ran the full suite to completion twice, in 185s and clean. Flagging it because that class is hard to trigger and unpleasant to meet in production; I could not reproduce it against the committed code.

Tier

needs-review — fork-original change to the highest-traffic file. Reviewed cross-family before merge per the autonomous merge gate.

---

Also fixes #1150 — the greenlet half of the same question

The review of this PR surfaced a second defect, filed as #1150 and reproduced on
main. It is the other half of "what is the lifetime of a Session here", so it
is fixed here rather than in a follow-up.

Why the thread fix alone was not enough

Restoring thread-locality is necessary but not sufficient. Requests are gevent
greenlets and monkey.patch_all() is deliberately absent, so every concurrent
request runs on one OS thread and threading.local() cannot separate them.
They therefore still shared a Session — while shutdown_session
(cps/__init__.py:469-482) calls session_factory.remove() on every request
teardown.

So the first request to finish closed the Session the others were still reading
from. The victim failed wherever it happened to be, which is why this family has
produced a different-looking traceback every time (#1048, #1121, CWA #1228):

InvalidRequestError: Instance '<Books at 0x...>' is not persistent within this Session

This is on main today and pre-dates this PR. The property alone would have made
it sharper, since a foreign remove() then changes what calibre_db.session
returns mid-request.

Fix

setup_db now builds the factory through _make_session_factory(), which passes
scopefunc=greenlet.getcurrent.

greenlet.getcurrent() is a distinct object per greenlet and a distinct root
greenlet per OS thread — measured, 4 distinct across main + 3 threads. So
greenlet scope subsumes thread scope rather than replacing it: worker threads
stay isolated exactly as the fix above made them, and request greenlets are now
isolated from each other too. Per-request remove() becomes correct instead of
destructive, because it can only reach the caller's own Session.

A leak the scopefunc introduces, found by measuring rather than assuming.
Passing any scopefunc swaps SQLAlchemy's ThreadLocalRegistry (a
threading.local, freed with its thread) for ScopedRegistry's plain dict,
which retains a Session for every greenlet that never reaches a teardown —
measured at 50 retained for 50 dead greenlets. Greenlets are
weak-referenceable, so the registry map is a WeakKeyDictionary, which measures
0. Note this means the earlier "the registry does not leak" finding was about
ThreadLocalRegistry and does not carry over.

greenlet is already a pinned hard requirement (requirements.txt:40) and a
dependency of gevent, so no new dependency. The ImportError fallback to
default thread scope exists only for the tornado path, where there are no
greenlets and thread scope is already correct.

One test needed repairing, and it was a real gap

test_calibre_session_configures_autoflush_on (janeczku#3670, case-variant tags)
source-pinned setup_db for the string autoflush=True. Extracting the factory
moved that string out of the function the pin was reading, so the pin would
have gone green while looking at nothing. It now builds a factory and asserts
the Session it hands out has autoflush enabled — the property the behaviour
actually depends on, which no relocation can hide. Verified falsifiable: flipping
autoflush to False in _make_session_factory fails it.

Validation

• tests/unit/test_1150_greenlet_scoped_sessions.py, 7 tests: the interleave
  above, per-greenlet isolation, per-thread isolation still holding, the registry
  leak guard, session identity stable across a foreign teardown, and a scope-key
  pin.
• Falsifiability control (notes/verify/FAILURE-MODES.md class 9b): the red
  test cannot be run against main — the function it imports does not exist
  there. So the file also runs the identical interleave against the factory
  main builds and asserts it still crashes. If that stops failing, the guard is
  worthless and the suite says so.
• Full tests/unit + tests/smoke: 8 failed / 4186 passed, the same 8 as
  main's baseline. The autoflush pin above was a 9th until it was repaired.
• Live in cwn-local: the mechanism reproduces under the container's own
  runtime (py3.13.14, SQLAlchemy 2.0.51 — a different version than the test
  venv's 2.0.49, so it is not version-specific) and stops reproducing with the
  fixed cps/db.py copied in. The deployed module really carries
  ScopedRegistry + greenlet.getcurrent + a weak-keyed map.
• Concurrency, class 7: 900 parallel requests (xargs -P 16) across / and
  /book/<id> — 0 error signatures, 0 tracebacks, library still renders.
  Stated honestly: this is a null result, not proof. A 42-book local library
  serves pages too fast to reliably produce the overlap the bug needs, so the
  hammer cannot discriminate it — its value here is evidence of no regression
  under concurrency, and the discrimination comes from the two reproductions
  above.
• Blast radius checked by hand: no gevent.spawn in cps/ outside the WSGI
  server, so requests are the only greenlets; and none of the three
  parallel.fan_out callsites (search_metadata, cover_booster,
  cover_picker) touch the ORM session in their workers — they are HTTP provider
  calls. So no request spans two greenlets while sharing a Session.

Still not fixed, deliberately

• Transaction boundaries remain process-global. StaticPool keeps one DBAPI
  connection (required, see notes/fix-udf-gil-deadlock-DESIGN.md), so a
  commit() still commits another in-flight write. Measured, unchanged by this,
  and equally true when every greenlet shared one Session. Tracked separately.
• cps/ub.py has the same defect at larger scale — init_db() collapses a
  scoped_session into one shared global, ~434 call sites across 33 files,
  documented in notes/cwa-1228-shared-session-gevent-DESIGN.md and deferred
  since 2026-05-07. Not touched here.

/security-review not run: this changes ORM session scoping, not auth/session,
CSRF, crypto, subprocess, user-controlled paths, or routes. Worth noting the
change moves away from two concurrent users' ORM objects sharing one identity
map.

Cross-family review (Terra, resumed thread) — disposition

Two HIGHs raised. Both proofed rather than taken on trust; one holds in reduced
form, one does not.

HIGH 2 — "N sessions on one StaticPool connection is a NEW failure mode."
NOT CONFIRMED. The claim was that B's teardown rollback can now discard A's
uncommitted work, where previously one shared Session meant one transaction
controller. Measured both shapes on the same interleave (A writes + flushes,
yields, B's teardown fires, A commits):

	A's commit	A raised	A's row after
shared session (main today)	ok	none	lost
per-greenlet (this PR)	ok	none	lost

Identical. The cross-transaction damage is a pre-existing property of one
process-wide DBAPI connection, not something this diff introduces — and the
predicted "cannot start a transaction within a transaction" did not occur in
either shape. What the diff removes is the additional harm the shared shape
had on top of this: destroying the other request's identity map, which is #1150.
Downgraded to the already-documented StaticPool caveat.

HIGH 1 — "refresh_for_new_data() no longer reaches other requests'
sessions." CONFIRMED, severity reduced to low. It iterates CalibreDB.instances
and expires inst.session, which now resolves to the calling greenlet's
session, so concurrently in-flight requests keep their snapshot. Real, and the
docstring's "on every instance" is now inaccurate. Scope, checked: the only
caller is the manual reconnect endpoint (cps/cwa_functions.py:684), sessions
live exactly one request (teardown removes them), so every request after the
refresh sees the new data and the staleness window is the tail of requests
already in flight. Not a blocker, not silently accepted: filed as a follow-up
with the measurement.

Review checks 1, 2, 3 and 5 returned no finding: no scope-key collision between
greenlets or threads, WeakKeyDictionary is compatible with the ScopedRegistry
operations scoped_session uses (the key is weak, not the value, and a runnable
greenlet stays strongly reachable), dispose()/editbooks recovery/desktop-compat
behave as intended, and no current production path creates a session outside a
Flask app context. Noted for the future: reaching into factory.registry.registry
is an implementation detail, and any future non-request greenlet touching
calibre_db.session needs an explicit cleanup wrapper.

[new-usemame]

Review disposition — cross-family (Terra) + proofing pass

Terra reviewed cps/db.py adversarially (thread 019f9d83-f152-7832-88a5-3bebbdf0f57e); I verified each finding empirically rather than taking it at face value, and found one it missed. Not merging yet — one blocker is on the hot path.

🔴 BLOCKER (found in the proofing pass, not by Terra or the PR)

calibre_db.session is no longer stable within a single request, under gevent.

This PR reasons about WorkerThread as a real OS thread, which is right. But it doesn't account for how web traffic is served: cps/server.py runs gevent.pywsgi.WSGIServer with a Pool, and there's no monkey.patch_all(), so every request greenlet shares one OS thread — therefore one scoped_session registry entry, one Session.

Meanwhile shutdown_session (cps/__init__.py:481) calls session_factory.remove() on every request teardown. Measured against this branch:

same session object for both greenlets: True
B's session object after A's teardown is the SAME: False
  B captured : 4475681824
  B now reads: 4465074416

So greenlet A finishing its request silently swaps the Session out from under greenlet B, mid-request. Before this change B kept reading the same object off the shared attribute (closed, but re-usable — a closed Session re-acquires a connection). Now a read early in a request and a commit() later in the same request can land on two different Sessions.

That converts a background-task-only failure into one reachable by any two concurrent requests, which is the opposite of the intent. It needs a design answer (a generation counter, or teardown that doesn't remove() while greenlets are in flight) before this can ship — and per the release policy a published tag can't be retracted, so it gets answered first.

🟠 Confirmed, not blocking

• dispose() can't reach foreign threads' sessions (Terra, HIGH → I assess 🟠). Correct that _peek_session() + inst.session = None only touch the disposing thread. Reduced severity because reconnect_db calls setup_db(), which installs a new session_factory; the property reads type(self).session_factory fresh, so a foreign thread's next read correctly builds from the new factory. The real residue is orphaned sessions in the old registry that never get closed, plus any caller holding s = calibre_db.session in a local across the reconnect.
• refresh_for_new_data() only refreshes the calling thread (Terra, HIGH → 🟠). Structurally true. Impact is lower than stated: since all web greenlets share one OS thread, calling it from a request does expire the session every web request reads, which is the user-visible purpose (new books visible after ingest). The worker's session isn't reached.
• expire_on_commit=False is silently dropped when the thread already materialised a session (fresh is False, so the preference isn't applied). Measured: worker_wanted=False got=True. Latent only — all 12 production callers pass init=True, and init_session() sets it explicitly. A future caller without init=True gets a silent downgrade.

🟡 Noted

• Unsynchronised check-then-set in _local can lose an override for CalibreDB.__new__()-built instances (Terra, MEDIUM). Not reachable for normally constructed instances.

✅ Verified sound

• No registry leak. Terra flagged possible accumulation; measured 0/50 sessions from dead threads still alive — SQLAlchemy's ThreadLocalRegistry is threading.local(), freed with the thread.
• Ordinary getter/setter/deleter isolation for an initialised instance is correct.
• Contract tests added in 7510bfe: red on main (5 of 6, including the cross-thread one), green here.

Where this leaves the PR

The core diagnosis is right and the OS-thread half of the fix is well built. What's missing is the greenlet half — session resolution and per-request remove() have to agree. Staying needs-review with the blocker tracked; next pass takes the lifecycle design.