Optimize S3 fetcher with conditional GETs and new download_if_changed callback #20
Conversation
… hook - add optional download_if_changed/2 to Fetcher behaviour and wire update loop to use it when available - implement conditional GETs in the S3 fetcher via If-None-Match and 304 handling - switch S3 version checks to ETag-only and document versioned/non‑versioned behavior - add S3 tests covering conditional GET and header usage
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| message: "Failed to fetch from primary bucket, attempting failover buckets" | ||
| }) | ||
|
|
||
| try_failover_buckets(op, failover_buckets, opts, state) |
There was a problem hiding this comment.
Failover buckets ignore 304 handling during conditional GETs
Medium Severity
The failover_on_error? predicate is used to prevent treating 304 responses as failures for the primary bucket, but it's not passed to try_failover_buckets. When download_if_changed triggers failover (e.g., primary returns 500) and a failover bucket returns 304 (not modified), the response is incorrectly treated as an error. This causes unnecessary requests to all remaining failover buckets and can result in "All buckets failed" errors with misleading log messages, even when the content is actually unchanged across all failover buckets.
Additional Locations (1)
There was a problem hiding this comment.
planning to work on the failover logic in a separate PR
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="lib/remote_persistent_term/fetcher/s3.ex">
<violation number="1" location="lib/remote_persistent_term/fetcher/s3.ex:212">
P1: This case-sensitive comparison will fail to find HTTP headers returned by AWS S3. S3 returns headers like `"ETag"` (mixed case), but this code searches for `"etag"` (lowercase). HTTP headers are case-insensitive per RFC 7230. The previous implementation correctly used `String.downcase()` for comparison. This will cause `extract_version/1` to always return `{:error, :not_found}` in production.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| defp header_value(headers, name) do | ||
| Enum.find_value(headers, fn | ||
| {key, value} when is_binary(key) and is_binary(value) -> | ||
| if key == name, do: value, else: nil |
There was a problem hiding this comment.
P1: This case-sensitive comparison will fail to find HTTP headers returned by AWS S3. S3 returns headers like "ETag" (mixed case), but this code searches for "etag" (lowercase). HTTP headers are case-insensitive per RFC 7230. The previous implementation correctly used String.downcase() for comparison. This will cause extract_version/1 to always return {:error, :not_found} in production.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/remote_persistent_term/fetcher/s3.ex, line 212:
<comment>This case-sensitive comparison will fail to find HTTP headers returned by AWS S3. S3 returns headers like `"ETag"` (mixed case), but this code searches for `"etag"` (lowercase). HTTP headers are case-insensitive per RFC 7230. The previous implementation correctly used `String.downcase()` for comparison. This will cause `extract_version/1` to always return `{:error, :not_found}` in production.</comment>
<file context>
@@ -207,14 +207,9 @@ defmodule RemotePersistentTerm.Fetcher.S3 do
-
- {key, value} when is_atom(key) and is_binary(value) ->
- if String.downcase(Atom.to_string(key)) == downcased, do: value, else: nil
+ if key == name, do: value, else: nil
_ ->
</file context>
| if key == name, do: value, else: nil | |
| if String.downcase(key) == String.downcase(name), do: value, else: nil |
✅ Addressed in 3df3517
…ttp client that doesnt downcase headers
| value | ||
| |> String.trim() | ||
| |> String.trim("\"") | ||
| end |
There was a problem hiding this comment.
Weak ETags are incorrectly normalized causing match failures
Low Severity
The normalize_etag function uses String.trim("\"") which only removes quotes from both ends if present. For weak ETags like W/"abc123", this removes only the trailing quote (since the first char is W), producing the malformed value W/"abc123 (missing closing quote). Subsequently, quote_etag wraps this as "W/\"abc123" instead of the correct W/"abc123", causing conditional GET requests to never match for S3-compatible services using weak validators.
Note
Introduces an optional fast-path update flow and optimizes the S3 fetcher to reduce redundant downloads.
download_if_changed/2toRemotePersistentTerm.Fetcher; updateupdate_term/3to prefer it when implemented; extractstore_term/4; unify error logging vialog_update_error/2.HEADto readETagforcurrent_version/1; implement conditional GET viaIf-None-Matchand handle304 Not Modified; normalize/quote ETags; adjust failover to not fail over on 304; add Req-based ExAws HTTP client (RemotePersistentTerm.Fetcher.S3.HttpClient).0.13.0; switch to Req for ExAws HTTP; update lockfile; addAGENTS.mdcontributor guidelines.Written by Cursor Bugbot for commit 3df3517. This will update automatically on new commits. Configure here.
Summary by cubic
Adds conditional GETs to the S3 fetcher and an optional download_if_changed callback to avoid re-downloading unchanged terms and cut S3/API usage. S3 version checks now use HEAD/ETag for faster, simpler change detection.
New Features
Dependencies
Written for commit 3df3517. Summary will update on new commits.