Skip to content

feat(pdf): add a remote circuit breaker and generalize provider dispatch - #1

Open
q2000s wants to merge 1 commit into
feat/mineru-first-strategy-standalonefrom
feat/pdf-remote-resilience
Open

q2000s wants to merge 1 commit into
feat/mineru-first-strategy-standalonefrom
feat/pdf-remote-resilience

Conversation

@q2000s

@q2000s q2000s commented Oct 15, 2026

Copy link
Copy Markdown
Owner

Summary

Remote resilience and provider generalization for the PDF parser, split out of volcengine#5967
so that PR can stay scoped to the dispatch behaviour change.

Stacked on volcengine#5967 (upstream: volcengine/OpenViking#5967). The base is
feat/mineru-first-strategy-standalone rather than main: the breaker only earns its
keep under remote-first dispatch, which is what volcengine#5967 introduces. On main the remote
parser is a local-first fallback, so a remote outage never produces the per-document
cost this is meant to remove. Review and merge after volcengine#5967.

This PR lives in the fork because its base branch lives here; the diff is exactly one
commit.

Why this is separate

Review feedback on volcengine#5967 suggested a circuit breaker and a provider abstraction. Both
are worth having, but neither is a prerequisite for the dispatch change, and folding
them in made that PR hard to review -- the reviewer judged the result to be "changed
too much". This PR carries them so volcengine#5967 can be judged on its own.

Remote circuit breaker

  • A provider that fails 3 times in a row is circuit-broken; the rest of the batch
    skips it instead of paying for a doomed call per document.
  • After a 20 second cooldown exactly one document is let through as a probe, so a
    recovered service is picked up again and a service that is still down costs a single
    call rather than the whole batch.
  • A success clears all of that provider's breaker state.
class PDFParser:
    _remote_circuit_threshold = 3      # consecutive failures that open the circuit
    _remote_circuit_cooldown = 20.0    # seconds before one probe is let through

The probe-failure regression

The first attempt at this reset into a per-document retry. With a threshold of 1 and a
cooldown of 3 documents, sustained failure produced the remote-call sequence
[1, 0, 0, 1, 1, 1] -- a call per document again, which is exactly what the breaker
exists to prevent.

_remote_record_failure now separates the two cases. A failure while the circuit is
closed counts toward the threshold. A failure from a half-open probe re-arms the
cooldown from that moment, so the following documents are skipped again.

test_a_failing_probe_rearms_the_cooldown_instead_of_retrying_every_document asserts
this directly, including that no remote call happens while the circuit is armed.

Observability

pdf_remote_degraded counts the documents that fell back, whether the remote failed
outright or the breaker kept them off it.

Provider dispatch

Providers are an ordered list of (name, converter) pairs from _remote_converters(),
so registering another remote parser is a provider entry rather than a new strategy
value.

Verification

  • 6 new breaker tests, including the probe-failure regression and an explicit assertion
    on the remote call sequence
  • 77 tests pass across test_pdf_strategy_dispatch.py and test_pdf_bookmark_extraction.py
  • ruff check clean

Related

…atch

Split out of volcengine#5967 so that PR can stay scoped to the dispatch behaviour change.
Reviewed against the feedback that suggested a breaker and a provider
abstraction as prerequisites: those are worth having, but they are not what the
dispatch change needs, and folding them in made that PR hard to review.

Remote resilience:
- A provider that fails 3 times in a row is circuit-broken; the rest of the
  batch skips it instead of paying for a doomed call per document.
- After a 20s cooldown exactly one document is let through as a probe, so a
  recovered service is picked up again and a service that is still down costs a
  single call rather than the batch.
- A failing probe re-arms the cooldown. It does not reset into a per-document
  retry, which is what the previous attempt did: with a threshold of 1 and a
  3-document cooldown, sustained failure produced [1,0,0,1,1,1].
- A success clears all of that provider's breaker state.

Observability:
- pdf_remote_degraded counts documents that fell back, whether the remote
  failed outright or the breaker kept them off it.

Provider dispatch is expressed as an ordered list of (name, converter) pairs, so
another remote parser is a provider entry rather than a new strategy value.

Tests: 6 breaker tests including the probe-failure regression, and an explicit
assertion on the call sequence. 77 pass across the dispatch and bookmark suites.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants