nandi/glean · 3e65fa0

⑂ 3e65fa0

Port the feed fetcher, fixing its retry policy

Retries exist for servers that are briefly unwell -- 429 and 5xx -- and for
nothing else. A 404 will still be a 404 in a second, and bytes that do not
parse will not parse differently on a second read.

That diverges from the Go implementation, because the Go retry policy does
not work as written. executeRequest returns its *http.Response only on
success, so Fetch's guard

    if resp != nil && !httpclient.IsRetryable(resp.StatusCode)

can never be true when err != nil. Every failure therefore retries the full
four attempts whatever the status, so a permanently-404 feed costs four
requests and about seven seconds of backoff per refresh. retryBackoff's
Retry-After handling is unreachable for the same reason -- lastResp is only
non-nil once the function has returned -- so a rate-limited server's own
delay is never honoured. And the status is never checked, so an error page
reaches the parser and fails there instead.

This port checks the status, honours Retry-After capped at ten seconds, and
retries only what is worth retrying. Written down in the module header as
well as here, since it is a behaviour change rather than a translation.

The probe scripts the transport: 503 twice then success, a 404 that must not
retry, a 429 naming its own delay. None of those can be asked of a real host
on demand.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

nandithebull committed 2026-09-21T18:06:44-07:00 Browse files

3e65fa0 parent: d1cf582

modified spike/nim/.gitignore +1 -0 Viewed

@@ -15,3 +15,4 @@ stores_probe
15	15	social_probe
16	16	nimcache/
17	17	feedparser_probe
	18	+feedfetcher_probe

  social_probe
  nimcache/
  feedparser_probe
++feedfetcher_probe

added spike/nim/atproto/feedfetcher.nim +180 -0 Viewed

new file mode 100644
@@ -0,0 +1,180 @@
	1	+## Fetching feeds over HTTP, ported from internal/feed/fetcher.go.
	2	+##
	3	+## Retries exist for servers that are briefly unwell -- 429 and 5xx -- and
	4	+## for nothing else. A 404 will still be a 404 in one second, and a feed that
	5	+## does not parse will not parse differently on a second read, so both fail
	6	+## immediately.
	7	+##
	8	+## ## A divergence from the Go implementation, deliberately
	9	+##
	10	+## The Go fetcher's retry policy does not work as written.
	11	+## `executeRequest` returns its `*http.Response` only on success, so in
	12	+## `Fetch` the guard
	13	+##
	14	+## if resp != nil && !httpclient.IsRetryable(resp.StatusCode)
	15	+##
	16	+## can never be true when `err != nil`: the response is always nil on the
	17	+## error path. Three things follow. Every failure retries the full four
	18	+## attempts regardless of status, so a permanently-404 feed costs four
	19	+## requests per refresh. `retryBackoff`'s Retry-After handling is likewise
	20	+## unreachable, because `lastResp` is only non-nil once the function has
	21	+## already returned. And the HTTP status is never checked at all, so an error
	22	+## page is handed to the parser and fails there instead.
	23	+##
	24	+## This port checks the status, honours Retry-After, and retries only what is
	25	+## worth retrying. It is a behaviour change, not a translation, which is why
	26	+## it is written down here rather than left to be discovered.
	27	+
	28	+import std/[httpclient, os, strutils, times]
	29	+import ./feedparser
	30	+
	31	+type
	32	+ FetchError* = object of CatchableError
	33	+ status*: int ## 0 when the request never got a response
	34	+ retryable*: bool
	35	+
	36	+ FetchConfig* = object
	37	+ timeoutMs*: int
	38	+ maxRetries*: int
	39	+ baseDelayMs*: int
	40	+ maxRetryAfterMs*: int
	41	+ userAgent*: string
	42	+
	43	+ ## Injectable so the probe can drive the retry logic without a network, and
	44	+ ## so at:// feeds can be served by the ATProto client instead.
	45	+ Transport* = proc(url: string): tuple[status: int, body: string,
	46	+ retryAfter: string] {.gcsafe.}
	47	+
	48	+const
	49	+ AcceptFeed* = "application/xml,application/atom+xml,application/rss+xml," &
	50	+ "application/rdf+xml,application/feed+json,text/html;q=0.9"
	51	+ DefaultUserAgent* = "glean-nim/0.1"
	52	+
	53	+proc defaultConfig*(): FetchConfig =
	54	+ FetchConfig(
	55	+ timeoutMs: 10_000,
	56	+ maxRetries: 3,
	57	+ baseDelayMs: 1000,
	58	+ maxRetryAfterMs: 10_000,
	59	+ userAgent: DefaultUserAgent,
	60	+ )
	61	+
	62	+proc isRetryable*(status: int): bool =
	63	+ ## 429 and 5xx only. Everything else is the server's settled opinion.
	64	+ status == 429 or status >= 500
	65	+
	66	+proc parseRetryAfter*(value: string): int =
	67	+ ## Milliseconds, or 0 when absent or unparseable.
	68	+ ##
	69	+ ## Retry-After is either delta-seconds or an HTTP date; both are in the
	70	+ ## wild, and a server that sends one is telling us something more useful
	71	+ ## than our own backoff curve.
	72	+ let v = value.strip()
	73	+ if v.len == 0: return 0
	74	+ try:
	75	+ return parseInt(v) * 1000
	76	+ except ValueError:
	77	+ discard
	78	+ for fmt in ["ddd, dd MMM yyyy HH:mm:ss 'GMT'",
	79	+ "ddd, dd MMM yyyy HH:mm:ss zzz"]:
	80	+ try:
	81	+ let target = parse(v, fmt, utc())
	82	+ let delta = (target - now().utc).inMilliseconds.int
	83	+ return max(delta, 0)
	84	+ except TimeParseError, TimeFormatParseError:
	85	+ continue
	86	+ 0
	87	+
	88	+proc backoffMs*(cfg: FetchConfig, attempt: int, retryAfter: string,
	89	+ status: int): int =
	90	+ ## Exponential, unless the server named a delay on a 429.
	91	+ if status == 429:
	92	+ let named = parseRetryAfter(retryAfter)
	93	+ if named > 0:
	94	+ # Capped: a server asking for an hour should not hold a worker for one.
	95	+ return min(named, cfg.maxRetryAfterMs)
	96	+ cfg.baseDelayMs * (1 shl max(attempt - 1, 0))
	97	+
	98	+proc httpTransport*(cfg: FetchConfig): Transport =
	99	+ result = proc(url: string): tuple[status: int, body: string,
	100	+ retryAfter: string] {.gcsafe.} =
	101	+ let client = newHttpClient(userAgent = cfg.userAgent,
	102	+ timeout = cfg.timeoutMs,
	103	+ maxRedirects = 5)
	104	+ defer: client.close()
	105	+ client.headers = newHttpHeaders({"Accept": AcceptFeed})
	106	+ let res = client.get(url)
	107	+ (res.code.int, res.body, res.headers.getOrDefault("Retry-After").string)
	108	+
	109	+proc raiseFetch(status: int, msg: string) =
	110	+ var e = newException(FetchError, msg)
	111	+ e.status = status
	112	+ e.retryable = isRetryable(status)
	113	+ raise e
	114	+
	115	+proc fetchFeed*(url: string, cfg = defaultConfig(),
	116	+ transport: Transport = nil,
	117	+ sleep: proc(ms: int) {.gcsafe.} = nil): ParseResult =
	118	+ ## Fetch and parse, retrying only what is worth retrying.
	119	+ let tr = if transport != nil: transport else: httpTransport(cfg)
	120	+ proc defaultSleep(ms: int) {.gcsafe.} = os.sleep(ms)
	121	+ let nap = if sleep != nil: sleep else: defaultSleep
	122	+
	123	+ var lastMsg = "feed fetch failed"
	124	+ var lastStatus = 0
	125	+
	126	+ for attempt in 0 .. cfg.maxRetries:
	127	+ var status = 0
	128	+ var body = ""
	129	+ var retryAfter = ""
	130	+ var transportFailed = false
	131	+
	132	+ try:
	133	+ (status, body, retryAfter) = tr(url)
	134	+ except CatchableError as e:
	135	+ # A connection that never completed: worth retrying, since it says
	136	+ # nothing about whether the feed exists.
	137	+ transportFailed = true
	138	+ lastMsg = "fetching " & url & ": " & e.msg
	139	+ lastStatus = 0
	140	+
	141	+ if not transportFailed:
	142	+ if status >= 200 and status < 300:
	143	+ try:
	144	+ return parseFeed(body, url)
	145	+ except FeedParseError as e:
	146	+ # Not retryable: the bytes will be the same next time.
	147	+ raiseFetch(status, "parsing " & url & ": " & e.msg)
	148	+ lastStatus = status
	149	+ lastMsg = "fetching " & url & ": HTTP " & $status
	150	+ if not isRetryable(status):
	151	+ raiseFetch(status, lastMsg)
	152	+
	153	+ if attempt == cfg.maxRetries:
	154	+ break
	155	+ nap(backoffMs(cfg, attempt + 1, retryAfter, lastStatus))
	156	+
	157	+ raiseFetch(lastStatus, lastMsg)
	158	+
	159	+proc isAtProtoFeed*(url: string): bool = url.startsWith("at://")
	160	+
	161	+proc fetchOrDiscover*(url: string, cfg = defaultConfig(),
	162	+ transport: Transport = nil,
	163	+ candidates: seq[string] = @[]): (ParseResult, string) =
	164	+ ## Fetch `url`; failing that, try feeds discovered at it.
	165	+ ##
	166	+ ## Returns which URL actually worked, because the caller stores that rather
	167	+ ## than what the user typed -- people paste a site's home page far more
	168	+ ## often than its feed.
	169	+ try:
	170	+ return (fetchFeed(url, cfg, transport), url)
	171	+ except FetchError:
	172	+ if isAtProtoFeed(url) or candidates.len == 0:
	173	+ raise
	174	+ for candidate in candidates:
	175	+ try:
	176	+ return (fetchFeed(candidate, cfg, transport), candidate)
	177	+ except FetchError:
	178	+ continue
	179	+ raiseFetch(0, "no working feed found at " & url)
	180	+ (ParseResult(), url) # unreachable; satisfies the return type

++## Fetching feeds over HTTP, ported from internal/feed/fetcher.go.
++##
++## Retries exist for servers that are briefly unwell -- 429 and 5xx -- and
++## for nothing else. A 404 will still be a 404 in one second, and a feed that
++## does not parse will not parse differently on a second read, so both fail
++## immediately.
++##
++## ## A divergence from the Go implementation, deliberately
++##
++## The Go fetcher's retry policy does not work as written.
++## `executeRequest` returns its `*http.Response` only on success, so in
++## `Fetch` the guard
++##
++##     if resp != nil && !httpclient.IsRetryable(resp.StatusCode)
++##
++## can never be true when `err != nil`: the response is always nil on the
++## error path. Three things follow. Every failure retries the full four
++## attempts regardless of status, so a permanently-404 feed costs four
++## requests per refresh. `retryBackoff`'s Retry-After handling is likewise
++## unreachable, because `lastResp` is only non-nil once the function has
++## already returned. And the HTTP status is never checked at all, so an error
++## page is handed to the parser and fails there instead.
++##
++## This port checks the status, honours Retry-After, and retries only what is
++## worth retrying. It is a behaviour change, not a translation, which is why
++## it is written down here rather than left to be discovered.
++
++import std/[httpclient, os, strutils, times]
++import ./feedparser
++
++type
++  FetchError* = object of CatchableError
++    status*: int          ## 0 when the request never got a response
++    retryable*: bool
++
++  FetchConfig* = object
++    timeoutMs*: int
++    maxRetries*: int
++    baseDelayMs*: int
++    maxRetryAfterMs*: int
++    userAgent*: string
++
++  ## Injectable so the probe can drive the retry logic without a network, and
++  ## so at:// feeds can be served by the ATProto client instead.
++  Transport* = proc(url: string): tuple[status: int, body: string,
++                                        retryAfter: string] {.gcsafe.}
++
++const
++  AcceptFeed* = "application/xml,application/atom+xml,application/rss+xml," &
++                "application/rdf+xml,application/feed+json,text/html;q=0.9"
++  DefaultUserAgent* = "glean-nim/0.1"
++
++proc defaultConfig*(): FetchConfig =
++  FetchConfig(
++    timeoutMs: 10_000,
++    maxRetries: 3,
++    baseDelayMs: 1000,
++    maxRetryAfterMs: 10_000,
++    userAgent: DefaultUserAgent,
++  )
++
++proc isRetryable*(status: int): bool =
++  ## 429 and 5xx only. Everything else is the server's settled opinion.
++  status == 429 or status >= 500
++
++proc parseRetryAfter*(value: string): int =
++  ## Milliseconds, or 0 when absent or unparseable.
++  ##
++  ## Retry-After is either delta-seconds or an HTTP date; both are in the
++  ## wild, and a server that sends one is telling us something more useful
++  ## than our own backoff curve.
++  let v = value.strip()
++  if v.len == 0: return 0
++  try:
++    return parseInt(v) * 1000
++  except ValueError:
++    discard
++  for fmt in ["ddd, dd MMM yyyy HH:mm:ss 'GMT'",
++              "ddd, dd MMM yyyy HH:mm:ss zzz"]:
++    try:
++      let target = parse(v, fmt, utc())
++      let delta = (target - now().utc).inMilliseconds.int
++      return max(delta, 0)
++    except TimeParseError, TimeFormatParseError:
++      continue
++  0
++
++proc backoffMs*(cfg: FetchConfig, attempt: int, retryAfter: string,
++                status: int): int =
++  ## Exponential, unless the server named a delay on a 429.
++  if status == 429:
++    let named = parseRetryAfter(retryAfter)
++    if named > 0:
++      # Capped: a server asking for an hour should not hold a worker for one.
++      return min(named, cfg.maxRetryAfterMs)
++  cfg.baseDelayMs * (1 shl max(attempt - 1, 0))
++
++proc httpTransport*(cfg: FetchConfig): Transport =
++  result = proc(url: string): tuple[status: int, body: string,
++                                    retryAfter: string] {.gcsafe.} =
++    let client = newHttpClient(userAgent = cfg.userAgent,
++                               timeout = cfg.timeoutMs,
++                               maxRedirects = 5)
++    defer: client.close()
++    client.headers = newHttpHeaders({"Accept": AcceptFeed})
++    let res = client.get(url)
++    (res.code.int, res.body, res.headers.getOrDefault("Retry-After").string)
++
++proc raiseFetch(status: int, msg: string) =
++  var e = newException(FetchError, msg)
++  e.status = status
++  e.retryable = isRetryable(status)
++  raise e
++
++proc fetchFeed*(url: string, cfg = defaultConfig(),
++                transport: Transport = nil,
++                sleep: proc(ms: int) {.gcsafe.} = nil): ParseResult =
++  ## Fetch and parse, retrying only what is worth retrying.
++  let tr = if transport != nil: transport else: httpTransport(cfg)
++  proc defaultSleep(ms: int) {.gcsafe.} = os.sleep(ms)
++  let nap = if sleep != nil: sleep else: defaultSleep
++
++  var lastMsg = "feed fetch failed"
++  var lastStatus = 0
++
++  for attempt in 0 .. cfg.maxRetries:
++    var status = 0
++    var body = ""
++    var retryAfter = ""
++    var transportFailed = false
++
++    try:
++      (status, body, retryAfter) = tr(url)
++    except CatchableError as e:
++      # A connection that never completed: worth retrying, since it says
++      # nothing about whether the feed exists.
++      transportFailed = true
++      lastMsg = "fetching " & url & ": " & e.msg
++      lastStatus = 0
++
++    if not transportFailed:
++      if status >= 200 and status < 300:
++        try:
++          return parseFeed(body, url)
++        except FeedParseError as e:
++          # Not retryable: the bytes will be the same next time.
++          raiseFetch(status, "parsing " & url & ": " & e.msg)
++      lastStatus = status
++      lastMsg = "fetching " & url & ": HTTP " & $status
++      if not isRetryable(status):
++        raiseFetch(status, lastMsg)
++
++    if attempt == cfg.maxRetries:
++      break
++    nap(backoffMs(cfg, attempt + 1, retryAfter, lastStatus))
++
++  raiseFetch(lastStatus, lastMsg)
++
++proc isAtProtoFeed*(url: string): bool = url.startsWith("at://")
++
++proc fetchOrDiscover*(url: string, cfg = defaultConfig(),
++                      transport: Transport = nil,
++                      candidates: seq[string] = @[]): (ParseResult, string) =
++  ## Fetch `url`; failing that, try feeds discovered at it.
++  ##
++  ## Returns which URL actually worked, because the caller stores that rather
++  ## than what the user typed -- people paste a site's home page far more
++  ## often than its feed.
++  try:
++    return (fetchFeed(url, cfg, transport), url)
++  except FetchError:
++    if isAtProtoFeed(url) or candidates.len == 0:
++      raise
++  for candidate in candidates:
++    try:
++      return (fetchFeed(candidate, cfg, transport), candidate)
++    except FetchError:
++      continue
++  raiseFetch(0, "no working feed found at " & url)
++  (ParseResult(), url)   # unreachable; satisfies the return type

added spike/nim/feedfetcher_probe.nim +229 -0 Viewed

new file mode 100644
@@ -0,0 +1,229 @@
	1	+## Checks the fetch retry policy.
	2	+##
	3	+## nim c -r -d:ssl feedfetcher_probe.nim
	4	+##
	5	+## The policy is the whole point, so almost everything here runs against a
	6	+## scripted transport rather than the network: the cases that matter are a
	7	+## server returning 503 twice then succeeding, a 404 that must not be
	8	+## retried, and a 429 naming its own delay. None of those are reproducible
	9	+## against a real host on demand.
	10	+
	11	+import std/[strformat, strutils, tables]
	12	+import atproto/[feedfetcher, feedparser]
	13	+
	14	+var failures = 0
	15	+
	16	+proc report(name: string, ok: bool, detail = "") =
	17	+ if not ok: inc failures
	18	+ let label = if ok: "PASS" else: "FAIL"
	19	+ if detail.len > 0: echo &" {label} {name} -- {detail}"
	20	+ else: echo &" {label} {name}"
	21	+
	22	+const goodFeed = """<?xml version="1.0"?>
	23	+<rss version="2.0"><channel><title>T</title><link>https://x.test</link>
	24	+<item><title>One</title><link>https://x.test/1</link></item>
	25	+</channel></rss>"""
	26	+
	27	+# --- a transport that answers from a script --------------------------------
	28	+
	29	+var attempts = 0
	30	+var slept: seq[int] = @[]
	31	+
	32	+proc scripted(responses: seq[tuple[status: int, body, retryAfter: string]]): Transport =
	33	+ attempts = 0
	34	+ result = proc(url: string): tuple[status: int, body: string,
	35	+ retryAfter: string] {.gcsafe.} =
	36	+ {.cast(gcsafe).}:
	37	+ let i = min(attempts, responses.high)
	38	+ inc attempts
	39	+ result = responses[i]
	40	+
	41	+proc failing(): Transport =
	42	+ attempts = 0
	43	+ result = proc(url: string): tuple[status: int, body: string,
	44	+ retryAfter: string] {.gcsafe.} =
	45	+ {.cast(gcsafe).}:
	46	+ inc attempts
	47	+ raise newException(IOError, "connection refused")
	48	+
	49	+proc recordSleep(ms: int) {.gcsafe.} =
	50	+ ## The probe is single-threaded; the annotation is what the callback
	51	+ ## type demands, not a claim about concurrency.
	52	+ {.cast(gcsafe).}:
	53	+ slept.add ms
	54	+
	55	+proc fastConfig(): FetchConfig =
	56	+ result = defaultConfig()
	57	+ result.baseDelayMs = 10 # the probe asserts the curve, not the wait
	58	+
	59	+proc main() =
	60	+ echo "Feed fetcher probe"
	61	+ echo ""
	62	+
	63	+ # --- the happy path -----------------------------------------------------
	64	+ block:
	65	+ slept = @[]
	66	+ let r = fetchFeed("https://x.test/feed", fastConfig(),
	67	+ scripted(@[(200, goodFeed, "")]), recordSleep)
	68	+ report("a 200 parses", r.articles.len == 1, r.feed.title)
	69	+ report("success does not retry", attempts == 1, $attempts)
	70	+ report("success does not sleep", slept.len == 0)
	71	+
	72	+ # --- transient failures -------------------------------------------------
	73	+ block:
	74	+ slept = @[]
	75	+ let r = fetchFeed("https://x.test/feed", fastConfig(),
	76	+ scripted(@[(503, "down", ""), (503, "down", ""),
	77	+ (200, goodFeed, "")]), recordSleep)
	78	+ report("a 5xx is retried until it succeeds",
	79	+ r.articles.len == 1 and attempts == 3, &"{attempts} attempts")
	80	+ report("backoff doubles", slept == @[10, 20], $slept)
	81	+
	82	+ block:
	83	+ slept = @[]
	84	+ var raised = false
	85	+ try:
	86	+ discard fetchFeed("https://x.test/feed", fastConfig(),
	87	+ scripted(@[(500, "down", "")]), recordSleep)
	88	+ except FetchError as e:
	89	+ raised = true
	90	+ report("a persistent 5xx eventually gives up", e.status == 500, $e.status)
	91	+ report("it raised rather than returning empty", raised)
	92	+ # maxRetries=3 means four attempts and three waits.
	93	+ report("it stops after maxRetries", attempts == 4, &"{attempts} attempts")
	94	+ report("it waits between each", slept == @[10, 20, 40], $slept)
	95	+
	96	+ # --- permanent failures -------------------------------------------------
	97	+ # The case the Go version gets wrong: a 404 is not going to change.
	98	+ block:
	99	+ slept = @[]
	100	+ var status = 0
	101	+ try:
	102	+ discard fetchFeed("https://x.test/feed", fastConfig(),
	103	+ scripted(@[(404, "nope", "")]), recordSleep)
	104	+ except FetchError as e:
	105	+ status = e.status
	106	+ report("a 404 is not retried", attempts == 1, &"{attempts} attempts")
	107	+ report("a 404 reports its status", status == 404, $status)
	108	+ report("a 404 does not sleep", slept.len == 0)
	109	+
	110	+ block:
	111	+ attempts = 0
	112	+ var status = -1
	113	+ try:
	114	+ discard fetchFeed("https://x.test/feed", fastConfig(),
	115	+ scripted(@[(403, "denied", "")]), recordSleep)
	116	+ except FetchError as e:
	117	+ status = e.status
	118	+ report("a 403 is not retried", attempts == 1 and status == 403,
	119	+ &"{attempts} attempts, status {status}")
	120	+
	121	+ # An error page that is not a feed must fail as a parse error, not be
	122	+ # retried: the bytes are the same every time.
	123	+ block:
	124	+ slept = @[]
	125	+ var msg = ""
	126	+ try:
	127	+ discard fetchFeed("https://x.test/feed", fastConfig(),
	128	+ scripted(@[(200, "<html>not a feed</html>", "")]),
	129	+ recordSleep)
	130	+ except FetchError as e:
	131	+ msg = e.msg
	132	+ report("an unparseable 200 is not retried", attempts == 1,
	133	+ &"{attempts} attempts")
	134	+ report("and reports a parse failure", "parsing" in msg, msg.split(':')[0])
	135	+
	136	+ # --- rate limiting ------------------------------------------------------
	137	+ block:
	138	+ slept = @[]
	139	+ discard fetchFeed("https://x.test/feed", fastConfig(),
	140	+ scripted(@[(429, "slow down", "2"), (200, goodFeed, "")]),
	141	+ recordSleep)
	142	+ report("a 429 is retried", attempts == 2, &"{attempts} attempts")
	143	+ # The server's own number beats our curve.
	144	+ report("Retry-After overrides the backoff curve", slept == @[2000], $slept)
	145	+
	146	+ block:
	147	+ slept = @[]
	148	+ discard fetchFeed("https://x.test/feed", fastConfig(),
	149	+ scripted(@[(429, "slow", "3600"), (200, goodFeed, "")]),
	150	+ recordSleep)
	151	+ # A server asking for an hour must not hold a worker for one.
	152	+ report("Retry-After is capped", slept == @[10_000], $slept)
	153	+
	154	+ block:
	155	+ slept = @[]
	156	+ discard fetchFeed("https://x.test/feed", fastConfig(),
	157	+ scripted(@[(429, "slow", ""), (200, goodFeed, "")]),
	158	+ recordSleep)
	159	+ report("a 429 without Retry-After falls back to the curve",
	160	+ slept == @[10], $slept)
	161	+
	162	+ # --- transport errors ---------------------------------------------------
	163	+ block:
	164	+ slept = @[]
	165	+ var raised = false
	166	+ try:
	167	+ discard fetchFeed("https://x.test/feed", fastConfig(), failing(),
	168	+ recordSleep)
	169	+ except FetchError:
	170	+ raised = true
	171	+ # A connection that never completed says nothing about the feed, so it
	172	+ # is worth retrying.
	173	+ report("a connection failure is retried", attempts == 4,
	174	+ &"{attempts} attempts")
	175	+ report("and eventually raises", raised)
	176	+
	177	+ # --- Retry-After parsing ------------------------------------------------
	178	+ block:
	179	+ report("Retry-After seconds parse", parseRetryAfter("120") == 120_000,
	180	+ $parseRetryAfter("120"))
	181	+ report("an absent Retry-After is zero", parseRetryAfter("") == 0)
	182	+ report("a nonsense Retry-After is zero", parseRetryAfter("soon") == 0)
	183	+ # An HTTP-date in the past means "now", not a negative wait.
	184	+ report("a past Retry-After date clamps to zero",
	185	+ parseRetryAfter("Wed, 01 Jan 2020 00:00:00 GMT") == 0)
	186	+
	187	+ block:
	188	+ report("429 is retryable", isRetryable(429))
	189	+ report("503 is retryable", isRetryable(503))
	190	+ report("404 is not retryable", not isRetryable(404))
	191	+ report("200 is not retryable", not isRetryable(200))
	192	+
	193	+ # --- discovery fallback -------------------------------------------------
	194	+ block:
	195	+ var served = initTable[string, tuple[status: int, body, retryAfter: string]]()
	196	+ served["https://site.test"] = (404, "no feed here", "")
	197	+ served["https://site.test/rss"] = (200, goodFeed, "")
	198	+ let tr: Transport = proc(url: string): tuple[status: int, body: string,
	199	+ retryAfter: string] {.gcsafe.} =
	200	+ {.cast(gcsafe).}:
	201	+ result = if url in served: served[url] else: (404, "", "")
	202	+
	203	+ let (res, usedUrl) = fetchOrDiscover(
	204	+ "https://site.test", fastConfig(), tr,
	205	+ candidates = @["https://site.test/atom", "https://site.test/rss"])
	206	+ report("discovery falls through to a working candidate",
	207	+ res.articles.len == 1, usedUrl)
	208	+ # The caller stores what worked, not what the user typed.
	209	+ report("it reports the URL that worked",
	210	+ usedUrl == "https://site.test/rss", usedUrl)
	211	+
	212	+ # --- one real fetch -----------------------------------------------------
	213	+ block:
	214	+ try:
	215	+ let r = fetchFeed("https://blog.rust-lang.org/feed.xml")
	216	+ report("a real feed fetches over HTTP", r.articles.len > 0,
	217	+ &"{r.feed.kind}, {r.articles.len} articles")
	218	+ except CatchableError as e:
	219	+ report("a real feed fetches over HTTP", false, e.msg)
	220	+
	221	+ echo ""
	222	+ if failures == 0:
	223	+ echo "RESULT: the fetcher retries what it should and nothing else."
	224	+ else:
	225	+ echo &"RESULT: {failures} check(s) failed."
	226	+ quit 1
	227	+
	228	+when isMainModule:
	229	+ main()

++## Checks the fetch retry policy.
++##
++##   nim c -r -d:ssl feedfetcher_probe.nim
++##
++## The policy is the whole point, so almost everything here runs against a
++## scripted transport rather than the network: the cases that matter are a
++## server returning 503 twice then succeeding, a 404 that must not be
++## retried, and a 429 naming its own delay. None of those are reproducible
++## against a real host on demand.
++
++import std/[strformat, strutils, tables]
++import atproto/[feedfetcher, feedparser]
++
++var failures = 0
++
++proc report(name: string, ok: bool, detail = "") =
++  if not ok: inc failures
++  let label = if ok: "PASS" else: "FAIL"
++  if detail.len > 0: echo &"  {label}  {name}  -- {detail}"
++  else: echo &"  {label}  {name}"
++
++const goodFeed = """<?xml version="1.0"?>
++<rss version="2.0"><channel><title>T</title><link>https://x.test</link>
++<item><title>One</title><link>https://x.test/1</link></item>
++</channel></rss>"""
++
++# --- a transport that answers from a script --------------------------------
++
++var attempts = 0
++var slept: seq[int] = @[]
++
++proc scripted(responses: seq[tuple[status: int, body, retryAfter: string]]): Transport =
++  attempts = 0
++  result = proc(url: string): tuple[status: int, body: string,
++                                    retryAfter: string] {.gcsafe.} =
++    {.cast(gcsafe).}:
++      let i = min(attempts, responses.high)
++      inc attempts
++      result = responses[i]
++
++proc failing(): Transport =
++  attempts = 0
++  result = proc(url: string): tuple[status: int, body: string,
++                                    retryAfter: string] {.gcsafe.} =
++    {.cast(gcsafe).}:
++      inc attempts
++    raise newException(IOError, "connection refused")
++
++proc recordSleep(ms: int) {.gcsafe.} =
++  ## The probe is single-threaded; the annotation is what the callback
++  ## type demands, not a claim about concurrency.
++  {.cast(gcsafe).}:
++    slept.add ms
++
++proc fastConfig(): FetchConfig =
++  result = defaultConfig()
++  result.baseDelayMs = 10        # the probe asserts the curve, not the wait
++
++proc main() =
++  echo "Feed fetcher probe"
++  echo ""
++
++  # --- the happy path -----------------------------------------------------
++  block:
++    slept = @[]
++    let r = fetchFeed("https://x.test/feed", fastConfig(),
++                      scripted(@[(200, goodFeed, "")]), recordSleep)
++    report("a 200 parses", r.articles.len == 1, r.feed.title)
++    report("success does not retry", attempts == 1, $attempts)
++    report("success does not sleep", slept.len == 0)
++
++  # --- transient failures -------------------------------------------------
++  block:
++    slept = @[]
++    let r = fetchFeed("https://x.test/feed", fastConfig(),
++                      scripted(@[(503, "down", ""), (503, "down", ""),
++                                 (200, goodFeed, "")]), recordSleep)
++    report("a 5xx is retried until it succeeds",
++           r.articles.len == 1 and attempts == 3, &"{attempts} attempts")
++    report("backoff doubles", slept == @[10, 20], $slept)
++
++  block:
++    slept = @[]
++    var raised = false
++    try:
++      discard fetchFeed("https://x.test/feed", fastConfig(),
++                        scripted(@[(500, "down", "")]), recordSleep)
++    except FetchError as e:
++      raised = true
++      report("a persistent 5xx eventually gives up", e.status == 500, $e.status)
++    report("it raised rather than returning empty", raised)
++    # maxRetries=3 means four attempts and three waits.
++    report("it stops after maxRetries", attempts == 4, &"{attempts} attempts")
++    report("it waits between each", slept == @[10, 20, 40], $slept)
++
++  # --- permanent failures -------------------------------------------------
++  # The case the Go version gets wrong: a 404 is not going to change.
++  block:
++    slept = @[]
++    var status = 0
++    try:
++      discard fetchFeed("https://x.test/feed", fastConfig(),
++                        scripted(@[(404, "nope", "")]), recordSleep)
++    except FetchError as e:
++      status = e.status
++    report("a 404 is not retried", attempts == 1, &"{attempts} attempts")
++    report("a 404 reports its status", status == 404, $status)
++    report("a 404 does not sleep", slept.len == 0)
++
++  block:
++    attempts = 0
++    var status = -1
++    try:
++      discard fetchFeed("https://x.test/feed", fastConfig(),
++                        scripted(@[(403, "denied", "")]), recordSleep)
++    except FetchError as e:
++      status = e.status
++    report("a 403 is not retried", attempts == 1 and status == 403,
++           &"{attempts} attempts, status {status}")
++
++  # An error page that is not a feed must fail as a parse error, not be
++  # retried: the bytes are the same every time.
++  block:
++    slept = @[]
++    var msg = ""
++    try:
++      discard fetchFeed("https://x.test/feed", fastConfig(),
++                        scripted(@[(200, "<html>not a feed</html>", "")]),
++                        recordSleep)
++    except FetchError as e:
++      msg = e.msg
++    report("an unparseable 200 is not retried", attempts == 1,
++           &"{attempts} attempts")
++    report("and reports a parse failure", "parsing" in msg, msg.split(':')[0])
++
++  # --- rate limiting ------------------------------------------------------
++  block:
++    slept = @[]
++    discard fetchFeed("https://x.test/feed", fastConfig(),
++                      scripted(@[(429, "slow down", "2"), (200, goodFeed, "")]),
++                      recordSleep)
++    report("a 429 is retried", attempts == 2, &"{attempts} attempts")
++    # The server's own number beats our curve.
++    report("Retry-After overrides the backoff curve", slept == @[2000], $slept)
++
++  block:
++    slept = @[]
++    discard fetchFeed("https://x.test/feed", fastConfig(),
++                      scripted(@[(429, "slow", "3600"), (200, goodFeed, "")]),
++                      recordSleep)
++    # A server asking for an hour must not hold a worker for one.
++    report("Retry-After is capped", slept == @[10_000], $slept)
++
++  block:
++    slept = @[]
++    discard fetchFeed("https://x.test/feed", fastConfig(),
++                      scripted(@[(429, "slow", ""), (200, goodFeed, "")]),
++                      recordSleep)
++    report("a 429 without Retry-After falls back to the curve",
++           slept == @[10], $slept)
++
++  # --- transport errors ---------------------------------------------------
++  block:
++    slept = @[]
++    var raised = false
++    try:
++      discard fetchFeed("https://x.test/feed", fastConfig(), failing(),
++                        recordSleep)
++    except FetchError:
++      raised = true
++    # A connection that never completed says nothing about the feed, so it
++    # is worth retrying.
++    report("a connection failure is retried", attempts == 4,
++           &"{attempts} attempts")
++    report("and eventually raises", raised)
++
++  # --- Retry-After parsing ------------------------------------------------
++  block:
++    report("Retry-After seconds parse", parseRetryAfter("120") == 120_000,
++           $parseRetryAfter("120"))
++    report("an absent Retry-After is zero", parseRetryAfter("") == 0)
++    report("a nonsense Retry-After is zero", parseRetryAfter("soon") == 0)
++    # An HTTP-date in the past means "now", not a negative wait.
++    report("a past Retry-After date clamps to zero",
++           parseRetryAfter("Wed, 01 Jan 2020 00:00:00 GMT") == 0)
++
++  block:
++    report("429 is retryable", isRetryable(429))
++    report("503 is retryable", isRetryable(503))
++    report("404 is not retryable", not isRetryable(404))
++    report("200 is not retryable", not isRetryable(200))
++
++  # --- discovery fallback -------------------------------------------------
++  block:
++    var served = initTable[string, tuple[status: int, body, retryAfter: string]]()
++    served["https://site.test"] = (404, "no feed here", "")
++    served["https://site.test/rss"] = (200, goodFeed, "")
++    let tr: Transport = proc(url: string): tuple[status: int, body: string,
++                                                 retryAfter: string] {.gcsafe.} =
++      {.cast(gcsafe).}:
++        result = if url in served: served[url] else: (404, "", "")
++
++    let (res, usedUrl) = fetchOrDiscover(
++      "https://site.test", fastConfig(), tr,
++      candidates = @["https://site.test/atom", "https://site.test/rss"])
++    report("discovery falls through to a working candidate",
++           res.articles.len == 1, usedUrl)
++    # The caller stores what worked, not what the user typed.
++    report("it reports the URL that worked",
++           usedUrl == "https://site.test/rss", usedUrl)
++
++  # --- one real fetch -----------------------------------------------------
++  block:
++    try:
++      let r = fetchFeed("https://blog.rust-lang.org/feed.xml")
++      report("a real feed fetches over HTTP", r.articles.len > 0,
++             &"{r.feed.kind}, {r.articles.len} articles")
++    except CatchableError as e:
++      report("a real feed fetches over HTTP", false, e.msg)
++
++  echo ""
++  if failures == 0:
++    echo "RESULT: the fetcher retries what it should and nothing else."
++  else:
++    echo &"RESULT: {failures} check(s) failed."
++    quit 1
++
++when isMainModule:
++  main()

modified spike/nim/glean_spike.nimble +1 -0 Viewed

@@ -29,6 +29,7 @@ const networkProbes = @[
29	29	"xrpc_probe",
30	30	"jetstream_probe",
31	31	"feedparser_probe", # fixtures are offline, but it also fetches real feeds
	32	+ "feedfetcher_probe", # retry policy is scripted; one check hits the network
32	33	]
33	34
34	35	proc runProbe(name: string) =

    "xrpc_probe",
    "jetstream_probe",
    "feedparser_probe",   # fixtures are offline, but it also fetches real feeds
++  "feedfetcher_probe",  # retry policy is scripted; one check hits the network
  ]
  proc runProbe(name: string) =

modified spike/nim/readme.md +40 -0 Viewed

@@ -406,6 +406,46 @@ four real feeds across two formats (Rust Blog, nim-lang, LWN, Hacker News)
406	406	and asserts every item has a guid and a title. All four parse, 65 items
407	407	between them, all dated and all carrying text.
408	408
	409	+## 12. Fetching feeds
	410	+
	411	+`atproto/feedfetcher.nim` ports `internal/feed/fetcher.go`. Retries exist for
	412	+servers that are briefly unwell -- 429 and 5xx -- and for nothing else. A 404
	413	+will still be a 404 in a second, and bytes that do not parse will not parse
	414	+differently on a second read.
	415	+
	416	+The retry policy is the substance, so the probe drives it with a scripted
	417	+transport: 503 twice then success, a 404 that must not be retried, a 429
	418	+naming its own delay. None of those are reproducible against a real host on
	419	+demand.
	420	+
	421	+### A bug in the Go fetcher
	422	+
	423	+The Go retry policy does not do what it reads as doing. `executeRequest`
	424	+returns its `http.Response` only on success*, so in `Fetch` the guard
	425	+
	426	+```go
	427	+if resp != nil && !httpclient.IsRetryable(resp.StatusCode) {
	428	+```
	429	+
	430	+can never be true when `err != nil` -- the response is always nil on the
	431	+error path. Three things follow:
	432	+
	433	+- Every failure retries the full four attempts regardless of status, so a
	434	+ permanently-404 feed costs four requests and ~7s of backoff on every
	435	+ refresh cycle.
	436	+- `retryBackoff`'s `Retry-After` handling is unreachable for the same reason:
	437	+ `lastResp` is only non-nil once the function has already returned, so a
	438	+ rate-limited server's own delay is never honoured.
	439	+- The HTTP status is never checked at all, so an error page is handed to the
	440	+ parser and fails there instead of failing as an HTTP error.
	441	+
	442	+This port checks the status, honours `Retry-After` (capped, so a server
	443	+asking for an hour cannot hold a worker for one), and retries only what is
	444	+worth retrying. That is a behaviour change rather than a translation, which
	445	+is why it is written down both here and at the top of the module.
	446	+
	447	+Worth fixing on the Go side too; it is a handful of lines.
	448	+
409	449	## What this does and does not prove
410	450
411	451	Proven: the cryptography, the storage layer, and the OAuth flow up to user

  and asserts every item has a guid and a title. All four parse, 65 items
  between them, all dated and all carrying text.
++## 12. Fetching feeds
++
++`atproto/feedfetcher.nim` ports `internal/feed/fetcher.go`. Retries exist for
++servers that are briefly unwell -- 429 and 5xx -- and for nothing else. A 404
++will still be a 404 in a second, and bytes that do not parse will not parse
++differently on a second read.
++
++The retry policy is the substance, so the probe drives it with a scripted
++transport: 503 twice then success, a 404 that must not be retried, a 429
++naming its own delay. None of those are reproducible against a real host on
++demand.
++
++### A bug in the Go fetcher
++
++The Go retry policy does not do what it reads as doing. `executeRequest`
++returns its `*http.Response` **only on success**, so in `Fetch` the guard
++
++```go
++if resp != nil && !httpclient.IsRetryable(resp.StatusCode) {
++```
++
++can never be true when `err != nil` -- the response is always nil on the
++error path. Three things follow:
++
++- Every failure retries the full four attempts regardless of status, so a
++  permanently-404 feed costs four requests and ~7s of backoff on every
++  refresh cycle.
++- `retryBackoff`'s `Retry-After` handling is unreachable for the same reason:
++  `lastResp` is only non-nil once the function has already returned, so a
++  rate-limited server's own delay is never honoured.
++- The HTTP status is never checked at all, so an error page is handed to the
++  parser and fails there instead of failing as an HTTP error.
++
++This port checks the status, honours `Retry-After` (capped, so a server
++asking for an hour cannot hold a worker for one), and retries only what is
++worth retrying. That is a behaviour change rather than a translation, which
++is why it is written down both here and at the top of the module.
++
++Worth fixing on the Go side too; it is a handful of lines.
++
  ## What this does and does not prove
  Proven: the cryptography, the storage layer, and the OAuth flow up to user