Skip to content

Database Layer Overhaul inclduing Thread Safety, Connection Management & Configuration#32

Merged
MatrixEditor merged 4 commits intoMatrixEditor:masterfrom
StrongWind1:fix/db-concurrency
Mar 21, 2026
Merged

Database Layer Overhaul inclduing Thread Safety, Connection Management & Configuration#32
MatrixEditor merged 4 commits intoMatrixEditor:masterfrom
StrongWind1:fix/db-concurrency

Conversation

@StrongWind1
Copy link
Copy Markdown
Contributor

Hey! While working on the SMB server and testing against a bunch of Windows clients at once, I ran into a database crash that turned out to be a threading issue in the DB layer. Fixing that led me down a rabbit hole of related improvements, so I ended up doing a thorough pass through all three files in dementor/db/. This PR brings the DB code in line with SQLAlchemy 2.0 best practices, adds a full test suite, and cleans up the configuration docs for users.

Nothing here changes the external API that protocol handlers use — add_auth(), add_host(), and the session.db.session property all work exactly as before. The changes are all internal.

The bug that started this

When multiple protocol handlers (SMB, HTTP, LDAP, etc.) were capturing hashes at the same time, the database would crash with pymysql.err.InternalError: Packet sequence number wrong, followed by an endless cascade of PendingRollbackError on every thread. Once it started, no more hashes could be stored for the rest of the session.

The root cause was in how scoped_session was being used. The code called scoped_session(factory)() at startup — that trailing () immediately created one Session on the main thread and stored it as a shared attribute. Every handler thread then used that same Session (and the same underlying database connection). With one client at a time that works fine, but under concurrent load the threads corrupt each other's connection state.

What's in this PR

Commit 1 — Code (dementor/db/)

Thread safety:

  • scoped_session is now stored as a registry, not a single instance. A session property returns each thread's own Session on demand — no shared state between threads.
  • Every public method wraps its DB work in try/finally and calls _scoped_session.remove() when done. This returns connections to the pool immediately instead of leaking them for the thread's lifetime.
  • The duplicate check and INSERT now run inside the same lock acquisition. Previously the check ran outside the lock, so two threads could both decide "not a duplicate" and both insert.
  • Every error path rolls back the session before re-raising, preventing one failure from poisoning every subsequent operation.
  • Pool exhaustion is handled gracefully (warning + skip) instead of crashing. The hash file stream is the primary capture path; the DB insert is secondary.
  • The "Captured" display message now only appears after a successful write. Previously it showed up even when the DB was read-only, which was misleading.

Code quality:

  • Split the 170-line add_auth() into _check_duplicate(), _log_credential(), and a focused add_auth() that orchestrates the two.
  • Replaced the manual lock.acquire()/lock.release() with no_lock boolean in add_host_extra() with a contextlib.nullcontext() pattern.
  • Fixed add_host() committing on every call even when nothing changed.
  • Fixed a missing commit() in add_host_extra() when updating an existing key.
  • Fixed the :memory: URL construction — it was missing the third slash, so SQLAlchemy parsed memory: as a hostname.

Engine configuration (per SQLAlchemy 2.0 docs):

  • :memory: SQLite uses StaticPool (DB only exists inside one connection).
  • File-based SQLite uses QueuePool (the new 2.0 default).
  • MySQL/MariaDB uses QueuePool with pool_use_lifo, pool_pre_ping, and a fast-fail timeout.
  • Added skip_autocommit_rollback and pool_reset_on_return=None to eliminate useless rollbacks under AUTOCOMMIT.
  • Dropped the deprecated future=True parameter.
  • Removed dead init_dementor_db() function and dead db_dialect/db_driver config fields.
  • Renamed db_raw_path to db_url to match the TOML key.

Constants:

  • Renamed _CLEARTEXT/_NO_USER/_HOST_INFO to CLEARTEXT/NO_USER/HOST_INFO with backward-compatible aliases so nothing breaks.

Commit 2 — Tests

124 tests total, all using SQLite — no external database needed for CI.

File Tests What it covers
test_db.py 121 Every function across all three DB files: constants, config loading, engine creation, host CRUD, credential storage, protocol resolution, duplicate detection (7 key-field variations), extras handling, logging output, error paths, connection release, thread safety
test_db_concurrency.py 2 20-thread stress tests on SQLite memory and file
test_version.py 1 Existing

Commit 3 — Config (Dementor.toml)

Rewrote the [DB] section so each option stands on its own with clear documentation:

[DB]

# Url (advanced) — full SQLAlchemy DSN, makes Path ignored
#   Url = "mysql+pymysql://user:pass@127.0.0.1/dementor"       # MySQL / MariaDB
#   Url = "postgresql+psycopg2://user:pass@127.0.0.1/dementor"  # PostgreSQL

# Path (default) — SQLite file
#   Path = "Dementor.db"            (default, in workspace)
#   Path = "/opt/dementor/creds.db" (absolute path)
#   Path = ":memory:"               (in-memory, lost on exit)

# DuplicateCreds — keep all hashes or deduplicate
DuplicateCreds = true

Commit 4 — Docs (database.rst)

Rewrote the Sphinx documentation to match the current code:

  • Added a "Choosing a backend" section with a comparison table
  • Each option documented with py:attribute, type/default, examples, and tips
  • Dialect and Driver marked as removed with versionremoved directives
  • Sphinx HTML build passes with -W --keep-going (zero warnings)

Live testing

Tested all database backends with multiple simultaneous Windows clients:

Backend Config Captures Errors Pool Warnings
Default SQLite file (omit both) 1,055 0 0
Custom file path Path = "/tmp/custom.db" 1,110 0 0
In-memory Path = ":memory:" 1,512 0 0
MariaDB (DuplicateCreds=true) Url = "mysql+pymysql://..." 1,484 0 0
MariaDB (DuplicateCreds=false) Url = "mysql+pymysql://..." 5 unique, 1,204 skipped 0 0
SQLite (DuplicateCreds=false) (default path) 5 unique, 1,034 skipped 0 0

Happy to adjust anything — let me know what you think!

Thread safety:
- Use scoped_session as a thread-local registry (property, not shared
  instance) so each handler thread gets its own Session and connection
- Add _release() via _scoped_session.remove() in try/finally on every
  public method to return connections to the pool immediately
- Atomic duplicate check + INSERT inside one lock scope to prevent
  TOCTOU race condition
- Rollback on every error path to prevent PendingRollbackError cascade
- Catch PoolTimeoutError gracefully (warning + skip, hash in file stream)
- Display logging gated on successful DB write

Code quality:
- Extract _check_duplicate() and _log_credential() from 170-line add_auth()
- Add _handle_db_error() shared helper for duplicated try/except
- Fix add_host_extra() lock: contextlib.nullcontext replaces manual
  acquire/release with bool flag
- Fix add_host_extra() connection leak on standalone calls (missing _release)
- Fix add_host() unconditional commit (only when values change)
- Fix missing commit() in add_host_extra() update branch
- Fix db_path for :memory: (was str(None))
- expire_on_commit=False so ORM objects survive _release()

Engine configuration (per SQLAlchemy 2.0 docs):
- :memory: -> StaticPool, SQLite file -> QueuePool, MySQL -> QueuePool
- skip_autocommit_rollback + pool_reset_on_return=None
- pool_use_lifo for natural idle-connection expiry
- Drop deprecated future=True, dead init_dementor_db(), dead config fields
- Fix :memory: URL (was missing third slash)
- Rename db_raw_path -> db_url

Constants:
- _CLEARTEXT/_NO_USER/_HOST_INFO -> CLEARTEXT/NO_USER/HOST_INFO
  with backward-compatible aliases
test_db.py (121 tests across 20 classes):
- __init__.py: constants, aliases, __all__ exports, normalize_client_address
- connector.py: DatabaseConfig defaults/loading, init_engine for all backends,
  create_db success and failure paths
- model.py: DementorDB init/lifecycle/close, session thread-locality,
  _release isolation, add_host, add_host_extra, add_auth, protocol resolution,
  duplicate detection (10 variations), extras handling, logging output,
  _check_duplicate, error handling, connection release, thread safety

test_db_concurrency.py (2 stress tests):
- 20-thread concurrent add_auth on SQLite memory and file

All tests use SQLite -- no external database required.
Three options, each with its own section header and examples:
- Url: advanced, for MySQL/MariaDB/PostgreSQL (makes Path ignored)
- Path: default SQLite backend (relative, absolute, or :memory:)
- DuplicateCreds: store all hashes or deduplicate

Removed stale commented-out Dialect/Driver fields.
- Add Choosing a backend section with comparison table
- Document all 3 active options (Url, Path, DuplicateCreds) with
  py:attribute directives, examples, bullet points, tips, and notes
- Mark Dialect and Driver as removed with versionremoved directives
- Sphinx HTML build passes with -W --keep-going (zero warnings)
Copy link
Copy Markdown
Owner

@MatrixEditor MatrixEditor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I will be reviewing the DB tests more thoroughly, but the overall concept is a good starting point,

@MatrixEditor MatrixEditor merged commit d086bf1 into MatrixEditor:master Mar 21, 2026
7 checks passed
@StrongWind1 StrongWind1 deleted the fix/db-concurrency branch March 22, 2026 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants