fix: matching consumed the click, breaking reinstalls and repeat matches - #21
Merged
Merged
Conversation
Closes #17. /match locked the winning click to the device while the candidate query only considered unmatched clicks, so a matched click became invisible to everyone — including the device that had just matched it. Two user-visible failures, one clause: - a reinstall recovered nothing. On iOS identifierForVendor is cleared when the last vendor app is uninstalled, so the returning device presents a new id and could not find its own click. - a repeat match silently re-attributed the user to a different referrer, falling through to the runner-up. Nothing logged an error. matched now means 'last matched by', not 'used up'. Preventing a code from being redeemed twice belongs in whatever records signups — the SDK's job is to report what was actually clicked, not to make the click disappear. Removing the filter alone would have lost an attribution whenever a device's signal later stopped clearing the threshold, so an existing binding is kept unless a newer click qualifies. Compared by the click's time rather than confidence: a stale click that scores higher is still wrong, and a newer click below the threshold must not take over or an unrelated recent click could steal an attribution. bindMatch() is deliberately separate from lockToDevice() rather than loosening it — that method's matched=0 predicate is a security guard on the claim path (#21), and relaxing it would have removed a claim-time check while appearing to fix a matching bug. Also fixes a parity divergence found alongside: PHP filtered candidates on expires_at, Node did not. Tests: run.php gains a database-backed section (in-memory SQLite, UTC_TIMESTAMP shimmed, still zero-dependency) — the scoring suite could never reach this, since the bug was in which rows the query could see rather than in scoring. Counter-checked by reintroducing only the matched=0 clause: 7 assertions fail and reproduce both symptoms. See docs/decisions.md #30.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #17.
/matchlocked the winning click to the device while the candidate query only considered unmatched clicks —WHERE matched = 0in PHP,eq(referralClicks.matched, false)in Node. A matched click therefore became invisible to everyone, including the device that had just matched it.lockToDevice()'s own docblock stated the original intent: "so it can never be matched twice." That intent was the bug.The principle behind the fix
The SDK's job is to report what was actually clicked. Preventing a referral code from being redeemed twice belongs in whatever records signups and rewards — that layer knows what redemption means in a given product; the matcher does not. Making the click disappear was the SDK enforcing a policy that was never its own.
So
matchednow means "last matched by", not "used up".Resolution
Removing the filter alone would have been wrong in one direction: a device whose fingerprint later stops clearing the threshold (its IP moved, say) would lose an attribution it had already been given.
Two constraints that are easy to get wrong:
bindMatch()rather than a looserlockToDevice()lockToDevice()'smatched = 0predicate is a security guard on the/claimpath (#21) — the deterministic tier's first real use of a click, where losing the race must reject rather than proceed. Matching has the opposite requirement, so it got its own method. Loosening the shared one would have quietly removed a claim-time guard while appearing to fix an unrelated matching bug.Also fixed
A parity divergence found alongside: PHP filtered candidates on
expires_at > UTC_TIMESTAMP(), Node had no expiry check at all. Harmless while expiry and the match window are the same duration, real the moment either is configured independently.Ordering for a device's bindings keys on the click's
created_at, notmatched_at— the latter is written byUTC_TIMESTAMP()into a plainTIMESTAMP, both one-second resolution, so same-second locks tie and resolve in arbitrary storage order.Verification
The scoring suite structurally could not catch this: the bug was never in scoring, it was in which rows the candidate query could see, and a pure-scoring suite has no database.
tests/run.phpnow carries a database-backed section — in-memory SQLite withUTC_TIMESTAMP()shimmed viasqliteCreateFunction(), so it still provisions and installs nothing:25 assertions pass under UTC, Africa/Lagos, America/New_York and Pacific/Chatham. Node: 37/37, typecheck and build clean on all three packages.
Counter-checked by reintroducing only the
matched = 0clause — 7 assertions fail and reproduce both reported symptoms exactly, including theALICE01→BOB0002flip.One caveat recorded in both the test comments and decisions #30: "a newer qualifying click takes over" passes even against the buggy code, since the old click is filtered out and the new one wins by default. It passes for the wrong reason there and is not load-bearing on its own.
Full writeup in
docs/decisions.md#30.