central: honor Retry-After and resume the same chunk on 429/503 (#118)

[philcunliffe]

Fixes#118. Client (proactive) half of the dual fix for the central-forward livelock; the server (defensive) half is hyparam/hypaware-server#3 (Fixes hyparam/hypaware-server#2).

Problem

The forward sink treated any non-2xx as fatal — postNdjson special-cased only 401 and threw on everything else, and that throw unwound the whole partition. With the sink also re-streaming each partition from row 0 every tick, a partition larger than the server's byte-rate burst (~64 MB) could never fully land: the server 429s once the bucket drains, the client gives up, and the tail beyond the burst is never delivered (and can age out of cache retention → silent loss). The server even sent Retry-After: 30, which the client ignored.

Fix

Treat 429/503 as backpressure, not failure:

• parse Retry-After (fall back to the linear ladder 30→60→120→300 when absent/garbage),
• sleep and retry the same chunk — identical body + X-Hyp-Batch-Id, so it stays idempotent,
• bound the inline wait per chunk (~5 min); past it, throw so the driver respools and the next tick resumes — cheap now that the server dedupes the delivered prefix without charging rate budget (hyparam/hypaware-server#2),
• the wait is abortable: close() aborts an in-flight pause so daemon shutdown is never wedged.

401 keeps its one-shot refresh+retry. Other 4xx/5xx still throw (driver classifies). Narrowing poison-drop (400/422) and adding a forward cursor / proactive pacing remain follow-ups — called out in proto.md.

Structure

• New central/src/backoff.js: the canonical home for parseRetryAfter + RETRY_BACKOFF_SECONDS (lifted from config_client.js, which now imports them) and a new interruptible abortableSleep.
• proto.md: the "Client status" caveat rewritten — Retry-After is now honored.

Tests

central-forward-chunking.test.js gains: Retry-After honored vs ladder fallback, 503-as-backpressure, repeated-429 ladder walk, inline-budget exhaustion → partition retry, central.forward.backpressure telemetry, and close() aborting a paused chunk (real abortableSleep). New central-backoff.test.js unit-tests abortableSleep. Backpressure waits are driven through an injected sleepFn so tests don't actually sleep.

Full suite green (1203 pass / 0 fail / 1 pre-existing skip); npm run typecheck and npm run lint clean.

Rollout

Land hyparam/hypaware-server#3 first (backward-compatible; helps unpatched clients), then this.

[philcunliffe]

Dual-agent review — request_changes

• Verdict:request_changes
• Risk class:medium
• Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

Source	Finding (severity, evidence)	Intersects
Codex #1 / Claude #1	major — sink.js:391-413 (+backoff.js:24-31) zero-delay livelock	Risks #1; Concurrency surface (postNdjson loop); Direct callers (parseRetryAfter)
Codex #7 / Claude #2	major (Codex) / minor (Claude) — sink.js:391-413 undrained body	Risks #2; Concurrency surface (postNdjson loop)
Claude #3	minor — sink.js/backoff.js no LLP capture / no @ref	Targets (backoff.js, sink.js); no correctness/leak surface

Codex review

Fix Validations

429/503 backpressure should retry the same chunk

• Status: incomplete
• Evidence:hypaware-core/plugins-workspace/central/src/sink.js:391, hypaware-core/plugins-workspace/central/src/sink.js:410, test/plugins/central-forward-chunking.test.js:297
• Assessment: Positive-delay 429/503 handling now retries the same body and batch id, with tests for Retry-After, fallback ladder, 503, budget exhaustion, telemetry, and close abort. One legal header case still breaks the promised bounded wait; see finding.

Existing respool handling alone did not resolve #118

• Status: correct
• Evidence:hypaware-core/plugins-workspace/central/src/sink.js:97, hypaware-core/plugins-workspace/central/src/sink.js:123
• Assessment: The existing partition retry path only reports retryPartitions after failure, so without inline same-chunk pacing it can keep replaying from the partition start and hit the same server throttle.

Findings

1) Behavioral Correctness

• Severity: major
• Confidence: high
• Evidence:hypaware-core/plugins-workspace/central/src/backoff.js:27, hypaware-core/plugins-workspace/central/src/backoff.js:29, hypaware-core/plugins-workspace/central/src/sink.js:397, hypaware-core/plugins-workspace/central/src/sink.js:410
• Why it matters: A legal Retry-After: 0 or past HTTP-date produces delayMs === 0, so waitedMs never advances and the for (;;) loop can retry the same throttled chunk forever, violating the stated 5 minute inline bound.
• Suggested fix: Add a wall-clock deadline or max-attempt cap independent of delayMs, or clamp sink backpressure sleeps to a positive minimum before applying the budget.

7) Resource Lifecycle & Cleanup

• Severity: major
• Confidence: medium
• Evidence:hypaware-core/plugins-workspace/central/src/sink.js:391, hypaware-core/plugins-workspace/central/src/sink.js:410, hypaware-core/plugins-workspace/central/src/sink.js:430
• Why it matters: The 429/503 retry path continues without draining or cancelling the response body, which can hold sockets/body streams under sustained backpressure.
• Suggested fix: Before sleeping/retrying, consume or cancel the backpressure response body, e.g. await response.body?.cancel().catch(() => {}) or a bounded drain helper.

No Finding

2. Contract & Interface Fidelity
3. Change Impact / Blast Radius
4. Concurrency, Ordering & State Safety
5. Error Handling & Resilience
6. Security Surface
7. Release Safety
8. Test Evidence Quality
9. Architectural Consistency
10. Debuggability & Operability

Evidence Bundle

• Changed hot paths:sink.js:56createForwardSink, sink.js:185forwardPartition, sink.js:357postNdjson, backoff.js:24parseRetryAfter, backoff.js:44abortableSleep, config_client.js:255 config pull throttling.
• Impacted callers:hypaware-core/plugins-workspace/central/index.js:63 production sink construction; test/plugins/central-forward-chunking.test.js:121 test sink construction.
• Impacted tests:test/plugins/central-forward-chunking.test.js:297, :315, :326, :338, :352, :370, :386; test/plugins/central-backoff.test.js:8, :14, :20, :29; test/plugins/central-config-pull.test.js:346, :359.
• Unresolved uncertainty: I did not rerun the suite; this review is based on the diff plus targeted caller/test tracing. Server behavior from hypaware-server#3 was not inspected, so the findings assume the client must defensively handle any standards-valid Retry-After.

Claude review

Claude review

Retry-After: 0 / past HTTP-date drives a tight infinite retry loop (5-min bound bypassed)

• Severity: major
• Confidence: 90
• Evidence: hypaware-core/plugins-workspace/central/src/sink.js:391-413 (with backoff.js:24-31)
• Why it matters: A legalRetry-After: 0 (RFC 9110 delta-seconds of 0) or an HTTP-date that is now/past (trivially produced by server clock skew) makes parseRetryAfter return 0, not undefined — so delaySeconds = retryAfter ?? ladder stays 0, delayMs = 0, the budget guard waitedMs + 0 > MAX is never true (waitedMs never advances), and backpressureRetries is incremented but never bounds the loop; the for (;;) re-POSTs the same chunk as fast as the event loop allows, hammering the server and completely bypassing the advertised ~5-min inline cap. This re-introduces the tight-livelock class of bug central forward sink livelocks on server 429 byte_rate — no Retry-After backoff, no resume, no forward cursor #118 was meant to kill, triggered by any standards-valid server response the PR's own defensive parse is supposed to tolerate.
• Suggested fix: Bound the loop independently of delayMs — e.g. clamp backpressure sleeps to a positive floor before the budget check, and/or add a wall-clock deadline or a max-attempt cap on backpressureRetries so a zero/near-zero delay still terminates inline.

Backpressure retry path leaks the un-drained response body

• Severity: minor
• Confidence: 80
• Evidence: hypaware-core/plugins-workspace/central/src/sink.js:391-413
• Why it matters: On the sleep-and-continue branch the 429/503 response body is never read or cancelled before the next send() (the throw branches do consume it via readErrorDetail); under undici each undrained body pins its socket/stream, so sustained backpressure accumulates them — and it compounds the tight-loop finding above into a high-rate leak.
• Suggested fix: Cancel or drain the body before sleeping, e.g. await response.body?.cancel().catch(() => {}) (or a small bounded drain helper) ahead of await sleepFn(...).

Backpressure design decision not captured in an LLP; new code carries no client-side @ref

• Severity: minor
• Confidence: 80
• Evidence: hypaware-core/plugins-workspace/central/src/sink.js:391-413, src/backoff.js:13-59 (no llp/ file touched; only server LLP 0001 cited)
• Why it matters: Reclassifying 429/503 from fatal to bounded-inline-backpressure-with-abortable-wait is exactly the kind of non-obvious Sinks design decision CLAUDE.md's "Living docs" / "@ref" conventions target; the established pattern in this subsystem annotates such choices (e.g. config_client.js carries @ref LLP 0025#config-pull-loop). The wire reference proto.mdwas updated in-commit (so no existing LLP is left factually stale — LLP 0025's 429 note is the config-pull path, untouched here), but the client-side backpressure strategy itself is recorded nowhere in llp/.
• Suggested fix: Add a short note to LLP 0014 (Sinks) or LLP 0025 capturing the bounded-inline-wait + respool + abort-on-close strategy, and attach a // @ref LLP NNNN#anchor [implements] above the backpressure loop / MAX_BACKPRESSURE_WAIT_MS.

---

Reports: .git/dual-review/pr-120

[philcunliffe]

Review findings addressed — 0598f24

Both major findings from the dual-review are fixed, plus the minor doc/@ref gap.

🔴 Zero-delay livelock (Codex #1 / Claude #1). A legal Retry-After: 0 or a past HTTP-date parsed to 0, and retryAfter ?? ladder kept the 0, so the loop retried with no delay, never advanced waitedMs, and spun. Fixed: sink.js now honors only a positiveRetry-After (||, not ??); a non-positive / absent / garbage value falls back to the ladder, so every wait advances and the ~5-min budget bounds the retries.

• Same latent bug existed in the config pull loop (config_client.js rescheduled at 0s → immediate re-poll). Fixed there too.

🟠 Undrained response body (Codex #7 / Claude #2). The sleep-and-continue branch now discardBody(response) (cancels the body) before parking, so undici returns the socket to the pool instead of pinning it across the pause and across piled-up retries.

🟡 LLP / @ref (Claude #3). Added a Forward sink backpressure section to LLP 0014 (retry-in-place, positive-Retry-After-or-ladder, bounded inline wait + respool, abortable close); sink.js now carries // @ref LLP 0014#forward-sink-backpressure. proto.md and the shared parseRetryAfter docstring are updated for the non-positive rule.

Tests added:Retry-After: 0 walks the ladder (no zero-delay spin) and respools; backpressure drains the response body; config pull with Retry-After: 0 reschedules via the ladder, not an immediate re-poll.

Full suite green (1206 pass / 1 pre-existing skip); typecheck + lint clean.