Skip to content

Commit c49c19e

Browse files
RamblurrRamblurr
committed
Improve Retry-After handling
1 parent ff17c46 commit c49c19e

File tree

4 files changed

+155
-28
lines changed

4 files changed

+155
-28
lines changed

‎ROADMAP.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919
## Milestones
2020

2121
### Milestone 1 – Session & Account Hardening
22-
- [x] Flesh out `impl.http/parse-http-time` for RFC 7231 Retry-After handling and surface durations through callers.
22+
- [x] Flesh out `impl.http/parse-http-time` for RFC 7231 Retry-After handling and surface durations through callers (updated 2025-10-29 to honour response `Date` for delta-seconds and expand regression tests).
23+
- [ ] Improve retry-after parsing based
2324
- [ ] Implement account `POST-as-GET`, contact updates, deactivation, and External Account Binding (RFC 8555 §7.3.4) with Pebble coverage.
2425
- [ ] Support account key rollover via directory `keyChange`, including verification that the new keypair is active.
2526

@@ -28,6 +29,7 @@
2829
- [ ] Implement order retrieval (`POST-as-GET`), polling helpers respecting Retry-After, and tracking of authorization URLs.
2930
- [ ] Complete finalize-order flow that submits CSR bytes from `impl.csr/create-csr`, checks order state transitions, and captures certificate URLs.
3031
- [ ] Provide certificate download helper that dereferences the `certificate` link and returns PEM chain + parsed certificates.
32+
- [ ] Add Pebble integration/E2E coverage for Retry-After once POST-as-GET helpers land.
3133

3234
### Milestone 3 – Authorizations & Challenges
3335
- [ ] Implement authorization retrieval and caching, ensuring challenge objects reflect Pebble behaviour.

‎specs/004-retry-after.md‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
# Feature Spec 004: Retry-After Time Parsing
2+
3+
**Status:** Completed
4+
**Created:** 2025-10-29
5+
**Author:** Codex
6+
**Last Updated:** 2025-10-29
7+
8+
## Overview
9+
10+
`ol.clave.impl.http/retry-after` currently trims the `Retry-After` header down to either a delta-seconds string or punts to the stubbed `parse-http-time`. RFC 7231 (§7.1.1.1) permits three HTTP-date wire formats (IMF-fixdate, obsolete RFC 850, and ANSI C's asctime). We need a tolerant parser that accepts all three, normalises them to an `Instant`, and keeps higher-level retry helpers deterministic and testable.
11+
12+
Meeting this milestone ensures we sleep the correct amount when Pebble (or real CAs) respond with Retry-After hints and gives callers the surfaced `Duration` they need for backoff policies.
13+
14+
## Competitive Analysis: acme4j
15+
16+
`extra/acme4j/acme4j-client/src/main/java/org/shredzone/acme4j/connector/DefaultConnection.java` parses Retry-After by:
17+
18+
- Treating digit-only headers as delta-seconds applied to the HTTP `Date` header when present, falling back to `Instant.now()` if absent.
19+
- Parsing absolute HTTP-date values strictly with `DateTimeFormatter.RFC_1123_DATE_TIME` (IMF-fixdate).
20+
- Raising a protocol error on malformed headers.
21+
22+
`DefaultConnectionTest` exercises both header flavours and ensures delta-seconds honour the response `Date`. We'll mirror the Date-header preference but keep our broader HTTP-date support and nil-on-invalid behaviour so callers can fall back gracefully.
23+
24+
## Implementation Comparison
25+
26+
| Aspect | acme4j | ol.clave |
27+
|-------------------------|-----------------------------------------------------------|---------------------------------------------------------------------------------------------------------|
28+
| Delta-seconds baseline | Prefers HTTP `Date` header, falls back to `Instant.now()` | Same behaviour, exposed via `retry-after-header->instant` using `parse-http-time` for the `Date` header |
29+
| Absolute date parsing | Strict RFC 1123 (`DateTimeFormatter.RFC_1123_DATE_TIME`) | Accepts IMF-fixdate, RFC 850, and ANSI C asctime via formatter suite |
30+
| Invalid header handling | Throws `AcmeProtocolException` | Returns `nil` so callers can fall back to supplied duration |
31+
| Test coverage | Delta vs date, absolute date, absence | Mirrors delta + date coupling, plus additional variants for all HTTP-date forms and fallback paths |
32+
| Time source | `Instant.now()` inline | Private `now` helper to enable deterministic testing |
33+
34+
## Goals
35+
36+
1. Parse all RFC 7231 HTTP-date variants into `java.time.Instant` without relying on deprecated `java.util.Date`.
37+
2. Prefer the server `Date` header as the baseline instant for delta-seconds retries, falling back to `Instant/now` if missing or invalid.
38+
3. Keep `retry-after-time` and `retry-after` deterministic for testing by routing `Instant/now` through an overridable helper.
39+
4. Provide regression tests covering delta-seconds (with and without `Date`), each HTTP-date flavour, future/past handling, and invalid header fallbacks.
40+
41+
## Non-Goals
42+
43+
- Implement broader HTTP header parsing or general date utilities.
44+
- Change public API shapes outside `ol.clave.impl.http` plumbing namespace.
45+
- Add resilience features (e.g. jitter) beyond reading Retry-After correctly.
46+
47+
## Implementation Plan
48+
49+
1. **Formatter suite**
50+
- Build a private vector of `DateTimeFormatter` instances created via `DateTimeFormatterBuilder` to cover:
51+
- `EEE, dd MMM yyyy HH:mm:ss 'GMT'` (IMF-fixdate / RFC 1123).
52+
- `EEEE, dd-MMM-yy HH:mm:ss 'GMT'` (obsolete RFC 850; ensure two-digit year mapping per RFC rules).
53+
- `EEE MMM d HH:mm:ss yyyy` (ANSI C's asctime; map implicit GMT).
54+
- Wrap them with `.withZone ZoneOffset/UTC` to ensure parsed instants are UTC.
55+
- Iterate formatters until one succeeds; return first successful `Instant`, else `nil`.
56+
57+
2. **Parsing helper**
58+
- Implement `parse-http-time` to:
59+
- Guard against blank inputs.
60+
- Try each formatter inside `try`/`catch`, ignoring `DateTimeParseException` until success.
61+
- Reject two-digit years earlier than 1970 by rolling per RFC (>=1970 vs +100 years) or simply using formatter with resolver style `STRICT`.
62+
- Return `nil` for garbage input.
63+
64+
3. **Now shim**
65+
- Add private `(defn- now [] (Instant/now))` and replace direct `Instant/now` calls in `retry-after-time` and `retry-after`.
66+
- Tests can `with-redefs` `now` to supply deterministic instants.
67+
68+
4. **Retry helper adjustments**
69+
- Refine the private `retry-after-header->instant` helper so it accepts the full response map (not just the raw header) and can read both `Retry-After` and `Date`.
70+
- Keep public `retry-after-time` focused on response maps and delegate to the helper.
71+
- When header is delta-seconds, parse the server `Date` header via `parse-http-time`, falling back to `(now)` if parsing fails.
72+
- When header contains HTTP-date, delegate to `parse-http-time`.
73+
- Keep nil-on-exception behaviour.
74+
- Leave `retry-after` signature intact but depend on `now` shim for comparisons.
75+
76+
5. **Testing**
77+
- New unit tests in `test/ol/clave/impl/http_test.clj`:
78+
- `parse-http-time` accepts each HTTP-date flavour (with sample strings from RFC 7231).
79+
- Returns `nil` for invalid strings / nil input.
80+
- `retry-after-time` handles delta seconds using the `Date` header baseline, delta seconds without `Date` (falls back to mocked `(now)`), and HTTP-date values.
81+
- `retry-after` computes zero-duration when Retry-After <= now, otherwise the correct positive duration.
82+
- Tests should be verbose and validate full map equality when asserting results.
83+
84+
6. **Documentation & Roadmap**
85+
- Update ROADMAP milestone item to checked.
86+
- Note implementation details in spec (this file) if adjustments are required during development.
87+
88+
## Test Matrix
89+
90+
| Scenario | Header Example | Expected Result |
91+
|---------------------|-----------------------------------------------------------|---------------------------------------------------------------------------------------|
92+
| Delta seconds | `Retry-After: 30` | `retry-after` returns `Duration/ofSeconds 30` when `now` mocked to the same baseline. |
93+
| IMF-fixdate | `Retry-After: Wed, 21 Oct 2015 07:28:00 GMT` | Parsed instant equals RFC sample. |
94+
| RFC 850 | `Retry-After: Wednesday, 21-Oct-15 07:28:00 GMT` | Parsed instant equals IMF-fixdate equivalent. |
95+
| asctime | `Retry-After: Wed Oct 21 07:28:00 2015` | Parsed instant equals IMF-fixdate equivalent. |
96+
| Past date | Header one minute in past | `retry-after` returns `Duration/ZERO`. |
97+
| Invalid | `Retry-After: nonsense` | `retry-after-time` returns `nil`, `retry-after` falls back to provided duration. |
98+
| Delta + Date header | `Retry-After: 120`, `Date: Wed, 21 Oct 2015 07:26:00 GMT` | Instant equals `07:28:00Z`. |
99+
100+
## Outcomes
101+
102+
- Correct Retry-After handling across all compliant server variants.
103+
- Deterministic tests for time-sensitive logic.
104+
- Clear path for future enhancements (e.g. jitter) built atop accurate duration calculations.

‎src/ol/clave/impl/http.clj‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -169,23 +169,24 @@
169169
(defn- now []
170170
(Instant/now))
171171

172-
(defn- retry-after-header->instant
173-
^Instant [raw]
174-
(let [s (some-> raw str str/trim)]
175-
(when (seq s)
176-
(try
177-
(if (re-matches #"\d+" s)
178-
(.plusSeconds ^Instant (now) (Long/parseLong s))
179-
(parse-http-time s))
180-
(catch Exception _e
181-
;; invalid header => nil, caller uses fallback
182-
nil)))))
172+
(defn retry-after-header->instant
173+
^Instant [resp]
174+
(when-let [raw (some-> (get-header resp "retry-after") str str/trim)]
175+
(try
176+
(if (re-matches #"\d+" raw)
177+
(let [delta (Long/parseLong raw)
178+
base (or (some-> (get-header resp "date") parse-http-time)
179+
(now))
180+
^Instant base base]
181+
(.plusSeconds base delta))
182+
(parse-http-time raw))
183+
(catch Exception _
184+
nil))))
183185

184186
(defn retry-after-time
185187
"Return java.time.Instant derived from a Retry-After header; nil if absent/invalid."
186188
^Instant [resp]
187-
(when-let [raw (get-header resp "retry-after")]
188-
(retry-after-header->instant raw)))
189+
(retry-after-header->instant resp))
189190

190191
(defn retry-after
191192
"Return a java.time.Duration until retry time; or fallback if header missing/invalid."
@@ -286,7 +287,7 @@
286287
"Perform a single request. Drain body into bytes. Decide if safe to retry.
287288
Returns {:resp :headers :status :body-bytes :nonce :retry? :err}.
288289
Cancellation: if cancel-token (a CompletableFuture) completes first, the request future is cancelled."
289-
[session {:keys [headers] :as req} {:keys [cancel-token]}]
290+
[session req {:keys [cancel-token]}]
290291
(let [req' (cond-> req
291292
true (assoc :async true) ;; run async so we can cancel mid-flight
292293
(not (get-in req [:headers :user-agent])) (update :headers assoc :user-agent default-user-agent))

‎test/ol/clave/impl/http_test.clj‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,27 +53,47 @@
5353
(is (nil? (http/parse-http-time " "))))))
5454

5555
(deftest retry-after-time-parsing
56-
(let [baseline (Instant/parse "2025-10-29T12:00:00Z")]
56+
(let [baseline (Instant/parse "2025-10-29T12:00:00Z")
57+
date-header "Wed, 29 Oct 2025 11:59:00 GMT"]
5758
(with-redefs [ol.clave.impl.http/now (fn [] baseline)]
58-
(testing "Delta-seconds header"
59-
(is (= (Instant/parse "2025-10-29T12:00:30Z")
60-
(@#'ol.clave.impl.http/retry-after-header->instant "30"))))
59+
(testing "Delta-seconds header uses response Date baseline"
60+
(let [resp {:headers {"retry-after" "30"
61+
"date" date-header}}
62+
expected (Instant/parse "2025-10-29T11:59:30Z")]
63+
(is (= expected (http/retry-after-header->instant resp)))
64+
(is (= expected (http/retry-after-time resp)))))
65+
(testing "Delta-seconds without Date falls back to now"
66+
(let [resp {:headers {"retry-after" "45"}}
67+
expected (Instant/parse "2025-10-29T12:00:45Z")]
68+
(is (= expected (http/retry-after-header->instant resp)))
69+
(is (= expected (http/retry-after-time resp)))))
6170
(testing "HTTP-date header"
62-
(is (= (Instant/parse "2015-10-21T07:28:00Z")
63-
(@#'ol.clave.impl.http/retry-after-header->instant "Wed, 21 Oct 2015 07:28:00 GMT"))))
64-
(testing "Response map header delegates correctly"
65-
(is (= (Instant/parse "2025-10-29T12:00:45Z")
66-
(http/retry-after-time {:headers {"retry-after" "45"}}))))
71+
(let [resp {:headers {"retry-after" "Wed, 21 Oct 2015 07:28:00 GMT"}}
72+
expected (Instant/parse "2015-10-21T07:28:00Z")]
73+
(is (= expected (http/retry-after-header->instant resp)))
74+
(is (= expected (http/retry-after-time resp)))))
6775
(testing "Invalid header produces nil"
68-
(is (nil? (@#'ol.clave.impl.http/retry-after-header->instant "later maybe"))))
69-
(testing "Nil header produces nil"
70-
(is (nil? (@#'ol.clave.impl.http/retry-after-header->instant nil)))))))
76+
(let [resp {:headers {"retry-after" "later maybe"}}]
77+
(is (nil? (http/retry-after-header->instant resp)))
78+
(is (nil? (http/retry-after-time resp)))))
79+
(testing "Missing header produces nil"
80+
(let [resp {:headers {}}]
81+
(is (nil? (http/retry-after-header->instant resp)))
82+
(is (nil? (http/retry-after-time resp))))))))
7183

7284
(deftest retry-after-duration-calculation
7385
(let [future-now (Instant/parse "2015-10-21T07:27:00Z")
7486
past-now (Instant/parse "2015-10-21T07:29:00Z")
7587
header "Wed, 21 Oct 2015 07:28:00 GMT"
76-
fallback (Duration/ofSeconds 5)]
88+
fallback (Duration/ofSeconds 5)
89+
delta-now (Instant/parse "2025-10-29T12:00:00Z")
90+
delta-header "Wed, 29 Oct 2025 12:00:00 GMT"]
91+
(testing "Delta seconds uses Date baseline to build target instant"
92+
(with-redefs [ol.clave.impl.http/now (fn [] delta-now)]
93+
(is (= (Duration/ofSeconds 30)
94+
(http/retry-after {:headers {"retry-after" "30"
95+
"date" delta-header}}
96+
fallback)))))
7797
(testing "Future retry-after yields positive duration"
7898
(with-redefs [ol.clave.impl.http/now (fn [] future-now)]
7999
(is (= (Duration/ofSeconds 60)

0 commit comments

Comments
 (0)