Fix bugs and optimize performance#71
Merged
Merged
Conversation
zas
force-pushed
the
fix/audit-bugs
branch
4 times, most recently
from
84dee5f to
c4d2f81
Compare
mwiencek
reviewed
May 15, 2026
Comment on lines
+60
to
+59
| self.engine = sqlalchemy.create_engine(self.config.database.create_url(), poolclass=NullPool) | ||
| self.engine = sqlalchemy.create_engine(self.config.database.create_url()) |
Member
There was a problem hiding this comment.
It should improve app performance though at the cost of reducing the available pgbouncer pool size for musicbrainz-server. However, we do have two pgbouncer-slave containers, so it might be fine. We agreed to test this in production on one of the containers.
mwiencek
reviewed
May 15, 2026
mwiencek
reviewed
May 15, 2026
…it split Remove spurious Sentry capture for expected ValueError in handle_options.
Eliminates one DB round-trip per request by inlining the GID redirect resolution into each resolve method's WHERE clause. Also removes the now-unused resolve_mbid() method and its separate call in handle(). Benchmark (10000 iterations, QueuePool, local PostgreSQL): 2 queries: 0.317 ms/req (baseline) 1 merged: 0.191 ms/req (1.66x faster, 40% less DB time)
Adds StaticCache that stores file content + mtime. Hot path is a dict lookup + time.monotonic() comparison (no syscalls). Re-checks mtime every 5 minutes; only re-reads on change. All static endpoints (HTML index pages, SVG images) use the shared _serve_static helper. Benchmark (50000 iterations): File type Disk read Cached Speedup HTML 10.1 µs/req 0.2 µs/req 63x SVG 10.2 µs/req 0.2 µs/req 64x
Defense-in-depth: use SQLAlchemy's identifier_preparer.quote() for interpolated table names in _resolve_entity_id_sql, as suggested in PR review.
…ibutes Remove unused LocalSysLogHandler Remove unused statuscode() Replace unused id with _ in thumbnail() Rename type parameter to cover_type (type is a builtin)
Set pool_mode=queue in config.ini to use QueuePool with persistent connections. Defaults to NullPool (no persistent connections). QueuePool benchmark (1000 iterations, local PostgreSQL): Endpoint NullPool QueuePool Speedup release/front 6.08 ms 1.22 ms 5.0x release/index 4.68 ms 0.94 ms 5.0x release-group/front 8.38 ms 2.29 ms 3.7x release/front (404) 7.04 ms 1.81 ms 3.9x QueuePool defaults: pool_size=2, pool_max_overflow=3, pool_recycle=300.
Contributor
Author
|
@mwiencek I reworked the PR, kept the default NullPool but added a config way to change it to QueuePool if we want to evaluate, dropped useless changes, and grouped commits when it was making sense. |
mwiencek
reviewed
May 18, 2026
…uery redir.new_id already holds the entity id, so the JOIN to the entity table was unnecessary. Suggested-by: Michael Wiencek <mwiencek@users.noreply.github.com> See: #71 (comment)
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
2 participants
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.
Summary
Bug fixes, performance optimizations, and cleanup for the artwork redirect service.
Changes
Bug fixes:
handle_redirectreturning a list instead of aResponseon empty filenamestatuscodeimport (dead after the above fix)test_all.pyimportingload_configfrom wrong modulehandle_options: remove dead guard, redundant None check, limit split; remove spurious Sentry capturePerformance:
resolve_mbidinto each query via COALESCE subquery (eliminates 1 DB round-trip per request, ~40% less DB time)Security / hardening:
identifier_preparerCleanup:
LocalSysLogHandler,statuscode(), dead attributestypeparameter tocover_type(shadows builtin)utils.pyTooling:
benchmarks/with auto-serve and status code reportingTesting
All 36 unit tests pass. Integration tests require the Docker-based database.