Add backfill_doab management command (refs #18) - #19
Draft
rdhyee wants to merge 7 commits into
Draft
Conversation
Slow, resumable, observable per-ID backfill for the 14,455 active DOAB records found missing from doab-check by the 2026-05-14 set-diff (#18). The nightly cron's rolling window cannot self-heal records lost during prior 429 stretches and the May 1-4 cursor stall; this command reads a static list of missing IDs and processes them via add_by_doab() with rate-limited, ID-keyed JSON state. Mirrors the design of Gluejar/regluit#1152 (codex-reviewed twice), adapted to doab-check's leaner data model (Item + Link only) and ID storage convention (oai:doab-books:<bare>): - Reuses add_by_doab() and the shared Retry-After sentinel from load_doab.py so a 429 hit by either command suppresses both. - 4-way per-record outcome taxonomy: ok / gone / present_locally / error_review. Unknown exceptions HALT (do not silently mark recovered). Validation errors are terminal but flagged. - Local existence precheck per record (Item.doab + status=1) short- circuits DOAB calls for IDs the nightly cron may have caught up on during the run. - Handles add_by_doab returning a set_deleted Item (record came back but was marked deleted): counted as 'gone', not 'ok'. - transaction.atomic() scoped to local writes only — remote OAI fetch happens before the DB transaction opens. - IntegrityError on write triggers re-precheck and promotion to present_locally (handles race with nightly cron). - Numeric ID sort. Newest-first by default. - Per-record atomic state writes. Totals recomputed from ids dict on every write — zero increment drift. - Stop conditions: first 429, max-remote-calls, error-rate-halt, gone-rate-halt, retry-after exceeds cap, network/5xx, drained. - SHA256 checksum of input file enforced on every run (live + dry). - Pacing: jittered sleep (default 3s ± 20%); 500 remote calls/run. Accepts input IDs in either bare ('20.500.12854/N') or prefixed ('oai:doab-books:20.500.12854/N') form; normalizes to canonical prefixed form for storage consistency with existing rows. No tests in this commit (followup); no cron entry yet; snapshot generator for the missing-IDs file is also a follow-up. Refs #18 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rdhyee
commented
May 14, 2026
rdhyee
left a comment
rdhyee
left a comment
Contributor
Author
There was a problem hiding this comment.
Code review findings:
-
High: the command continues processing after a short 429.
_handle_rate_limit()returnsrecovered, the caller retries the record, and then the loop continues to the next pending ID. That violates the stated first-429 stop condition and can keep calling DOAB after the shared sentinel has been written. I would checkpoint the current ID and exit after the courtesy retry, or remove the courtesy retry and always exit on 429. -
High: active rows that already exist locally with
status=0will not be recovered. The precheck intentionally skips onlystatus=1, butload_doab_record()usesItem.objects.get_or_create(doab=doab_id)and never resetsstatusto1for an active OAI record. The backfill then sees the returned item still hasstatus=0and marks the ID terminalgone, so these active records stay deleted and will not be retried. The loader or backfill needs to reactivate the item before categorizing the result asok. -
Medium: deleted OAI records are not consistently categorized as
gone.set_deleted()returnsNonewhen the deleted item is absent locally, and the command marks that aserror_review. For this backfill, a deleted OAI header for a missing local row should be a terminalgone, not an error-review case.
I did not run tests; this was a code review against the loader contracts and the PR diff.
All four codex high+medium findings on PR #19: 1. HIGH: "first 429 of run" stop condition was violated. After _handle_rate_limit returned 'recovered', the code did one retry but then fell through into the next loop iteration, continuing to make DOAB calls after the shared sentinel had been written. Fix: after the single courtesy retry, set stop_after_this_record=True and break out of the loop after categorizing the result. All 'continue' paths within the loop body now also check this flag. 2. HIGH: locally-deleted-but-DOAB-active rows were never recovered. The precheck filtered Item.status=1 (correct — we want to skip only present-AND-active rows), but load_doab_record uses get_or_create() and never resets status when an existing row is found. So an Item with status=0 + active OAI record came back from add_by_doab with status still 0, and the backfill marked it gone. Net result: rows that were correctly identified as recoverable stayed deleted forever. Fix: after a successful active-record load, if the returned item has status=0, flip it to 1 with an explicit save(update_fields=['status']) and mark 'ok' (with a note='status_flipped_0_to_1' for traceability). The underlying load_doab_record bug is filed as a separate concern. 3. MEDIUM: deleted OAI records were inconsistently categorized. set_deleted() returns None when the local Item doesn't exist (logger.warning 'no item'), and that None reached step 7f and was marked error_review instead of gone. Fix: capture record_is_deleted = record[0].isDeleted() or not record[1] before the add_by_doab call, then categorize step 7f around all four real (record_deleted × item_present × item_status) cases. Now record-deleted always becomes gone regardless of whether the local Item existed. 4. MEDIUM: transaction.atomic() framing was misleading. add_by_doab makes downstream HTTP calls (the bitstream/cover paths), so the tx does hold during HTTP. Replaced the misleading comment with an honest trade-off note. Refs #18 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed all four codex review findings in commit 54b8b99:
Followup worth tracking separately: codex finding #2 reveals an underlying bug in |
Codex pass-2 finding: the stock-pyoai urllib.error.HTTPError(429) fallback was routing through _handle_rate_limit(), which may sleep the Retry-After and return 'recovered' — but the caller immediately saved state and broke without doing the courtesy retry that the patched-fork RateLimitedError path now does. The current ID also got no terminal mark. The patched EbookFoundation/pyoai fork (pinned in Pipfile) is the supported path; the HTTPError fallback only fires if we somehow load stock pyoai. Make that fallback unambiguously conservative: write the shared sentinel, mark the current ID as 'retry', exit cleanly. No sleep, no retry attempt. The next run will honor the sentinel. Refs #18 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Pass-2 codex finding addressed in cba93b9: stock-pyoai HTTPError(429) fallback now writes sentinel + marks 'retry' + exits cleanly. No sleep, no retry attempt — keeps that behavior exclusively in the patched-fork path which already has the stop_after_this_record handling. |
rdhyee
commented
May 14, 2026
rdhyee
left a comment
rdhyee
left a comment
Contributor
Author
There was a problem hiding this comment.
LGTM from this review pass. The previous 429 fallback issue is resolved, the updated command compiles, and I do not see remaining blocking findings.
Residual caveat: tests and a small capped smoke run are still important before taking this out of draft / running at production scale.
- codex
The cron and the new backfill_doab command co-own the shared Retry-After sentinel file. Two latent races existed: - write_block_deadline() blindly overwrote: a short 429 racing a long one (or a stale writer) could shorten an active ban. Now monotonic - only ever moves the deadline later. - clear_sentinel() did read-then-unconditional os.remove(): a fresh future deadline written by another process between the read and the remove was erased, dropping an active ban (an unavoidable TOCTOU). It now NEVER deletes the file - an expired deadline is already harmless (every caller compares it to now) and the next 429 overwrites it. clear_sentinel() returns the still-active deadline (or None); all callers honor that. Mirror of the Gluejar/regluit fix; the two repos' sentinel helpers are kept consistent. Converged through 4 Codex review rounds (final: LGTM). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
One-off catch-up for the ~14.5k DOAB records missing from doab-check (measured independently of regluit's gap - the two services have different per-IP rate-limit histories). Divergence-aware port of the Gluejar/regluit backfill hardening: - transaction.atomic() KEPT here (unlike regluit): doab-check's add_by_doab -> load_doab_record is pure DB (get_or_create on Item/Timestamp/Link, zero HTTP), so the transaction is held for milliseconds across the multi-table write only - atomicity is cheap and correct here. The misleading "may make HTTP calls" comment copied from regluit was corrected. (regluit removed atomic() because its loader interleaves DB writes with minutes of HTTP per record.) - Second 429 after the courtesy wait now extends the shared sentinel. - Stock-pyoai 429 fallback parses Retry-After per RFC 9110 (delta-seconds OR HTTP-date) via a new local parser instead of bare int(). No discovery here (--ids-file required): doab-check#18's worklist is produced externally. Converged through 4 Codex review rounds (final: LGTM). Code-LGTM only: orchestration and tests are separate follow-ups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…18) Mirror of the Gluejar/regluit change. The orchestration runner that drains the full worklist must distinguish "drained" from "safe to re-invoke" from "stop, a human must inspect" — otherwise a naive cron loop defeats the in-command error_rate_halt / gone_rate_halt circuit breakers across ticks. exit 0 drained exit 3 benign checkpoint — runner re-fires next tick exit 4 circuit-breaker halt — runner freezes via .halted marker Every Retry-After SKIP path (step 6 sentinel ban, step 6 concurrent-writer ban) now sys.exit(3) instead of plain return. Without this they would exit 0 and the runner would write .done on the first ban, permanently freezing the backfill as "complete". doab-check has no discovery (--ids-file required), so the step-0 paths differ from regluit but the dispatch + skip semantics match exactly. Codex-reviewed end-to-end (orchestration round 3: LGTM). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation Adds the orchestration to actually drain the ~14.5k-record backfill (refs #18) without ever doubling the DOAB-OAI request rate on this host. - scripts/doab-backfill.sh: NEW. One bounded backfill_doab pass per cron tick. Honors the command's 0/3/4 exit-code contract via .done/.halted marker files (halt circuit-breaker honored ACROSS ticks, not just within one run). Holds /var/lock/doab-oai.lock via flock -n for the whole pass. Inert by default: hard-refuses unless the operator sets IDS_FILE — that opt-in IS the CROSS-HOST INVARIANT (regluit AWS + doab-check DO share the DOAB endpoint but the lock is host-local; only one host's backfill may be armed at a time; regluit drains first). Suggested crontab is in the script header (15,45 offset vs regluit's 0,30 as defense-in-depth). - scripts/doab_load.sh: PATCH. Wrap the existing nightly harvest in the same /var/lock/doab-oai.lock flock. Same-host serialisation guarantee with the new backfill runner: at most one DOAB-OAI client per host at any time. If the lock is held, the harvest tick skips; the 3-day rolling window self-heals on the next clear night (the script's own pre-existing comments already documented this resilience). Retry-After is still enforced independently by load_doab.handle(). No email/alerting added: operator treats cron mail as a signal — failure is expressed via .halted marker + the log. Codex-reviewed end-to-end (orchestration rounds 1-3, final: LGTM). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
2026-05-20 status update after cross-host snapshot testing:
Demo caveat: the same DOAB ID, |
Contributor
|
doab-check doesn't actually load the ebook, while regluit's does. (tries) |
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
2 participants
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.
Closes #18 once drained. Recovers the ~14.5k DOAB records missing from doab-check, measured independently of regluit's gap (per the no-extrapolate posture: per-IP rate-limit histories differ; the two services were measured separately).
What's in this PR (4 commits, bisectable)
c9221e2load_doab: monotonic + never-delete shared Retry-After sentinel (race-free). Mirror of the regluit fix; the two repos' sentinel helpers are kept consistent.f5a8ff0backfill_doab: Codex-hardened reliability. Divergence-aware port of the Gluejar/regluit work:transaction.atomic()is kept here because doab-check'sadd_by_doab → load_doab_recordis pure DB (get_or_create Item/Timestamp/Link, zero HTTP), so the transaction holds for milliseconds — atomicity is cheap and correct. (regluit removed atomic() because its loader interleaves DB writes with minutes of HTTP per record.) 2nd-429 sentinel extension + RFC-9110 stock-pyoai fallback parser.8f58b6fbackfill_doab: distinct exit codes0/3/4for halt-aware orchestration. Skip/rate-limit paths nowsys.exit(3)(not return-exit-0) so the runner does not falsely mark.doneon the first ban.4f00d80scripts/:doab-backfill.sh(the runner) +doab_load.shpatch (wrap the existing nightly harvest in the same/var/lock/doab-oai.lockflock — same-host serialization with the new backfill so they never hit DOAB OAI concurrently).Approval status
Code converged through 4 Codex review rounds + 3 orchestration code-review rounds (final: LGTM in both tracks).
clear_sentinel()return-contractclear_sentinel; Retry-After:0 → 60s endorsed as deliberate (over-conservative, slow-and-gentle posture)scripts/doab_load.shhad no flock → patched in commit4f00d80Divergence vs regluit (deliberate)
transaction.atomic()aroundadd_by_doab--ids-fileoptional)--ids-filerequired)This is exactly the kind of divergence the no-extrapolate review posture is meant to surface — Codex initially suggested mirroring regluit's atomic() removal here, which would have been wrong.
Cross-host invariant (read before arming)
The
/var/lock/doab-oai.lockflock is host-local. regluit (AWS) and doab-check (DigitalOcean) hit the same DOAB endpoint, so the lock does not serialise them across hosts. The doab-check runner is inert by default — hard-refuses unless the operator setsIDS_FILE— so the opt-in IS the cross-host serialisation gate. Operational posture: regluit drains first (.donemarker present), then doab-check is armed. Documented loudly inscripts/doab-backfill.sh.What's out of scope
backfill_doab— tracked for follow-up.Backlinks
🤖 Generated with Claude Code (Opus 4.7, 1M context) — multi-round Codex CLI adversarial review