fix(etl-uvicorn): declare a rate limit retryable and normalize plugin_error.error_reason - #79
paulkarayan wants to merge 4 commits into
Conversation
…wire plugin_error_of declared retryable = false for the whole legacy UserError family, RateLimitError included, so a consumer that re-dispatches off the declared field fails a record on the first 429 instead of backing off -- and charges the customer for a condition that would have cleared on its own. It also typed the 429 as error_type = "configuration", which reads as "the customer misconfigured something" rather than "an external dependency throttled us", and error.type feeds the err-by-type SLI. RateLimitError now serializes dependency / rate_limited / user / retryable = true, mirroring utic_plugin_base.errors.RateLimitError field for field (platform-libs #889 fixed the identical bug in that class). Every other member of the family is unchanged: terminal stays the safe default for a failure nothing has classified, so a transient condition declares itself. Audience stays user for the 429. Retryability is orthogonal to audience -- it is still the caller's provider quota. No consumer behaviour changes today. plugins_controller decides retryability from the HTTP status band (is_retryable_http_status returns true for 429) and reads only plugin_error.audience off the envelope, which is exactly why the wrong wire value survived this long. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…_reason error_reason was assigned straight from the raised error's failure_category. Those are two vocabularies, not one: the preflight failure categories are SCREAMING_SNAKE (AUTH_PERMISSION_DENIED, PROVIDER_RATE_LIMITED -- see utic_auth.precheck, which maps each one onto a SEPARATE lower_snake_case error_reason), while error.reason is specified lower_snake_case. Any plugin setting a failure_category on a UserError put the wrong spelling on the wire. failure_category is arbitrary plugin-supplied text rather than a closed enum, so lowercasing alone would not yield a snake_case token. Punctuation and whitespace runs collapse to single underscores, and a category with no usable characters falls back to the default reason instead of emitting garbage. The top-level failure_category response field still carries the original verbatim, so nothing is lost -- only the copy that lands in error_reason is normalized. Wire-format change, and safe on the read side. No consumer constrains or branches on this value: SERVICE_ERROR_SCHEMA types error_reason as a bare string with no enum and utic_plugin_base only isinstance-checks it; plugins_controller reads only plugin_error.audience; check_executioner reads only the nested error_type; platform-api has no reference to plugin_error at all. The one reader of the value, platform-plugins' local_plugin_tester, displays it in a dev report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…branch An independent review read the new rate-limit branch as promising a fixed error_reason of "rate_limited" and flagged that an explicit failure_category still overrides it. The precedence is deliberate and unchanged -- a plugin that declares a category has said something more specific than the class did, and the whole UserError family has always let it win -- but nothing said so, and the CHANGELOG asserted the reason flatly rather than as a default. No behaviour change. The CHANGELOG now calls "rate_limited" a default and names the override, the docstring explains why the more specific declaration wins, and a test pins the precedence with PROVIDER_RATE_LIMITED so a future reader sees a decision rather than an accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Repro-first proofBugfix — plugin_error_of: stop marking a provider 429 terminal, and stop putting the SCREAMING_SNAKE failure_category vocabulary in the lower_snake_case error.reason field Ticket: Reproduced the broken state
Probe: serialize the legacy unstructured_ingest.error family through the wrapper's own
Failing test (red)
Fix
Proof it's resolved
BEFORE (origin/main d119d8e) AFTER (this branch) Suite, before and after: Auto-generated from this branch's |
…o 0.1.0 consumers
There was a problem hiding this comment.
1 issue found across 4 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="unstructured_platform_plugins/etl_uvicorn/api_generator.py">
<violation number="1" location="unstructured_platform_plugins/etl_uvicorn/api_generator.py:129">
P2: When a plugin supplies a `str` subclass whose `lower()` raises, `_as_error_reason` escapes while building the sanitized error response and produces a raw 500. Invoke the base `str.lower` before normalization.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| """ | ||
| if category is None: | ||
| return None | ||
| reason = re.sub(r"[^a-z0-9]+", "_", category.lower()).strip("_") |
There was a problem hiding this comment.
P2: When a plugin supplies a str subclass whose lower() raises, _as_error_reason escapes while building the sanitized error response and produces a raw 500. Invoke the base str.lower before normalization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unstructured_platform_plugins/etl_uvicorn/api_generator.py, line 129:
<comment>When a plugin supplies a `str` subclass whose `lower()` raises, `_as_error_reason` escapes while building the sanitized error response and produces a raw 500. Invoke the base `str.lower` before normalization.</comment>
<file context>
@@ -111,19 +112,52 @@ def failure_category_of(error: BaseException) -> Optional[str]:
+ """
+ if category is None:
+ return None
+ reason = re.sub(r"[^a-z0-9]+", "_", category.lower()).strip("_")
+ return reason or None
+
</file context>
| reason = re.sub(r"[^a-z0-9]+", "_", category.lower()).strip("_") | |
| reason = re.sub(r"[^a-z0-9]+", "_", str.lower(category)).strip("_") |
What & why
Problem: A consumer that trusts this library's
retryableflag is told a provider rate limit is permanent, which is the one failure whose entire meaning is "try again later".plugin_error_ofhard-codedretryable = falseANDerror_type = "configuration"for the whole legacyUserErrorfamily;RateLimitErrorcarriesstatus_code = 429and inherited both, so a throttled request serialized asconfiguration / invalid_input / user / retryable = false-- terminal, and typed as caller misconfiguration rather than a dependency failure. The taxonomy this envelope implements says the opposite on both counts: a 429 from the caller's own provider is user-audience and retryable, anderror.type,audienceandretryableanswer three independent questions, so retryability is orthogonal to audience rather than derivable from it. Separately,plugin_error.error_reasonwas assigned straight from the raised error'sfailure_category, so any plugin that set one put a SCREAMING_SNAKE token (AUTH_PERMISSION_DENIED) on a field the same taxonomy specifies as lower_snake_case. These are two distinct vocabularies and the platform already treats them as such elsewhere -- a sibling module maps each SCREAMING_SNAKE category onto its own distinct lower_snake reason -- and this library collapsed them into one. Both defects are this library mis-declaring the error contract it publishes to every plugin that imports it.Change:
RateLimitErrornow serializeserror_type = "dependency",audience = "user",retryable = trueand anerror_reasondefaulting torate_limited; every other member of the family keepsconfiguration / invalid_input / user / retryable = false, because terminal is the safe default and the transient condition is the one that has to declare itself.error_reasonis now normalized to a lower_snake_case token rather than copied verbatim, and the top-levelfailure_categoryresponse field still carries the plugin's original string unchanged.Known gap left alone
For any error that is not a
UserError,plugin_error_ofreturnsNoneand emits no envelope at all, soaudienceis absent rather than merely wrong, and a plugin has no way to declare a non-user audience. That is deliberately untouched here. The repository's own test pins the current behaviour, and changing it would add a new plugin-facing contract surface -- a design decision about what a plugin may declare, not a bug fix. Calling it out so it is acknowledged rather than discovered.Risk / rollback
Low. A pure mapping function with no state, no migration, and no configuration. Back it out by reverting the PR, or by pinning consumers to 0.1.0.
How it was verified
Ran the library's own suite (
make check-ruffclean on the repo's pinned ruff; full pytest run green, six new tests added and no existing test modified or deleted). Wrote a probe that serializes the wholeunstructured_ingest.errorfamily throughplugin_error_ofand prints every field a consumer reads off the wire, ran it against the base revision and against this branch, and diffed the two -- output below. The consumer enumeration above was checked against each repository'sorigin/main, not a local checkout. Not verified: no deployed environment was exercised, because this package is the wrapper library a plugin imports rather than a deployed service, so a real local run of the wrapper plus its suite is the closest real environment. The retryability flip in particular cannot be proven end to end from a consumer's behaviour today, since nothing reads the field yet.Proof
Repro (on the base revision), same probe both times:
A 429 reached the wire terminal and typed as a misconfiguration, and a plugin-supplied
AUTH_PERMISSION_DENIEDlanded verbatim on a lower_snake_case field.Failing test before the fix:
After the fix, same probe and same command. Only the two rows the change targets moved:
The four rows that should not move are byte-identical, and
ProviderErrorstill emits no envelope -- the gap described above, reported and deliberately not fixed here.Why normalize rather than lowercase:
failure_categoryis arbitrary plugin-supplied text validated only as a string, so.lower()on"Auth Permission-Denied "yields a space rather than a snake_case token. Punctuation and whitespace runs collapse to single underscores, and a category with no usable characters ("///") falls back to the class default instead of putting garbage on the wire. Both cases are pinned by tests.Dependencies / merge order
none