Add backfill_doab management command (refs #1151) - #1152
Draft
rdhyee wants to merge 7 commits into
Draft
Conversation
Slow, resumable, observable per-ID backfill for the 20,608 active DOAB records found missing from the local DB by the 2026-05-14 set-diff (#1151). The 4:30 UTC nightly cron's 3-day rolling window cannot self-heal records lost during prior 429 stretches; this command reads a static list of missing IDs and processes them via add_by_doab() with rate-limited, ID-keyed JSON state. Design highlights (codex-reviewed twice): - Reuses add_by_doab() and the shared Retry-After sentinel from load_doab.py so a 429 hit by either command suppresses the other. - 4-way per-record outcome taxonomy: ok / gone / present_locally / error_review. Unknown exceptions HALT (do not silently mark recovered). Known validation errors are terminal but flagged. - Local existence precheck per record short-circuits DOAB calls for IDs the nightly cron may have caught up on during the run. - transaction.atomic() scoped to local writes only — remote OAI fetch happens before the DB transaction opens, not inside it. - IntegrityError on write triggers re-precheck and promotion to present_locally (handles race with nightly cron). - Numeric ID sort, not lexical. Newest-first by default. - Per-record atomic state writes (tmp + os.replace). 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. No tests in this commit (followup); no cron entry yet (lives in regluit-provisioning, separate change). Snapshot generator for doab_missing.txt is also a follow-up. Refs #1151 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
Member
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. -
Medium: the transaction still wraps remote work. The command fetches the OAI record before
transaction.atomic(), butdoab.add_by_doab()can still make cover/bitstream HTTP calls viaupdate_cover_doab()/store_doab_cover()/get_streamdata(). That means the DB transaction can be held during HTTP despite the design note. Either move those remote side effects out of the atomic block, add a loader path that skips cover fetching during backfill, or narrow the transaction around only the actual DB writes. -
Medium: deleted OAI records are not categorized as
gone. A deleted/no-content record gets toadd_by_doab(), returnsNone, and the command marks iterror_review. That inflates review errors and bypasses the intended gone-rate halt. The command should detect deleted/no-content OAI headers before callingadd_by_doab()and mark those asgone.
I did not run tests; this was a code review against the loader contracts and the PR diff.
Three of the four codex high+medium findings on PR #1152: 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. MEDIUM: deleted OAI records were inconsistently categorized. A record with status="deleted" header (or empty content) reaches add_by_doab(), which returns None when record[1] is falsy — and that None reached step 7f and was marked error_review instead of gone. Fix: in step 7d, after fetch but before add_by_doab(), check record[0].isDeleted() or not record[1] and mark gone directly. 3. MEDIUM: transaction.atomic() framing was misleading. The original comment claimed the DB tx was "scoped to local writes only" but add_by_doab() makes downstream HTTP calls (cover image fetch, bitstream API path) — so the tx actually does hold during HTTP. Fix: replace the misleading comment with an honest trade-off note. Refactoring add_by_doab to support fetch-vs-write phase separation is out of scope for this PR. The 4th finding (status flip) applies only to doab-check. Refs #1151 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Addressed codex review findings in commit e103d02:
The 4th codex finding (status flip) only applies to doab-check; addressed in EbookFoundation/doab-check#19. |
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 requirements_versioned.pip) 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 #1151 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Pass-2 codex finding addressed in 3c14882: 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
Member
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 and let us hit a still- banned endpoint early. 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. Strictly safer for the production nightly harvester: a ban can now only be extended, never shortened or wiped by a racing process. 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 ~20.6k active DOAB records the nightly load_doab cron structurally cannot self-heal (3-day rolling window can't recover records modified during a 429 ban). - Worklist auto-discovery (Option B): crawl DOAB OAI ListIdentifiers, diff vs local Identifier rows, write SHA-pinned .ids/.stale/.orphans; a namespace-drift guard refuses to write a worklist if headers were seen but zero DOAB IDs recognized. - Removed transaction.atomic() around add_by_doab: that loader interleaves DB writes with up to ~3 min of HTTP per record (online_to_download + cover-fetch redirect chain). An atomic() wrapper held an RDS transaction open across minutes of network I/O ~20.6k times. The production nightly cron has called add_by_doab with no transaction wrapper for years; autocommit + add_by_doab idempotency is the loader's established model. - Precheck no longer freezes a partially-committed load as 'present_locally': only never-attempted IDs short-circuit; previously- attempted IDs re-run the idempotent loader to heal. - 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) instead of bare int(). - Removed dead parse_id_line. Converged through 4 Codex review rounds (final: LGTM). Code-LGTM only: orchestration to drain the full worklist and tests are separate follow-ups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…1151) The 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 (or discovery-found-zero) exit 3 benign checkpoint — runner re-fires next tick (max_remote_calls, 429_recovered_*, retry_after_too_long, transient_failure, network_failure, stock_pyoai_429_fallback, 429_after_wait_still_failing) exit 4 circuit-breaker halt — runner freezes via .halted marker (error_rate_halt, gone_rate_halt, unknown_http_error_halt, unknown_exception_halt) Also: every Retry-After SKIP path (step 0 discovery ban, 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". Codex-reviewed end-to-end (orchestration round 3: LGTM). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…1151) Today each regluit host (test/dj42/prod) would run its own ~17-min DOAB OAI ListIdentifiers crawl during discovery. The active set DOAB exposes is universal — the only host-specific part is the diff against the local DB — so we can amortize the one expensive operation across the fleet. Producer side: discover_missing_ids() now emits two new artifacts alongside the existing per-host worklist/stale/orphan files: .active — raw OAI active set (~125k IDs), sorted by numeric suffix .deleted — raw OAI deleted set, sorted Consumer side: two new flags, --use-active-file / --use-deleted-file. When provided, a new discover_via_active_file() reads the precomputed snapshot from disk and diffs against the local Identifier table without making any DOAB OAI calls. The per-record load loop downstream is unchanged; the shared Retry-After sentinel still gates DOAB traffic the moment loading begins. Result: one OAI crawl serves all three regluit hosts. doab-check (separate codebase) can consume the same .active file format with its own diff in a follow-up. Codex review (4 rounds) hardened: - Refuse silently shadowing --use-active-file with a stale local cache; operator must be explicit (--refresh-ids + fresh --state-file). - Mutual-exclusion guard: --use-active-file vs --ids-file at the top of handle(); --use-deleted-file requires --use-active-file. - Producer/consumer ID validation drift fixed: discovery now validates the stripped DOAB ID against ID_RE and refuses to emit a snapshot the consumer's load_input_ids() would reject (catches DOAB OAI namespace drift early, paired with the existing 0-recognized namespace-drift guard). - Multi-file atomic-write ordering: .ids is written LAST in both producer and consumer paths, so its presence as the cache_exists marker proves all sibling artifacts are fresh and mutually consistent. - --dry-run --use-active-file no longer writes any files (was seeding a real cache from a validation run); the diff is still computed and reported. - Mismatched-snapshot-pair guard: active ∩ deleted must be empty (by construction from a single crawl); if --use-deleted-file's set overlaps --use-active-file's, refuse — the operator paired files from different crawls. Tests deferred per existing #1151 scope (see ORCHESTRATION_PLAN). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
2026-05-20 status update after the cross-host snapshot work:
Operational caveat surfaced by the demo: DOAB ID |
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
1 participant
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 #1151 once drained. Recovers the ~20.6k active DOAB records the nightly
load_doabcron structurally cannot self-heal (3-day rolling window can't reach records modified during a 429 ban).What's in this PR (3 commits, bisectable)
2115772aload_doab: make the shared Retry-After sentinel monotonic + race-free (never-delete). A ban can now only be extended, never shortened or wiped by a racing process. Used by both the nightly harvest and the new backfill.88480de9backfill_doab: auto-discovery (Option B) + Codex-hardened reliability — removedtransaction.atomic()aroundadd_by_doab(the loader interleaves DB writes with up to ~3 min of HTTP per record; the nightly cron has run without an atomic wrapper for years). Precheck no longer freezes partial commits; 2nd 429 extends the sentinel; stock-pyoai fallback parses RFC 9110 Retry-After.2ff6226bbackfill_doab: distinct exit codes0/3/4so the orchestration runner (separate PR EbookFoundation/regluit-provisioning#TBD) can honor the in-commanderror_rate_halt/gone_rate_haltcircuit breakers ACROSS ticks (a naive re-run loop would defeat them). Every Retry-After SKIP path nowsys.exit(3)so the runner does not falsely mark.doneon the first ban.Approval status
Code converged through 4 Codex review rounds + 3 orchestration code-review rounds (final: LGTM in both tracks). High-confidence on the code itself; the design calls below want human eyes (see "For Eric to weigh" — see the linked operator notes).
Codex code-review trail (this PR + its
load_doabdependency)int()(mishandles HTTP-date), shared-sentinel races, discovery namespace-drift silently emptyclear_sentinel()safe, callers ignored refused-clear → fixed: returns live deadline, 5 call sites honor itRetry-After: 0→ 60s non-fix endorsed (over-conservative, aligns with slow-and-gentle); root TOCTOU onclear_sentinel'sos.remove→ fixed per Codex's own prescription: never delete (an expired sentinel is harmless; next 429 overwrites; monotonic write guarantees correctness)Design calls that deserve maintainer eyes (not Codex's domain)
Removing
transaction.atomic()aroundadd_by_doabin the regluit loader. Justified by the loader's HTTP-in-DB-transaction problem (cover fetch up to ~120s) + the established prod cron pattern (load_doab_oaihas never wrappedadd_by_doabin a transaction). The precheck change (commit88480de9) depends onadd_by_doab's idempotency to heal any partial commits — please sanity-check that idempotency claim against the cover/metadata/ISBN follow-up code paths.load_doab.pysentinel change (commit2115772a) touches the live nightly harvester's shared helpers. The change is strictly safer (bans can only be extended; noos.removerace), but it is production-cron-affecting and so wants a careful read.What this enables but does not deliver
feature/doab-backfill-orchestration) — adds the runner script under sharedflockwith the nightly harvest, marker-file circuit breaker,deploy_type == 'prod'-gated cron. The exit-code change in this PR is what makes that runner safe.backfill_doabitself: deliberately out of scope for this PR (the command is straightforward to exercise by hand; the design budget went to Codex hardening + the orchestration). Tracked for follow-up.Backlinks
🤖 Generated with Claude Code (Opus 4.7, 1M context) — multi-round Codex CLI adversarial review