Repository navigation
fix: Crossref 429 resilience + salvage sweep + OpenAlex select fix (v1.3.1) - #7
Merged
Merged
Conversation
…1.3.1) - fetchRetry honors Retry-After on 429 with a shared per-host cooldown (jittered), so parallel reference checks stop retrying into the same rate-limit window and dying as check_failed en masse. - quickCheck/checkDocument re-run check_failed refs once, sequentially, after the burst drains (60s budget) — transient failures self-heal. - OpenAlex /works select used invalid field is_oa (HTTP 400 on every lookup); select open_access and read open_access.is_oa.
📝 WalkthroughWalkthroughVersion 1.3.1 updates HTTP 429 retry coordination, adds transient citation salvage checks, corrects OpenAlex open-access enrichment, and adds tests covering retry, cooldown, salvage, and field-selection behavior. ChangesReliability and enrichment updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)HTTP retry cooldown flowsequenceDiagram
participant fetchRetry
participant HostCooldowns
participant HTTPServer
fetchRetry->>HostCooldowns: await shared host cooldown
fetchRetry->>HTTPServer: send request
HTTPServer-->>fetchRetry: return 429 and Retry-After
fetchRetry->>HostCooldowns: note host cooldown
fetchRetry->>HTTPServer: retry after shared wait
Citation salvage flowsequenceDiagram
participant quickCheck
participant checkDocument
participant salvageCheckFailed
participant Crossref
quickCheck->>salvageCheckFailed: pass failed citation results
checkDocument->>salvageCheckFailed: pass failed document citations
salvageCheckFailed->>Crossref: sequentially recheck failed references
Crossref-->>salvageCheckFailed: return recovered or failed result
salvageCheckFailed-->>quickCheck: update results in place
salvageCheckFailed-->>checkDocument: update results in place
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/quick-check.ts`:
- Around line 217-234: Update salvageCheckFailed so each recheck invocation is
isolated from failures: catch exceptions from recheck(inputs[i]!) and continue
processing the remaining failed indices, while preserving successfully returned
retry results in results[i]. Ensure a thrown retry does not propagate to
quickCheck, checkDocument, or abort earlier salvaged results; add coverage for a
mid-sweep recheck rejection if the existing tests support it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3cf7369-3c22-4112-bb49-61c5e72ef105
📒 Files selected for processing (10)
CHANGELOG.mdpackage.jsonsrc/document.tssrc/http.tssrc/openalex.tssrc/quick-check.tssrc/references/match.tstest/http-retry.test.tstest/openalex.test.tstest/salvage.test.ts
Comment on lines
+217
to
+234
| export async function salvageCheckFailed<T>( | ||
| results: CitationCheckResult[], | ||
| inputs: T[], | ||
| recheck: (input: T) => Promise<CitationCheckResult>, | ||
| ): Promise<void> { | ||
| const failed = results | ||
| .map((r, i) => (r.status === "check_failed" ? i : -1)) | ||
| .filter((i) => i >= 0); | ||
| if (failed.length === 0) return; | ||
| // Let any active rate-limit window pass before the first re-attempt. | ||
| await new Promise((r) => setTimeout(r, 1000)); | ||
| const deadline = Date.now() + 60_000; | ||
| for (const i of failed) { | ||
| if (Date.now() > deadline) return; | ||
| const retry = await recheck(inputs[i]!); | ||
| if (retry.status !== "check_failed") results[i] = retry; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
salvageCheckFailed will crash the whole sweep if a single recheck call throws.
The loop's await recheck(inputs[i]!) isn't guarded. If any recheck (e.g. Crossref succeeds but a subsequent DOAJ/ISSN lookup in checkSingle/checkFreeTextRef throws) raises an unhandled exception, it propagates out of salvageCheckFailed and up through quickCheck/checkDocument, aborting the entire operation — including references already salvaged earlier in the same loop. This undermines the sweep's whole purpose: turning a resilience feature into a single point of failure. The same risk applies to the document.ts call site since it shares this function.
🛡️ Proposed fix: don't let one bad retry abort the sweep
for (const i of failed) {
if (Date.now() > deadline) return;
- const retry = await recheck(inputs[i]!);
- if (retry.status !== "check_failed") results[i] = retry;
+ try {
+ const retry = await recheck(inputs[i]!);
+ if (retry.status !== "check_failed") results[i] = retry;
+ } catch {
+ // Leave the original check_failed result; a bad retry should not
+ // abort the remaining sweep or crash the caller.
+ }
}Consider also adding a test where recheck throws mid-sweep, to lock in this behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function salvageCheckFailed<T>( | |
| results: CitationCheckResult[], | |
| inputs: T[], | |
| recheck: (input: T) => Promise<CitationCheckResult>, | |
| ): Promise<void> { | |
| const failed = results | |
| .map((r, i) => (r.status === "check_failed" ? i : -1)) | |
| .filter((i) => i >= 0); | |
| if (failed.length === 0) return; | |
| // Let any active rate-limit window pass before the first re-attempt. | |
| await new Promise((r) => setTimeout(r, 1000)); | |
| const deadline = Date.now() + 60_000; | |
| for (const i of failed) { | |
| if (Date.now() > deadline) return; | |
| const retry = await recheck(inputs[i]!); | |
| if (retry.status !== "check_failed") results[i] = retry; | |
| } | |
| } | |
| export async function salvageCheckFailed<T>( | |
| results: CitationCheckResult[], | |
| inputs: T[], | |
| recheck: (input: T) => Promise<CitationCheckResult>, | |
| ): Promise<void> { | |
| const failed = results | |
| .map((r, i) => (r.status === "check_failed" ? i : -1)) | |
| .filter((i) => i >= 0); | |
| if (failed.length === 0) return; | |
| // Let any active rate-limit window pass before the first re-attempt. | |
| await new Promise((r) => setTimeout(r, 1000)); | |
| const deadline = Date.now() + 60_000; | |
| for (const i of failed) { | |
| if (Date.now() > deadline) return; | |
| try { | |
| const retry = await recheck(inputs[i]!); | |
| if (retry.status !== "check_failed") results[i] = retry; | |
| } catch { | |
| // Leave the original check_failed result; a bad retry should not | |
| // abort the remaining sweep or crash the caller. | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/quick-check.ts` around lines 217 - 234, Update salvageCheckFailed so each
recheck invocation is isolated from failures: catch exceptions from
recheck(inputs[i]!) and continue processing the remaining failed indices, while
preserving successfully returned retry results in results[i]. Ensure a thrown
retry does not propagate to quickCheck, checkDocument, or abort earlier salvaged
results; add coverage for a mid-sweep recheck rejection if the existing tests
support it.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/http.ts">
<violation number="1" location="src/http.ts:118">
P2: Concurrent retries still stampede when the shared cooldown expires: jitter is applied once to the shared deadline, so all waiters wake at the same instant. Apply a per-request randomized delay after the shared gate (while rechecking any extended cooldown) so the released batch is actually spread out.</violation>
</file>
<file name="src/quick-check.ts">
<violation number="1" location="src/quick-check.ts:231">
P2: A recheck started just before `deadline` can still block the scan for over a minute because its retry/cooldown waits are not deadline-bound. Enforce the remaining budget through the recheck/HTTP cancellation path, or at least prevent the sweep from claiming a 60-second wall-clock cap.</violation>
</file>
<file name="src/references/match.ts">
<violation number="1" location="src/references/match.ts:258">
P3: Minor cleanup: `?? oa.is_oa` fallback is dead code since `lookupByDoi` never returns `is_oa` at the top level. Could be dropped to `isOa: oa.open_access?.is_oa` (keeping `??` only if there's intent to defend against a future shape change).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (attempt < retries) { | ||
| if (res.status === 429) { | ||
| const ra = parseRetryAfterMs(res) ?? 300 * (attempt + 1) ** 2; | ||
| noteCooldown(host, ra + Math.random() * 300); |
There was a problem hiding this comment.
P2: Concurrent retries still stampede when the shared cooldown expires: jitter is applied once to the shared deadline, so all waiters wake at the same instant. Apply a per-request randomized delay after the shared gate (while rechecking any extended cooldown) so the released batch is actually spread out.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/http.ts, line 118:
<comment>Concurrent retries still stampede when the shared cooldown expires: jitter is applied once to the shared deadline, so all waiters wake at the same instant. Apply a per-request randomized delay after the shared gate (while rechecking any extended cooldown) so the released batch is actually spread out.</comment>
<file context>
@@ -46,22 +46,78 @@ export interface FetchOpts {
- if (attempt < retries) {
+ if (res.status === 429) {
+ const ra = parseRetryAfterMs(res) ?? 300 * (attempt + 1) ** 2;
+ noteCooldown(host, ra + Math.random() * 300);
+ if (attempt < retries) continue; // top of loop awaits the cooldown
+ } else if (attempt < retries) {
</file context>
| const deadline = Date.now() + 60_000; | ||
| for (const i of failed) { | ||
| if (Date.now() > deadline) return; | ||
| const retry = await recheck(inputs[i]!); |
There was a problem hiding this comment.
P2: A recheck started just before deadline can still block the scan for over a minute because its retry/cooldown waits are not deadline-bound. Enforce the remaining budget through the recheck/HTTP cancellation path, or at least prevent the sweep from claiming a 60-second wall-clock cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/quick-check.ts, line 231:
<comment>A recheck started just before `deadline` can still block the scan for over a minute because its retry/cooldown waits are not deadline-bound. Enforce the remaining budget through the recheck/HTTP cancellation path, or at least prevent the sweep from claiming a 60-second wall-clock cap.</comment>
<file context>
@@ -207,6 +207,32 @@ async function checkSingle(item: CslItemData): Promise<CitationCheckResult> {
+ const deadline = Date.now() + 60_000;
+ for (const i of failed) {
+ if (Date.now() > deadline) return;
+ const retry = await recheck(inputs[i]!);
+ if (retry.status !== "check_failed") results[i] = retry;
+ }
</file context>
| result.openalexMatch = { | ||
| citedByCount: oa.cited_by_count, | ||
| isOa: oa.is_oa, | ||
| isOa: oa.open_access?.is_oa ?? oa.is_oa, |
There was a problem hiding this comment.
P3: Minor cleanup: ?? oa.is_oa fallback is dead code since lookupByDoi never returns is_oa at the top level. Could be dropped to isOa: oa.open_access?.is_oa (keeping ?? only if there's intent to defend against a future shape change).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/references/match.ts, line 258:
<comment>Minor cleanup: `?? oa.is_oa` fallback is dead code since `lookupByDoi` never returns `is_oa` at the top level. Could be dropped to `isOa: oa.open_access?.is_oa` (keeping `??` only if there's intent to defend against a future shape change).</comment>
<file context>
@@ -255,7 +255,7 @@ export async function checkFreeTextRef(raw: string): Promise<CitationCheckResult
result.openalexMatch = {
citedByCount: oa.cited_by_count,
- isOa: oa.is_oa,
+ isOa: oa.open_access?.is_oa ?? oa.is_oa,
journalName: oa.primary_location?.source?.display_name,
};
</file context>
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
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.
Problem
A parallel batch of reference checks against Crossref's anonymous pool reliably hits 429 storms: 10 concurrent
query.bibliographicsearches get rate-limited, the fixed 300ms/1200ms retry backoff re-enters the same window, all attempts die, and the majority of a real bibliography lands ascheck_failed("could not be checked"). Reproduced with a realistic 35-ref APA list: 14/35 check_failed, 58× HTTP 429.Separately, every OpenAlex
/workslookup silently failed with HTTP 400 becauseis_oais not a valid top-level select field — no citation ever got citation-count / OA enrichment.Fix
fetchRetry: honorRetry-After(delta-seconds or HTTP-date, capped 30s) on 429 and share the wait across all requests to the same host via a per-host cooldown map, with jitter so a released batch does not stampede back in lockstep. 5xx/network/timeout keep the existing backoff.quickCheck+checkDocument): refs that still endedcheck_failedare re-checked once, sequentially, after the burst has drained, under a 60s wall-clock budget. Transient failures self-heal instead of surfacing to the user.open_access(valid) and readopen_access.is_oa.Result (same 35-ref list, anonymous pool)
Before: 14 check_failed / 15 verified, OpenAlex 100% HTTP 400.
After: 0 check_failed / 25 verified / 9 partial / 1 not_found, OpenAlex enrichment working. 429s still occur but are absorbed.
Tests
10 new tests (http-retry, salvage, openalex select); full suite 138 pass.
Summary by cubic
Improves Crossref rate-limit handling and restores OpenAlex enrichment, reducing false
check_failedresults during parallel checks. Honors server backoff and retries failed refs once to self-heal transient errors.fetchRetryhonorsRetry-After(seconds or HTTP-date), applies a shared per-host cooldown with jitter, and keeps existing backoff for 5xx/network errors.check_failedrefs once, sequentially, under a 60s budget inquickCheckandcheckDocument.open_accessand readopen_access.is_oa(replaces invalidis_oaselect) to restore citation-count and OA enrichment.Written for commit a22f3be. Summary will update on new commits.
Summary by CodeRabbit