Commit graph

7 commits

Author SHA1 Message Date
Claude
da523c30e5
Torture harness, and the fixes it found
tests/connection_torture.py drives one Datasette from several event loops
(threads) with the smallest pool, 1ms idle timeout and 10ms schema polling
through every connection operation, plus add/remove_database, scratch
databases, external schema changes, replaced/deleted files, cancellation,
Database.close()/Datasette.close() mid-flight and non-threaded mode. One
scenario per process, so crashes and hangs are results. It checks for hangs,
crashes, undocumented exceptions, per-database write order, lost writes,
read-your-writes, catalog freshness, and fds/threads/the instance released
after close(). tests/test_connection_torture.py runs a short fixed-seed
subset of every scenario.

Fixes for what it found:
- A cursor that left the thread owning its connection (the cursor returned
  by execute_write_many()/execute_write_script(), one returned by a write
  callback, one rejected by execute_fn(), or one held by an exception's
  frames) could be freed on the event loop thread while the connection ran
  the same SQL again; on Python < 3.12 freeing it resets the shared cached
  statement, and the other query failed with "bad parameter or other API
  misuse". Such cursors are now closed on the owning thread. Pre-existing on
  main (51 failures per 1,800 multi-loop execute_write_many calls).
- invoke_startup() is single-flight across event loops (a concurrent.futures
  future, datasette.utils.inflight) instead of _startup_sequence() holding
  an asyncio.Lock: a second loop got "bound to a different event loop" or
  hung, and two loops could run the startup hooks twice. A request made
  from a startup hook no longer re-runs or deadlocks startup.
- The index page takes one snapshot of ds.databases: a database removed,
  renamed or closed mid-request, or whose file was deleted, no longer turns
  the page into a 500. Database.size is 0 for a deleted file.

The scan-coalescing helpers move to datasette/utils/inflight.py.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwkLo2rJWidpkDVT3HkvNv
2026-10-04 09:07:48 +00:00
Claude
d7a2ce91cc
Fix concurrency, lifecycle and contract problems from the second review
Schema watcher:
- Scan coalescing is loop-agnostic: in-flight scans are concurrent.futures
  futures any event loop can wait for, taken over only when their loop has
  closed or stopped; each refresh() resolves only its own scans. A write on
  one loop no longer hangs forever when another loop's scan is in flight.
- At most one polling task across event loops; replaced pollers exit.
- A scan or catalog write that fails transiently (internal database locked,
  shared-cache lock, cancellation) re-marks the database and retries it from
  later requests and sweeps, with back-off; requests are never failed by it.
  Write threads mark schema changes directly (under a lock) instead of
  relying on a notification reaching a loop that may have closed.
- add_database() file databases and named in-memory databases use the
  default_schema_watch mode (external unless configured). Named in-memory
  databases can be external: polled with PRAGMA schema_version on their
  write connection. They are not scanned at registration time, which raced
  plugins filling them (datasette-copy-to-memory).
- close() no longer persists a fingerprint for an owned database whose file
  header shows a schema the catalog has not seen.
- The derived-table permission rule (external-content FTS) trusts the
  catalog only when a stat and header read prove it is current; otherwise
  it asks the database, and schedules a rescan if the catalog is behind.

Connections and write threads:
- A BaseException from a write or isolated callback is delivered to the
  caller as a RuntimeError instead of killing the thread silently.
- conn.close() on a write lease raises ConnectionLeaseError, and the write
  thread reopens a connection something else closed.
- Write connections pin PRAGMA recursive_triggers=on (sqlite-utils FTS
  triggers rely on it; idle reopen lost the old drift).
- execute_fn() accepts generators that no longer hold the connection or a
  cursor.
- --crossdb _memory connections count against max_open_connections, are
  reaped, and are discarded when databases are added or removed.

Lifecycle:
- The temp internal database file uses weakref.finalize, not atexit, so
  closed or abandoned instances can be garbage collected.
- The read-pool reaper no longer keeps the last pool alive in its frame.
- The scratch directory is loaded (and locked) at the end of __init__, so a
  failed construction leaves nothing locked.
- config_dir/scratch is used only if empty or created by Datasette; a second
  instance on the same config dir starts without it; sidecar files are never
  deleted the first time a directory is opened.

Regression tests: tests/test_connection_review_fixes.py (24 of 27 fail on
a509b81; the other 3 guard behaviour that must not change).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwkLo2rJWidpkDVT3HkvNv
2026-10-04 08:32:09 +00:00
Claude
990c8782db
Merge spike/schema-watcher into spike/integrated, reconciling the three spikes
Read path: the leased pool replaces the thread-local cache. Pooled
connections record Database._conn_generation when opened; a stale one is
discarded on checkout or return, and _invalidate_connections() (called
by the SchemaWatcher on a replaced/deleted file) closes that database's
idle connections at once. Leased ones are only ever closed by their own
thread on release.

Write path: the idle-exit write thread keeps its lock protocol; its
connection lives in a _WriteConnection holder that is reopened when the
generation moves, before the next task. Post-write schema_version check
and _after_write catalog await are kept for every write API in both
modes. Non-threaded write connections are reopened on a generation change
and never cached half-prepared.

Deleted files: write connections use mode=rw once Datasette has seen the
file exist (_file_seen), so a deleted file is never silently recreated,
while a database added before its file exists is still created by its
first write.

Settings: max_open_connections (clamped to >= 4 x num_sql_threads),
connection_idle_timeout_ms (30000, read connections and write threads),
schema_watch_interval_ms, default_schema_watch (external|owned). The
spike-only knobs (connection_idle_timeout, connection_pool_wait_ms,
pool_read_connections, write_thread_idle_timeout_ms) are gone, along with
the pool's wait/disable code paths. The reaper falls back to checkout-time
reaping where threads cannot be started (Pyodide).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwkLo2rJWidpkDVT3HkvNv
2026-10-04 04:49:04 +00:00
Claude
c87eeceb48
Schema watcher: scan connections are untracked so close() cannot pull them from under a scan
remove_database() closes the Database, which closed every connection in
_all_connections - including a catalog scan connection still in use on a
worker thread: segfault (crashed an xdist worker in the full suite,
reproduced with an add/remove stress loop). Database.connect(track=False)
leaves the watcher's short-lived connections to the watcher, which always
closes them on the thread that opened them. Regression test added.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwkLo2rJWidpkDVT3HkvNv
2026-10-04 04:27:33 +00:00
Claude
a7359a7065
Schema watcher: lock-free header check when no -wal/-journal has content
When the fingerprint shows no non-empty -wal or -journal the main file is
the whole database, so the sweep reads the schema cookie (offset 40 of
the header) with pread instead of opening a connection. No SHARED lock
means no busy back-off for a concurrent writer: polling was costing
write-heavy phases ~10% throughput and up to 2x p99. The fingerprint is
still taken first, so a commit in flight is covered by the racy rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwkLo2rJWidpkDVT3HkvNv
2026-10-04 04:06:13 +00:00
Claude
3ec3b490df
Schema watcher: never recreate deleted files; O(1) derived-table cache key
- write connections reopened after the watcher saw the file replaced or
  deleted use mode=rw, so a write to a deleted database fails instead of
  silently creating a new empty file
- derived_table_dependencies() keys its cache on the schema_version the
  watcher stored in the catalog (plus its scan count) instead of running
  PRAGMA schema_version on a pooled read connection - allowed_resources()
  calls it for every database on every page

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwkLo2rJWidpkDVT3HkvNv
2026-10-04 03:51:24 +00:00
Claude
7a3c534ced
Spike: SchemaWatcher keeps the _internal catalog current off the request path
Per-database schema_watch modes (owned / external / immutable):
- write path reads PRAGMA schema_version after every write task and
  rebuilds that one database's catalog before the write returns
- external files are polled with a stat() fingerprint prefilter
  (racily-clean fingerprints re-verified), PRAGMA schema_version only on
  change, rebuild only when the version moved or the inode changed
- immutable databases are scanned once
- remove_database deletes its catalog rows explicitly; startup deletes
  rows for databases that are not attached (no per-refresh full scans)
- catalog reads use one short-lived connection per database inside one
  read transaction; shared-cache memory databases go through their
  write thread
- replaced/deleted files invalidate cached connections (generation)
- fingerprints persisted in catalog_databases.fingerprint (migration
  0002) so restarts skip unchanged files
- new setting schema_watch_interval_ms (default 1000, 0 = no polling)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwkLo2rJWidpkDVT3HkvNv
2026-10-04 03:18:16 +00:00