Skip to content

FLUX-511 - Hydrate Oxylabs content that reports no parse_status_code - #11

Merged
qschmick merged 1 commit into
mainfrom
fix/FLUX-511-content-hydration
Sep 9, 2026
Merged

qschmick merged 1 commit into
mainfrom
fix/FLUX-511-content-hydration

Conversation

@qschmick

@qschmick qschmick commented Sep 3, 2026

Copy link
Copy Markdown
Member

FLUX-511

1. Impact

Production pec-platform failed_jobs carries 7,116,382 rows of this single signature, ~104,727/day, failing continuously since 2025-10-24, on queue oxylabs-insights-retrieve (~1% of daily volume, burst pattern):

TypeError: AlwaysOpen\OxylabsApi\DTOs\Amazon\AmazonProductResult::__construct():
Argument #1 ($content) must be of type
AlwaysOpen\OxylabsApi\DTOs\Amazon\AmazonProductResultContent|string, array given,
called in vendor/spatie/laravel-data/src/Resolvers/DataFromArrayResolver.php:104

Each failure sits inside retry(5, …) in pec's retrieve command, so this is roughly 520k wasted Oxylabs result fetches/day. Worse: TypeError is an Error, not an Exception, so it escaped pec's outer catch (Exception $e) and killed the job before storeInsightsDataToBlob() — we have archived zero raw payloads for this cohort since 2025-10-24.

2. Mechanism

AmazonProductResult::$content is declared AmazonProductResultContent|string. spatie's DataTypeFactory::inferPropertiesForCombinationType() classifies that union as DataObject (left-to-right $kind ??=), so nested hydration is attempted. It fails inside DataFromArrayResolver::createData() with ArgumentCountError → CannotCreateData::constructorMissingParameters, because the promoted int $parse_status_code had neither a value in the payload nor a default. Then CastPropertiesDataPipe::cast() swallows it:

} catch (CannotCreateData $exception) {
    if ($property->type->type instanceof CombinationType) {
        // Try another type in the union (which will need to be a simple type like string, int)
        return $value;                // <-- the raw ARRAY leaks out here
    }
    throw $exception;
}

→ new AmazonProductResult(content: [...]) → the ticket's TypeError.

Root cause: parse_status_code is a field of a response the Oxylabs parser actually produced, not of every Oxylabs response. When the parser never ran — bot-check / CAPTCHA / error-page classes, unsupported page types, raw type=html retrievals — Oxylabs omits the key entirely, and the DTO made that payload class unrepresentable. Strictness that cannot reject cleanly is not strictness; it is a crash.

3. Fix — and why parser_type is part of it, not scope creep

  • parse_status_code → ?int (default null) on AmazonProductResultContent, WalmartProductResultContent, GoogleShoppingProductResultContent, GoogleShoppingPricingResultContent, AmazonSellerResultContent.
  • parser_type → ?string (default null) on AmazonProductResult, GoogleShoppingProductResult, GoogleShoppingPricingResult — matching AmazonResult / AmazonPricingResult / WalmartProductResult / eBayResult, which are already ?string = null.

The second half is load-bearing. Real unparsed Oxylabs responses drop parser_type too (tests/Fixtures/amazon_response_no_parse.json and amazon_pricing_png_result.json have no such key). Verified: with only the parse_status_code fix applied, a payload missing parser_type throws

CannotCreateData: … AmazonProductResult … requires 10 parameters, 9 given.
Parameters missing: parser_type.

which extends Exception — so unlike today's TypeError it is caught by pec's outer catch (Exception $e) → reattempt() → no failed_jobs row and no blob archive. Fixing only parse_status_code would let a failed_jobs-count-based verification read green while part of the cohort silently converted to invisible retry churn. A flat TypeError count is therefore not sufficient proof of fix (see gates below).

Corollary that shaped the fixtures: because the 7.1M prod rows are Argument #1 ($content) … array given, the outer constructor was reached, so the prod cohort has parser_type present and parse_status_code absent. The primary regression fixture reproduces exactly that (parser_type: ""); parser_type-absent is covered as a separate payload class by its own test.

Zero pec impact from the parser_type widening: grep -rn parser_type pec/app returns nothing.

4. Semantics — three states stay three states

Added ParseStatus::NOT_REPORTED = 0 and hasParseStatus(): bool; getParseStatusCode() keeps its exact non-nullable ParseStatusEnum signature.

state parse_status_code hasParseStatus() getParseStatusCode()
vendor reported a status we model e.g. 12000 true that case, e.g. ParseStatus::SUCCESS
vendor reported a status we do not model yet e.g. 12001 true ParseStatus::UNKNOWN
vendor reported no status at all null false ParseStatus::NOT_REPORTED

NOT_REPORTED = 0 sits deliberately outside the vendor's 120xx numbering space: it is our statement about a missing field, never a code Oxylabs sent. It is explicitly not conflated with the vendor's own 12007 UNKNOWN. parse_status_code itself stays null on the DTO, so the blob archive and the toJson() round trip stay faithful and reconcilable.

success() needed no change on any of the five content DTOs — NOT_REPORTED !== SUCCESS, so it correctly returns false.

5. Blast radius: the trait is shared by 6 content DTOs

from() → tryFrom() in Traits/ParseStatus also affects Universal, Walmart, both Google Shopping and Amazon-seller content. This is a second live bugfix: the enum genuinely skips 12001 (it has 12000, 12002…12009), so ParseStatus::from() was throwing ValueError — an Error, uncatchable by pec's catch (Exception) — for any unmodelled code, on all six. Those DTOs now return ParseStatus::UNKNOWN instead of throwing. Stated here so nobody attributes a future Walmart/Google classification change to the wrong commit.

6. Behavioural delta, stated explicitly

The |string raw-HTML path is unchanged and pinned by tests: '<html>…' stays a string (test 8), '' stays '' (test 9), and AmazonProductResult with string content stays a string (test 10 — the suite had no string-content test for the exact class the prod error names).

One real delta: a string containing JSON object/array syntax ('{}', '[]', '{"asin":"B01"}') now hydrates into a content object instead of falling through to string, because every array-shaped payload can now satisfy the content DTO. Oxylabs does not emit this shape for type: parsed. Test 11 pins it so it is a decision, not a surprise.

Also new: loud failure becomes representable data — {}, [], {"unrecognised":"keys"} all now produce a well-formed all-null content object. Consumers must guard on success() / hasParseStatus(), never on instanceof (now satisfied by an all-null object) and never on empty() (always false for an object). This is documented in the new README section.

7. Gates

gate before (main) after
composer test (pest --no-coverage) 37 passed 52 passed / 117 assertions
new tests run against unpatched src 12 failed (exact prod TypeError + CannotCreateData) 0 failed
composer analyse (phpstan) Found 4 errors Found 4 errors — byte-identical, zero new, zero baseline entries added
vendor/bin/pint --test PASS PASS (142 files)

The 4 PHPStan errors are pre-existing baseline drift on main (3 unmatched ignoreErrors patterns for src/OxylabsApiClient.php + 1 function.alreadyNarrowedType in src/Traits/Renderable.php) and are intentionally left untouched, so that "analysis-neutral" is verifiable for this change. .github/workflows/phpstan.yml runs on every **.php push, so that workflow is presumably already red on main — worth its own PR (follow-up 12).

The fix depends on spatie's DefaultValuesDataPipe injecting null for missing nullable properties before CastPropertiesDataPipe runs — behaviour present throughout spatie/laravel-data 4.x, not a pinned patch level. (Verified against 4.17.0; composer.lock is gitignored and CI runs composer update --prefer-stable on PHP 8.3 / Laravel 11.)

Also added tests/Feature/*.png to .gitignore: the existing suite writes tests/Feature/7350883412053343233.png and …_walmart.png, which were untracked and un-ignored.

8. Scope discipline

Reflection audit of required (non-defaulted, non-nullable) constructor params after this change:

content DTO params still required after fix verdict
AmazonProductResultContent 65 none fully closed
WalmartProductResultContent 13 none fully closed by one word
GoogleShoppingProductResultContent 12 none fully closed
GoogleShoppingPricingResultContent 7 none fully closed
UniversalResultContent 1 none (already) no change needed
AmazonPricingResultContent 9 url, page, title, asin_in_url, review_count follow-up
AmazonSellerResultContent 12 url, query, page_type, description relaxed for trait consistency only, not claimed fixed
eBayResultContent 12 10 fields follow-up

9. Release

Tags here are manual and there is no release workflow — merging this PR ships nothing. Green CI is not a fix.

gh release create v0.8.8 --generate-notes --target main

v0.8.8 is honest: no BC break (getParseStatusCode(): ParseStatusEnum keeps its exact signature, the enum case is additive, property types only widen), so pec's existing ^0.8.5 constraint (locked at v0.8.7) picks it up with no constraint bump.

10. Merge / close gates — do not skip

  1. GROUP BY the prod failed_jobs exception signature before closing FLUX-511. If the 7.1M rows are not ~100% Argument #1 ($content) … array given, this PR does not close the ticket. Two verified sibling crash classes are not fixed by anything here: content: null → TypeError … null given (spatie skips casting on null, so raw null reaches the non-nullable union), and a scalar-type mismatch inside content, e.g. {"parse_status_code":12000,"rating":"N/A"} → TypeError: AmazonProductResultContent::__construct(): Argument #12 ($rating) must be of type ?float, string given, thrown inside $context->from() where only CannotCreateData is caught. Note HEAD is literally FLUX-432 - Type AmazonProductResultContent::$rating as float, not int, so that class has already bitten once. Do not speculatively widen the outer union — get evidence first.
  2. Fixture upgrade (cheap, recommended): pull one real payload for this signature out of prod failed_jobs and use it verbatim in place of the synthetic fixture. That single step settles gate 1, whether parser_type was present, and array given vs null given. Seven million real examples exist.
  3. Post-deploy verification is two signals, not one: (a) failed_jobs growth on the AmazonProductResult::__construct signature goes flat, and (b) new blobs appear under insights/oxylabs/amazon_product/<Y/m/d/H>/ for the previously-lost cohort. Per §3, (a) alone can read green while the cohort churns invisibly. Secondary signal: oxylabs-insights-retrieve queue latency/CPU should drop as the 5× retry storm disappears.

11. Follow-ups — listed, deliberately not implemented here

pec-platform (paired PR, own FLUX ticket, immediately after the SDK release):

  1. composer update always-open/oxylabs-api → v0.8.8. No constraint bump. Merging this SDK PR alone deploys nothing.
  2. Classification is now wrong-by-default in 3 non-null-safe sites. Insights/RetrieveAmazonProductResults.php:178, RetrieveAmazonProductListingResult.php:253, RetrieveAmazonPriceOffersResult.php:219 all do ->content->parse_status_code === ParseStatus::NOT_SUPPORTED->value with no ?->. null === 12003 → false, and the preceding empty($response->results[0]->content) guard is now false for any object, so these payloads fall through to MEASUREMENT_REQUEST_FAULTED + reattempt() — the 7.1M failed_jobs rows convert to bounded retry churn, not a clean terminal state. Add one shared helper (e.g. isUnparsedContent(?object $content): bool) and route to MEASUREMENT_CONTENT_NOT_FOUND + failedExternally(). Key it on hasParseStatus() / success(), never instanceof, never empty().
  3. validateResult() null-content early return. RetrieveAmazonProductListingResult.php:315 calls MarketplaceProduct::validAsin($content->asin); validAsin(string $asin) is non-nullable, and an all-null content passes the preceding asin !== asin_in_url check (null !== null → false). The call sits in an array_filter inside try { … } finally { … } with no catch, so it propagates out of handle(). Different queue from FLUX-511, so not a blocker for the fire — but it is a new crash the moment a null-status listing payload lands.
  4. processAmazonProductResult (386-430) / createOfferData (590) / getBuyboxCondition (711) now receive all-null content objects where they previously never ran. Buybox-suppression keys off price_buybox == -1 plus an empty featured merchant — both trivially true for an empty content object. Reject null-status content before this logic.
  5. Product decision: NOT_REPORTED is in none of the three in_array($content->getParseStatusCode(), [FAILURE_COULD_NOT_PARSE, NOT_SUPPORTED, FAILURE_PRODUCT_NOT_FOUND]) lists (RetrieveAmazonProductListingResult.php:423, RetrieveWalmartProductListingResult.php:264, RetrieveWalmartProductOfferResult.php:477), so a null status will not map to Seller::PRODUCT_NOT_AVAILABLE. Decide deliberately whether it should.
  6. Observability: emit a distinct parse_status=absent metric tag. We are about to remove the crash that was accidentally serving as this cohort's alert.
  7. Verified inert, no action: RetrieveWalmartProductListingResult.php:120,122, RetrieveWalmartProductOfferResult.php:135,137, RetrieveWalmartProductOfferUrlResult.php:105, RetrieveGoogleShoppingPricingOfferResult.php:140 are already ?->… ?? null and already treat non-SUCCESS as faulted. RetrieveWalmartProductOfferResult.php:373 constructs content with named arg parse_status_code: SUCCESS->value — still valid. There is no match on ParseStatus anywhere in pec (grepped), so the additive enum case is safe. All 8 pec tests that fabricate parse_status_code pass 12000 explicitly and keep passing. Re-run pec's PHPStan after the bump: AmazonProductResultContent::$parse_status_code widens int → ?int — safe for === against ->value, but will surface if it is ever passed into an int parameter.

oxylabs-api SDK (separate ticket):

  1. Relax AmazonPricingResultContent (also needs url, page, title, asin_in_url, review_count), AmazonSellerResultContent (also needs url, query, page_type, description; plus parser_type on AmazonSellerResult), eBayResultContent (10 more fields; note it cannot take = null on parse_status_code without an optional-before-required deprecation, because additional_properties follows it). Amazon pricing is the highest priority — it shares the oxylabs-insights-retrieve queue via TYPE_OFFER, and RetrieveAmazonPriceOffersResult.php:219 dereferences ->content->parse_status_code with no null-safe operator.
  2. AmazonSellerResult and GoogleShoppingProductResult have non-union content, so malformed payloads there fail loudly with CannotCreateData instead of degrading into an opaque TypeError — a better-behaved symptom, still a crash.
  3. Structural fix for the whole bug class: a #[WithCast] on $content that returns strings as strings, calls Content::from() for arrays, and lets CannotCreateData surface as a typed SDK exception. spatie's CombinationType swallow will otherwise keep converting any future required-field mismatch into this same opaque constructor TypeError. Out of scope here because it changes the raw-HTML path's behaviour.
  4. Unrelated latent bug spotted, deliberately untouched: in AmazonSellerResultContent, the #[DataCollectionOf(SellerFeedback::class)] attribute and its @var SellerFeedback[] docblock sit immediately above $parse_status_code instead of above $recent_feedback.
  5. .github/workflows/phpstan.yml runs on every **.php push and main already carries 4 errors. Fix in its own PR.

@qschmick
qschmick merged commit 4f2aa2a into main Sep 9, 2026
4 checks passed
@qschmick
qschmick deleted the fix/FLUX-511-content-hydration branch September 9, 2026 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant