Skip to content

Inactive offers still receive traffic — fix must preserve sticky assignments, honour trash mode, and not regress uppercase UUIDs #9

Closed

Description

@izzipizzy

The underlying bug reported in #7 is real and confirmed: ClickHandler picks from flow.target_offers and resolves the winner with findById(), neither of which looks at is_active. Turning an offer off in the admin UI does not stop it receiving traffic. A malformed offer_id in the JSONB also reaches the uuid column and aborts the request with 22P02.

This issue tracks the fix, because the approach proposed in #7 — filter the candidate list before OfferPicker, then look the winner up in that filtered map — introduces three problems. All three were reproduced against a live test database.

1. Uppercase offer_id becomes a regression

ClickHandler::OFFER_ID matches case-insensitively (/i), so an uppercase UUID is accepted as a candidate. PostgreSQL compares uuid case-insensitively too, so the row is found — and returned with its canonical lowercase id. The map is therefore keyed lowercase while isset($activeOffers[$target]) looks it up under the original uppercase key, and an active offer is silently dropped.

Active offer, target_offers = [{"offer_id": "<UPPERCASE UUID>", "weight": 100}]:

before:  status=302  location=https://probe-a.example/
after:   status=204  (offer dropped)

Any fix must normalise the id on both sides, or stop accepting mixed case in the first place.

2. "No routable offer" must fall through to the campaign trash mode

When every candidate is inactive, $outUrl stays null and the schema decides what to do — for HttpRedirectSchema that is 204. The operator's configured fallback never fires.

Campaign with trash_mode = 2 (HTTP 403), flow matched, its only offer inactive:

expected: 403   (what the operator configured)
actual:   204

This contradicts the contract the $flow === null branch already states in its own comment:

No flow matched — fall through to the campaign's trash mode so the operator-configured fallback (302/403/404/200) actually fires.

"Flow matched but has no routable offer" is the same situation and should take the same path: correct status, resolved trash_url, the real out_url logged, cookie attached.

3. Sticky routing reshuffles visitors the change should not touch

OfferPicker in sticky mode is hash(visitor_uuid) % sum(weights). Filtering candidates before the pick changes the modulus, so every assignment moves — not just the ones sitting on the offer that was switched off.

Measured, three offers at equal weight, middle one deactivated, 3000 synthetic visitors:

of the visitors NOT on the disabled offer: stayed=996  moved=987  → 49.8% churn

Half of the unaffected audience changes offer. That breaks what the parameter promises: "sticky → one visitor always lands on the same offer".

Preferred shape: pick from the unfiltered list first, so existing assignments are preserved, then check only the winner. On a miss — inactive, missing, or malformed — do a second deterministic pick among the active candidates and overwrite Context::$matchedOfferId with what was actually served. The hot path stays a single primary-key lookup, and only visitors on the disabled offer move.

Coverage the fix needs

  • a sticky visitor on an unaffected offer keeps that offer when a sibling is deactivated
  • campaign trash modes fire when a matched flow has no routable offer — at least 302 and 403/404
  • an uppercase UUID candidate still routes
  • matchedOfferId matches the offer actually served after a fallback pick
  • {offer:<name>} in trash_url when the newest offer of that name is inactive — current behaviour is "newest wins, then reject", which should be pinned down deliberately

Tests from #7 that stay valid: inactive offer receives no traffic; inactive candidate does not win on weight; malformed id does not take a healthy sibling down; missing id is skipped; {offer:name} and {offer:uuid} in trash_url refuse an inactive offer. The two that encode 204 as the expected outcome need to change with item 2.

Activity

  1. izzipizzy commented on Aug 23, 2026

    @izzipizzy
    OwnerAuthor

    Fixed in #10 (678cbad) — auto-closed by the commit, adding the verification here.

    All three constraints are covered by tests that were checked against the implementation in #7 to confirm they are guards rather than decoration; five go red there:

    • uppercase offer_id still routes
    • a matched flow with nothing routable falls through to the campaign trash mode, both the 403 and the redirect variants
    • a flow carrying a static schema_config.url keeps using it
    • deactivating one offer leaves the other sticky visitors where they were

    Integration suite 231 passed, 0 failed. PHPStan over src/ unchanged at its pre-existing count.

  2. added a commit that references this issue on Aug 24, 2026
    3102fa4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions