Skip to content

fix(scraper): one chunk failure no longer aborts enrich batch (#47) - #53

Closed
nelsonmfinda wants to merge 1 commit into
deandevz:mainfrom
nelsonmfinda:fix-enrich-batch-abort
Closed

fix(scraper): one chunk failure no longer aborts enrich batch (#47)#53
nelsonmfinda wants to merge 1 commit into
deandevz:mainfrom
nelsonmfinda:fix-enrich-batch-abort

Conversation

@nelsonmfinda

@nelsonmfinda nelsonmfinda commented May 8, 2026

Copy link
Copy Markdown
Contributor

Summary

A single chunk's failure no longer aborts the entire concurrent enrich batch. asyncio.gather was called without return_exceptions=True, so the first non-transient ProviderError from _enrich_one cancelled every in-flight chunk and crashed king-scrape mid-pipeline. After the fix, _enrich_one is contract-bound never to raise (non-transient errors break the retry loop early and fall through to the schema fallback; everything else is absorbed and the chunk is dropped), and enrich_chunks adds return_exceptions=True as a defensive guard with a per-batch summary on stderr.

While in there: schema-fallback enrichments are no longer cached under the primary's cache key (was a silent quality regression — next run with the primary healthy would short-circuit at the cache check and serve fallback content), the schema fallback is deduplicated when it points at the same underlying client as the primary's FallbackClient inner fallback (saves one LLM call per failed chunk in that config), and the bare except Exception was narrowed so programming errors propagate instead of hiding as transient retries.

Related issue

Closes #47.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

How it was tested

pytest -q
739 passed, 1 skipped

16 tests in test_enrich.py (4 rewritten for the new "never raise per chunk" contract, 5 new):

  • The original repro: a non-transient ProviderError on one chunk in a batch of three; the bad chunk drops, the other two enrich, no exception escapes
  • Non-transient primary error falls through to the schema fallback (the layer designed to absorb malformed JSON, which previously never got a chance)
  • Cache invariant: a schema-fallback success does not write to the primary's cache key
  • gather with return_exceptions=True keeps siblings alive when one task raises, with a capsys assertion on the per-batch summary line
  • Per-chunk CancelledError returned by gather treated as a drop, not a batch abort
  • Existing 4 tests that previously asserted pytest.raises(ProviderError) now assert result == [] and verify call counts plus the new dedupe behaviour

I haven't run a live king-scrape against a 500+ chunk site in this PR. Happy to do that in review if you want; the unit tests cover the issue's repro precisely.

Checklist

  • My code follows the project style (English-only code, comments, and identifiers)
  • I ran the test suite locally and it passes (pytest)
  • I added or updated tests where it made sense
  • I updated the documentation in docs/ and/or README.md if behavior changed (no public-surface change beyond the user-visible warning lines on stderr; CHANGELOG entry added)
  • I read the Contributing guide and the Code of Conduct
  • My commits follow the project commit message style
  • I confirmed there are no secrets or credentials in the diff

@deandevz

Copy link
Copy Markdown
Owner

Best PR of the four, no concerns. Verified empirically: one non transient ProviderError in a five chunk batch leaves four enriched and one dropped, schema fallback success does not poison the primary cache (zero writes when only the fallback succeeded), PROMPT_VERSION derived from sha256(prompt)[:16] invalidates cache on any prompt edit. 122 scraper tests pass.

One thing worth flagging in your description: this is a hard dependency of #51's UpdateReport.lost contract. Without this fix, any non transient error during king-scrape update would crash the pipeline instead of being counted as lost. Wrote it up in #55 along with consolidated feedback on the other three PRs.

@nelsonmfinda nelsonmfinda mentioned this pull request May 14, 2026
10 tasks
@deandevz

Copy link
Copy Markdown
Owner

Superseded by #56, merged as ae1f941. The per-chunk failure
isolation, cache invariant, and FallbackClient dedup all
landed verbatim in the unified stack. Thanks for the
original work, closing as superseded.

@deandevz deandevz closed this May 14, 2026
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.

[Bug] Non transient enrich error aborts entire batch

2 participants