Conversation
The shared REQUEST_TIMEOUT was (0.5, None), so a hung code generator, Mailgun or Slack endpoint could pin a worker forever waiting for a response, and the VPS liveness request had no timeout at all. Both now use bounded connect and read timeouts. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
check_image_exists memoized its whole result, so a registry timeout or an image pushed a moment later stayed rejected for the life of the worker. Only images confirmed to exist are remembered now, and failed lookups are retried. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
Migration 1.5.6 dropped the index on files.dataset_id and never restored it, so dataset.files, TransformRequest.all_files and the per-dataset delete in the lifecycle task scanned the whole table. This adds the index back and declares the already existing index on TransformRequest.submit_time on the model so it matches the schema. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
Dataset obsoletion loaded every dataset and filtered in Python with a commit per row; it now issues a single UPDATE. The data lifecycle endpoint deletes expired transforms and orphaned datasets inside one transaction instead of committing per row and then touching objects the commit had expired, eager loads the submitting user, and returns 400 for a missing or unparseable cutoff_timestamp. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
Emptying a bucket issued one remove_object call per key. It now uses minio's batched remove_objects, consumes the returned error iterator, and logs through the app logger instead of print. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
FileURLGenerator built a boto3 client on every request. The client is now built once per set of object store settings, and the presigned URL expiry is computed in UTC so it no longer depends on the server's local timezone. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
get_jwt_user was wrapped in a JWT check even though both callers, auth_required and is_admin_user, already call verify_jwt_in_request first, so every authenticated API request decoded and verified its token twice. The test fixture that had been hiding this now mocks the identity lookup directly. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
ariostas
force-pushed
the
perf/app-queries
branch
from
September 9, 2026 13:56
631c070 to
eae77b5
Compare
12 tasks
MattShirley
reviewed
Sep 21, 2026
| todelete = [ | ||
| _.id for _ in datasets if _.last_updated and (now - _.last_updated) > delta | ||
| ] | ||
| cutoff = now - delta |
MattShirley
reviewed
Sep 21, 2026
|
|
||
| expirydelta = 365 * 24 * 60 * 60 | ||
| expiry = int(datetime.datetime.now().timestamp() + expirydelta) | ||
| expiry = int(datetime.now(timezone.utc).timestamp() + expirydelta) |
This branch has not been deployed
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.
🤖 AI text below 🤖
Cuts the per-row and per-request work out of the app's hot paths and bounds its outbound HTTP calls: the two lifecycle endpoints now do their deletions in bulk inside a single transaction, the
filestable gets back the index it needs, authenticated requests decode their JWT once instead of twice, the presigning S3 client is reused, and no outbound request can hang forever.How to review. Each finding is one commit; the Commits tab shows them in severity order. Each entry below links to its commit; tick approve or reject under it. To ask for a change instead, leave a review comment on the commit. Rejected commits will be dropped from the branch and this list will be updated to match.
ec37cf6· GiveREQUEST_TIMEOUTa real read timeout and add one to the VPS liveness request2ea3d89· Remember only images confirmed to exist, so failed registry lookups are retried80ffe44· Restore the index onfiles.dataset_idand declare the existingsubmit_timeindex on the modeld872baa· Bulk update in dataset obsoletion, one transaction and eager loading in the data lifecycle task15f6c33· Empty buckets with minio's batchedremove_objectsand log through the app logger0e271c1· Build the presigning S3 client once per set of object store settings and compute the expiry in UTCeae77b5· Stop verifying the JWT a second time insideget_jwt_userFollow-ups noticed, out of scope
v1_8_4); whichever of the two merges second needs itsdown_revisionre-pointed at the other.servicex_app_test/test_transformer_manager.pypasses only as part of the full suite:test_init_invalid_configandtest_persistent_claim_existsrely on an application context that an earlier test file leaves pushed. This is pre-existing and untouched here, but it makes the file impossible to run on its own.urllib3.Timeoutdoes not implement__eq__, so the VPS liveness test comparesconnect_timeoutandread_timeoutby hand instead of usingassert_called_with. A small comparison helper would read better if more call sites start passing timeouts.transformer_sidecar/src/transformer_sidecar/transformer.pyconstructs a freshObjectStoreManager, and therefore a fresh minio client, in two places rather than reusing one.Dataset.find_by_idinmodels.pystill uses the legacycls.query.get(id), which SQLAlchemy 2.0 deprecates in favour ofdb.session.get; the other two call sites in the same file were already converted.Part of #1539.