fix: @thumbor-auth prefers storage conn over psycopg pool (related to #8) - #10
Merged
Merged
Conversation
The REST service's per-image SQL auth check called pool.getconn(), which contends with other request-scoped catalog queries for the same per-pod psycopg pool. Under cold-cache production load this was a contributor to the PoolTimeout incident (#8). Prefer the ZODB storage connection (already held for the request) so the auth check doesn't touch the pool at all. Falls back to the pool when no ZODB storage is in scope (tests, scripts, non-Zope contexts). The SQL itself is unchanged — semantics preserved. Related to #8. Co-Authored-By: Claude Opus 4.7 (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
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
ThumborAuthServiceinrestapi.py.get_storage_connection(self.context)— the ZODB storage's PG connection already held for the request — overpool.getconn(), eliminating pool contention for the@thumbor-authcode path.Why minimal (not a full rewrite)
Unlike
_needs_auth_url(closed by #9), this service answers a multi-principal question:rolesForPermissionOnalone is not sufficient here —allowed_rolesalso carriesuser:<id>tokens from local-role grants, whichrolesForPermissionOndoesn't return. A full Zope-native rewrite (load the object, askcheckPermission("View", obj)against the current security manager) is a larger change, trades one PG round-trip for one ZODB load, and changes semantics (live vs. cached). Worth considering separately, but out of scope for the pool-saturation fix.This PR is the smallest change that stops
@thumbor-authfrom contending on the psycopg pool. It:pool.getconn()from this hot path (the origin of thePoolTimeouttraceback in Short-circuit _needs_auth_url on anonymous requests (skip DB call) #8).plone-pgcatalogitself uses in_get_pg_read_connection— storage-conn first, pool as fallback.Behaviour matrix (unchanged from before)
allowed_roles{}{"error": "Unauthorized"}zoidmissing / non-hex{"error": ...}object_state{"error": "Not found"}{"error": "Service unavailable"}Test plan
test_prefers_storage_connection_over_pool— verifiesget_storage_connectionis called,get_pool/get_request_connectionare not.test_falls_back_to_pool_when_no_storage_connection— verifies pool path still works when no storage conn is available._patch_dependencieshelper andtest_db_error_returns_503to patch the new import.🤖 Generated with Claude Code