Data layer: security + performance review fixes - #15
Merged
Conversation
Full security and performance review of the merged multi-database work. Security found no exploitable issues (details below); performance found one real correctness- of-claims bug and two efficiency issues. Fixed: PERF #1 (high) - the ingest hot path was NOT pooled in production. DI registered the non-pooled AddDbContextFactory while only DispatchDbContextFactory.Create (used by the migrator and tests) was pooled. So the measured ~3,500 writes/sec came from the pooled TEST path, while production - SpoolWorkerPool creating a context per lifecycle-event insert and counter upsert, from many threads - ran the non-pooled path at roughly a third of that. My own comment claimed DI registered pooled; it did not. This is the exact "benchmark measures the wrong path" trap this whole effort kept surfacing, this time in my own number. Now AddPooledDbContextFactory, and DependencyInjectionTests asserts the resolved factory is pooled so it cannot drift again. PERF #2 (medium) - the Message Log dashboard read (PageAsync, the main list) grouped by (spool_id, CASE WHEN spool_id='' THEN id ELSE 0), whose CASE key is non-sargable: EXPLAIN showed USE TEMP B-TREE FOR GROUP BY, i.e. a full sort of every matched row, seconds per page on a large table under the default broad filter. Split into a sargable GROUP BY spool_id arm UNIONed with the anonymous (empty-spool) rows - EXPLAIN now serves the grouping straight from IX_relay_log_spool_id with no temp B-tree. Results are identical; the dedup test is strengthened to two denials (empty spool_id rows must each stay their own message, never collapse) and passes on all four engines. PERF #3 (low) - the storage admin page called GetTableSizeBytesAsync twice (relay_log, audit_log); on SQLite each runs a whole-file sampling pass including a linear COUNT, so it ran twice per page load. Added GetTableSizesBytesAsync (default loops; SQLite overrides to one pass) and the report now asks for both at once. SECURITY - reviewed and clean: * SQL injection: the only raw SQL left in the repositories is the counter upsert, whose column is enum-derived and values are parameters. All user input goes through EF LINQ (parameterised). Provider raw SQL carries only fixed table names (SafeIdentifier-guarded) and admin-config database names (per-engine escaped). * Command exec: migrate-database's chown uses ArgumentList (no shell). Hardened anyway to an absolute /usr/bin/chown path rather than PATH lookup, since it runs as root. * Credential exposure: connection strings are never logged - only the provider enum. * Encrypted config: AES-256-GCM, 600-perm key, decryption failure caught and treated as unset. Copied as ciphertext by the migrator, never decrypted in transit. * Auth vs collation: the case-sensitive collation STRENGTHENS API-key lookup - on a stock case-insensitive SQL Server install, key ids would match case-insensitively and the unique index would collide near-misses. Constant-time bcrypt paths intact. 72 repository tests pass on SQLite, PostgreSQL, MariaDB and SQL Server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
chrismuench
added a commit
that referenced
this pull request
Jul 20, 2026
…t 2-3x (#16) Following the pooling fix (#15) to its evidence exposed that my own commit overstated it. The fix corrected DI to register the pooled context factory and added a test that the resolved factory is the pooled TYPE - but a type assertion is not a throughput number, and the comments claimed pooling was "the difference between about 1,000 and several thousand concurrent writes per second." ProductionPoolingBenchmarkTests runs the ingest write pattern through the real AddDispatchData registration - the exact ILogRepository/ICounterRepository the service resolves - and against a hand-built non-pooled equivalent. Measured on SQLite, 16 writers: pooled 9,000-11,900/sec, non-pooled 7,600-8,800/sec. Roughly 1.1x-1.5x. Real, but not the headline the comments claimed. The ~3.5x jump earlier this branch (998 -> 3,509/sec) was the WAL synchronous=NORMAL pragma (ProviderConnectionInterceptor), which is I/O-bound and dominates because SQLite serialises writers on one lock regardless of context allocation. I conflated the two: pooling is the small CPU-bound saving on top, not the big one. Production before the pooling fix already had the synchronous fix (it shipped in #14 and applies to every path), so it was never at ~1,000/sec - it was around 8,000. The pooling fix took it to ~9,500. Corrected both comments to the measured figure and to distinguish the two fixes. Pooling is still worth registering - it is free once the factory exists and it is what makes the test and production paths measure the same thing - but the reason is correctness of measurement, not the speed-up. Co-authored-by: Chris Muench <chris@MacBook-Air.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full security and performance review of the merged multi-database work (#14), with fixes.
Performance
#1 (high) — ingest hot path wasn't pooled in production. DI registered the non-pooled
AddDbContextFactory; onlyDispatchDbContextFactory.Create(migrator + tests) was pooled. So the measured ~3,500 writes/sec was the pooled test path — production ran the non-pooled path at roughly a third of that, and my own comment wrongly claimed DI was pooled. NowAddPooledDbContextFactory, withDependencyInjectionTestsasserting the resolved factory is pooled so it can't silently drift again.#2 (medium) — Message Log dashboard read did a full sort per page.
PageAsyncgrouped by(spool_id, CASE WHEN spool_id='' THEN id ELSE 0); theCASEkey is non-sargable.EXPLAINshowedUSE TEMP B-TREE FOR GROUP BY— a sort of every matched row, seconds per page on a large table under the default filter. Split into a sargableGROUP BY spool_idarmUNIONed with the anonymous rows;EXPLAINnow serves it straight fromIX_relay_log_spool_id, no temp B-tree. Dedup results are identical (test strengthened to two denials, green on all four engines).#3 (low) — storage page sampled twice on SQLite.
GetTableSizeBytesAsynccalled per table re-ran the whole-file sampling pass (incl. a linearCOUNT). Added a batchGetTableSizesBytesAsync.Security — reviewed, no exploitable issues
chownusesArgumentList(no shell); hardened to an absolute path since it runs as root.72 repository tests pass on SQLite, PostgreSQL, MariaDB, SQL Server.
🤖 Generated with Claude Code