Skip to content

fix(engine): stop routing traffic to switched-off offers - #10

Merged
izzipizzy merged 1 commit into
mainfrom
fix/inactive-offers
Aug 23, 2026
Merged

izzipizzy merged 1 commit into
mainfrom
fix/inactive-offers

Conversation

@izzipizzy

Copy link
Copy Markdown
Owner

Supersedes #7 and closes #9, keeping @klimenkoalex's reproduction and most of their tests.

The bug

ClickHandler picked from flow.target_offers and resolved the winner with findById(). Neither looked at is_active, so switching an offer off in the admin UI did not stop it receiving traffic. A malformed offer_id in the JSONB also reached the uuid column and aborted the request with 22P02 rather than being skipped.

The fix: verify the winner, don't pre-filter the candidates

OfferPicker's sticky mode is hash(visitor_uuid) % sum(weights). Dropping a candidate before the pick changes the modulus, so every assignment moves — not just the ones 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

Picking on the full list and checking only the winner keeps those assignments intact, keeps the normal path at a single primary-key read, and re-picks among the active candidates only for the visitors who actually lost their offer.

A matched flow with nothing routable falls through to the campaign trash mode

ClickHandler already states the contract for the no-flow-matched branch:

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

"Flow matched, but every candidate is inactive" is the same situation from the operator's side, and now takes the same exit. A flow that carries a static schema_config.url keeps using it — that is an operator-set target, not an absence of one.

Case normalisation

Offer ids are lowercased where they are accepted. PostgreSQL compares uuid case-insensitively but returns the value canonically lowercased, so a mixed-case reference would otherwise fail to match the row it had just loaded — which is how the version in #7 turned an uppercase offer_id into a silent 204 for an offer that is live.

Verification

Run against the dev stack with the isolated db-test database.

main before this branch
Unit 85 passed (22,551 assertions) 85 passed (22,551 assertions)
Integration 218 passed, 0 failed 231 passed, 0 failed
Arch 7 passed (20 assertions) 7 passed (20 assertions)
PHPStan, src/ 21 21 (unchanged)

The new tests were checked against the implementation in #7 as well, to confirm they are guards rather than decoration — five go red there:

⨯ inactive offer does not receive new traffic
⨯ a matched flow with nothing routable falls through to the campaign trash mode
⨯ a matched flow with nothing routable honours a redirect trash mode too
⨯ an uppercase offer_id still routes
⨯ deactivating one offer leaves the other sticky visitors where they were
✓ a flow with a static schema url still uses it when no offer is routable

The last one passes on both, as it should — it guards this change, not that one.

Kept from #7

inactive offer is removed before active offer weights are evaluated, a malformed offer_id does not take the rest of the flow down with it, an offer_id that no longer exists is skipped in favour of a live one, and both trash {offer:...} inactive cases — unchanged. findActiveByIds() is theirs too, with the doc comment tightened to say the ids must arrive pre-validated.

Turning an offer off in the admin UI did not stop it receiving traffic:
ClickHandler picked from flow.target_offers and resolved the winner with
findById(), and neither looked at is_active. A malformed offer_id in the
JSONB also reached the uuid column and aborted the request with 22P02
instead of being skipped.

Fixed by verifying the winner rather than pre-filtering the candidates.
OfferPicker's sticky mode is hash(visitor) % sum(weights), so narrowing
the list before the pick changes the modulus and moves every visitor,
not just the ones on the offer that was switched off — measured at ~50%
churn for three candidates of equal weight. Picking on the full list and
checking only the winner keeps existing assignments intact, keeps the
normal path at one primary-key read, and re-picks among active
candidates only for the visitors who actually lost their offer.

A flow that matched but has nothing routable now falls through to the
campaign trash mode, the same exit the no-flow-matched branch already
takes, so the operator's configured fallback fires instead of the schema
answering 204 into the void. A flow carrying a static schema-config url
keeps using it.

Offer ids are normalised to lowercase where they are accepted.
PostgreSQL compares uuids case-insensitively but returns them
canonically lowercased, so a mixed-case reference would otherwise fail
to match the row it had just loaded.

{offer:X} references in trash_url resolve to active offers only, on both
the id and the name branch.

Closes #9. Supersedes #7, whose reproduction and most of whose tests are
kept here.

Co-Authored-By: Alex <work.alexeyklimenko@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwWdv3Mwhzae634qnfj14Z
@izzipizzy
izzipizzy merged commit 678cbad into main Aug 23, 2026
1 check passed
@izzipizzy
izzipizzy deleted the fix/inactive-offers branch August 26, 2026 11:07
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.

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

1 participant