feat(pl-drivers): cache presigned download URLs#1701
Merged
Conversation
Cache GetDownloadURL responses by signed resource id to avoid a backend round-trip on every blob read. Each entry's TTL is derived from the expiry encoded in the presigned URL (X-Amz-Date/Expires for S3 and the FS remote driver, X-Goog-Date/Expires for GCS), minus a 30s safety margin; the timestamp is UTC. URLs with no encoded expiry (local storage://) get a bounded default TTL.
🦋 Changeset detectedLatest commit: 4df307e The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a caching mechanism for presigned download URLs in @milaboratories/pl-drivers using lru-cache to avoid redundant GetDownloadURL round-trips. The cache derives TTLs from AWS SigV4 or GCS V4 URL expiries with a safety margin, falling back to a default TTL for local storage URLs. Feedback suggests throwing immediately on aborted signals even on cache hits, removing an unused logger parameter from the DownloadUrlCache constructor, and defensively verifying that parsed URL expiry values are non-negative.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1701 +/- ##
===========================================
+ Coverage 44.56% 55.06% +10.49%
===========================================
Files 43 312 +269
Lines 2540 17548 +15008
Branches 663 3830 +3167
===========================================
+ Hits 1132 9662 +8530
- Misses 1225 6647 +5422
- Partials 183 1239 +1056 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The URL cache derives its TTL from the expiry encoded in the presigned URL minus a 30s margin, but clock skew between this host and the storage backend can still leave the cache serving a URL the storage rejects. With no recovery path, every read of that blob failed until the entry's TTL lapsed. Move all cache interaction into withBlobContent (its sole consumer) and, on a 4xx from a cached URL, evict the entry and retry once with a freshly fetched URL. Expired URLs surface as 4xx across all backends: S3 403, the pl FS remote driver 403, GCS 400 ExpiredToken - so the retry keys off the whole 4xx range, not 403 alone. grpcGetDownloadUrl is now a pure fetcher.
Bump @milaboratories/pframes-rs-* to 1.1.48 (ships the caching runtime) and wire its CachingObjectStore into the PFrames remote blob provider. Downloaded parquet byte ranges are now served from a local cache that survives restarts, backed by a new parquetCachePath (default <workDir>/parquetCache, not emptied on startup). Cache tuning is exposed via parquetCacheOps (8 GiB budget, 0.2 single-file admission fraction, 50k tracked files); getCacheMetrics() and resetCache() now report real data instead of null / no-op.
…ypass) getContentDirect mirrors getContent but never reads from or writes to the ranges cache - a fresh read straight from storage that does not populate the cache. Shared via getContentImpl with an internal bypassRangesCache flag that skips the cache get and the tee/set in withContent; for local handles it is identical to getContent. Kept off the block-facing BlobDriver interface. The PFrames data-source blob reader (pl-middle-layer pool driver) now uses getContentDirect, since PFrames manages its own caching.
vadimpiven
force-pushed
the
feat/cache-download-urls
branch
from
8335198 to
ae79571
Compare
extractUrlExpiryMs and downloadUrlCacheTtlMs were unexported, breaking download_url_cache.test.ts. Re-export them and align the malformed-params test with the current contract (present-but-invalid presign params return null -> not cached, distinct from undefined = no expiry encoded). Also: log parsed cache metrics via JSON.stringify, and log cached download URL acceptance.
Remove the per-cache-hit "cached download URL ... accepted" log and the "Parquet cache metrics" log (with its now-unused getCacheMetrics call).
kevindetry-milaboratories
approved these changes
Jun 17, 2026
downloadUrlCacheTtlMs now reads Date.now() internally instead of taking nowMs, so the tests pin the system clock with fake timers instead of passing a timestamp. Also fix the stale nowMs reference in its doc comment.
The parquet caching object store (newly wired in this branch) adds latency to
every parquet read, which the model-test exercises via MiddleLayer.init. The
test was under-provisioned versus its siblings (table-test/filter-column-test):
'with args' used a 10s timeout and 'block-level services' relied on the 5s
awaitBlockDone default, so on slow CI the block did not reach Done in time
('Test timed out in 10000ms' / 'Aborted while awaiting block done').
Align with table-test: 60s test timeouts and an explicit 50s awaitBlockDone.
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.
What
Caches
GetDownloadURLresponses inClientDownloadso repeated reads of the same blob don't each pay a backend round-trip.DownloadUrlCache(LRU, keyed bySignedResourceId) holding the full{downloadUrl, headers}response.grpcGetDownloadUrlnow checks the cache before the gRPC/REST call and populates it on success.X-Amz-Date+X-Amz-Expires— S3 and the FS remote driver (AWS SigV4).X-Goog-Date+X-Goog-Expires— GCS (V4 signed URLs).Z/Zulu), verified against plutil/storage/v4sign/presigner.go. The compactYYYYMMDDTHHMMSSZform is expanded to the extended ISO form before parsing (V8 cannot parse the compact form).storage://URLs carry no encoded expiry and get a bounded 1h default TTL.Why
GetDownloadURLis called on every blob read; the returned URL is valid for its full TTL (1h for FS-remote/GCS by default), so re-requesting it per read is wasted work.Notes
DownloadUrlCache.set()(not before the await), so the cache entry never outlivesexpiry − margin.isInternalUsebecause the only caller always passesfalse.Tests
download_url_cache.test.ts— 10 unit tests covering S3/FS, GCS, the UTC-independence guarantee, malformed/non-URL input, the 30s margin, and the no-expiry default.Greptile Summary
This PR adds a
DownloadUrlCache(LRU, keyed bySignedResourceId) toClientDownloadso that eachGetDownloadURLbackend call is only made once per presigned URL's lifetime instead of once per blob read. Cache entry TTLs are computed from the expiry embedded in the presigned URL (X-Amz-Date/X-Amz-Expiresfor S3 and the FS remote driver,X-Goog-Date/X-Goog-Expiresfor GCS), with a 30 s safety margin;storage://URLs fall back to a 1 h default.download_url_cache.ts— New module withextractUrlExpiryMs,downloadUrlCacheTtlMs, andDownloadUrlCache; the injectedloggeris currently unused, and a malformed presign date (however unlikely) would cause a fall-through to the 1 h default TTL instead of bypassing the cache.download.ts—grpcGetDownloadUrlnow checks the cache before the gRPC/REST call and populates it on success; diff is minimal and correct.download_url_cache.test.ts— Tests coverextractUrlExpiryMsanddownloadUrlCacheTtlMsthoroughly, but theDownloadUrlCacheclass itself (round-trip get/set, TTL enforcement, thettl ≤ 0skip path) has no direct test coverage.Confidence Score: 4/5
Safe to merge; the caching logic is correct for the happy path and the changes are well-scoped to a single client class.
The core TTL derivation and LRU integration are sound. The three findings are all quality/observability concerns: an unused logger, a misleading comment, and a theoretical over-caching scenario on malformed presign dates that the backend never produces in practice. Nothing in the changed path can cause data loss or incorrect auth.
download_url_cache.ts deserves a second look for the unused logger and the malformed-date fallback path; the test file is missing coverage of the DownloadUrlCache class itself.
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(pl-drivers): cache presigned downlo..." | Re-trigger Greptile