Repository navigation
fix: an inactive offer can still receive traffic - #7
klimenkoalex wants to merge 3 commits into
Conversation
core.offers.id is a uuid column, so a target whose offer_id is not
UUID-shaped does not miss the lookup — it aborts it with SQLSTATE 22P02
and the campaign answers 500 for every visitor, not just the share the
picker would have sent to that entry. FlowForm validates only that
target_offers decodes as an array, so such a row is reachable through
the admin UI.
candidateOfferId now drops anything that isn't UUID-shaped, the same way
OfferPicker already drops an empty offer_id, and the regex resolveOffer
was carrying inline is hoisted into one constant.
Tests cover the three paths the change touches: a malformed id next to a
healthy offer, an offer_id no longer present in core.offers, and a
{offer:<uuid>} trash reference to an inactive offer. All three fail on
7c27ba4; the malformed one still fails on 8fb670a.
|
Thank you for this one — the bug is real and it was the most valuable of the three. #5 and #6 are merged. I ran your branch against a live test database rather than only reading it. The suite is green (226 passed, 0 failed, and PHPStan reports nothing new under 1. An uppercase 2.
"Flow matched, but every candidate is inactive" is the same situation from the operator's point of view. Campaign with Before your change that campaign redirected to a disabled offer, which is the bug. After it, the operator's fallback is skipped instead. Both are wrong; the fallback should fire. 3. Filtering before the pick moves visitors the change should not touch. Half the unaffected audience changes offer. For a TDS with sticky routing that is a real cost, and it is not visible from the tests as written. The shape I'd rather have: pick from the unfiltered list first so existing assignments survive, verify only the winner, and fall back to a second deterministic pick among active candidates on a miss — overwriting Because this touches the sticky algorithm and the fallback contract, both of which are opinionated calls about how the product should behave, I'm taking the fix in-house rather than sending it back to you — that's on me, not a reflection on the work. I'm closing this PR in favour of #9, which carries your reproduction and keeps the tests of yours that hold. Your commit structure — test, then fix, then the adjacent failure mode — made all of this straightforward to verify, and the |
|
Closed in favour of #9, which carries your reproduction, and #10, which is now merged. Everything of yours that held is in there unchanged: Your three commits made the difference here. The 22P02 catch in particular was not in the report and would not have been found by reading the diff alone. Thanks, and please do send more. |
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 izzipizzy#9. Supersedes izzipizzy#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
Summary
Turning an offer off in the admin UI should stop it from receiving new traffic.
On
main(a25a43e), an inactive offer can still be selected when an active flow or atrashtarget references it. Fixing that surfaced one adjacent failure mode: a malformedoffer_idreaches PostgreSQL's UUID column and aborts the whole candidate lookup, returning500even when a healthy offer is available.This PR proves both behaviours with integration tests and fixes them together.
User-visible impact
maintoday302)204)100, active fallback has weight1trash = {offer:name}or{offer:uuid}points to an inactive offer204)500)302)In plain language: after this change, the Inactive switch means what an operator expects, and one damaged flow entry cannot take the entire campaign down.
Root cause
trashoffer references did not consistently enforce active status.uuidcolumn without validating that they were UUID-shaped.Fix
trashreferences to resolve to an active offer.Reproduction and commit structure
The commits are separated so the failure and the fix can be reviewed independently:
5f1a2f7— adds integration tests reproducing traffic to inactive offers.5ddc439— excludes inactive offers from normal andtrashrouting.5c33a80— covers malformed/missing ids and UUIDtrashreferences, then prevents malformed ids from aborting the campaign.Measured on this branch, rebased onto
a25a43e, running--filter=ClickHandler:5f1a2f7(tests only)5ddc4395c33a80The third commit is not padding. Checking out
5ddc439and applying only the test file from5c33a80leaves exactly one red test —a malformed offer_id does not take the rest of the flow down with it— with the other 13 green.Verification
The commit-by-commit table above was measured with the repository's Docker test environment (
make test,--filter=ClickHandler).Full suites are from the unmodified upstream workflow, run in a public fork:
main(a25a43e)5c33a80)Unit (85 passed, 22,551 assertions), Arch (7 passed, 20 assertions) and PHPStan (no errors) are identical across all three, as are the deprecation, warning and skip counts.
The single failure on the middle row is
CurlProxySchemaTest, which fails the same way onmainand has nothing to do with this branch — #5 restores it. The last row is that combination, and it is green.Safety and scope
OfferRepository, the selection change inClickHandler, and additional cases in the existingClickHandlerTest.a25a43eand merges cleanly.