fix: matching consumed the click, breaking reinstalls and repeat matches

docs/decisions.md

@@ -1438,3 +1438,115 @@ Node analogue and would blur what that file is for.
 **Credit.** Found downstream against a real deployment and reported
 upstream; independently reproduced here, including the sign-flip case,
 before the fix was applied.
+
+## 30. Matching consumed the click, so reinstalls lost attribution and repeat matches re-attributed silently — Done
+
+**Problem, reported downstream from real end-to-end device testing and
+reproduced here before anything was changed.** `/match` did not only read —
+it locked the winning click to the device, and the candidate query only ever
+considered unmatched clicks (`WHERE matched = 0` in PHP,
+`eq(referralClicks.matched, false)` in Node). A matched click therefore
+became invisible to everyone, *including the device that had just matched
+it*. `ClickStore::lockToDevice()`'s own docblock stated the original intent
+plainly: "so it can never be matched twice." That intent was the bug.
+
+**Symptom one — a reinstall recovers nothing.** One click, three install
+cycles. On iOS `identifierForVendor` is cleared when the last vendor app is
+uninstalled, so a reinstalled app presents a *new* device id:
+
+```
+install #1 (device-after-install-1): got FRIEND99 at 100.00
+install #2 (device-after-install-2): NO CODE RECOVERED
+install #3 (device-after-install-3): NO CODE RECOVERED
+```
+
+It compounded at `/claim`, which verifies the click is bound to the claiming
+device — so even a click that could still be found would have been rejected
+as unverified after a reinstall.
+
+**Symptom two — a repeat match credits a different referrer.** Same device,
+two identical calls seconds apart, two clicks in the window. The device's
+real referrer is `ALICE01`:
+
+```
+attempt 1 (SAME device): attributed to ALICE01 at 100.00
+attempt 2 (SAME device): attributed to BOB0002 at 85.00
+```
+
+Worse than symptom one, because nothing looks like an error. A retry, or a
+relaunch before signup completes, silently re-attributes the user to someone
+else. **The selection logic was never at fault and this was not
+non-determinism** — `match()` scans every candidate and keeps the maximum,
+with `ORDER BY created_at DESC` only settling ties. The second call correctly
+picked the best of what it could still see; the pool had changed underneath
+it. That misdiagnosis cost real time downstream, so it is written down here.
+
+**Decision.** Matching does not consume a click. `matched` now means "last
+matched by", not "used up" — guarding against a referral code being redeemed
+twice belongs in whatever records signups, not in making the click invisible
+to matching. The `matched` filter is gone from both backends' candidate
+queries.
+
+Removing the filter alone would have been wrong in one direction, though: a
+device whose fingerprint later stops clearing the threshold (IP moved, say)
+would lose an attribution it had already been given. So the resolution is:
+
+| existing binding | new candidate above threshold | result |
+| --- | --- | --- |
+| no | yes | best candidate — unchanged from before |
+| yes | no | keep the existing binding |
+| yes | yes | whichever **click** is newer |
+
+Two constraints that are easy to get wrong, both learned the hard way:
+
+- **Compare by click time, not confidence.** A stale click that happens to
+  score higher is still the wrong answer. Attribution is last-click-wins.
+- **A newer click below the confidence threshold must not take over**, or an
+  unrelated recent click could steal an attribution merely by being recent.
+
+An earlier attempt downstream stopped at "return the device's existing lock
+first." That was rejected, correctly: it binds a device to the first click it
+ever matched, so a user who clicks a *newer* referral link keeps being
+credited to the older referrer for the whole window.
+
+**A second method rather than a looser one.** `lockToDevice()`'s
+`AND matched = 0` is a security guard on the `/claim` path — the
+deterministic tier's first real use of a click, where losing the race must
+reject rather than proceed (#21). Matching has the opposite requirement, so
+it got its own `bindMatch()` which rebinds unconditionally. Loosening
+`lockToDevice()` itself would have quietly removed a claim-time guard while
+appearing to fix an unrelated matching bug.
+
+**Ordering by the click's time, not the lock's.** A device can now hold more
+than one binding, so the newest has to win. That ordering keys on
+`created_at` rather than `matched_at`: `matched_at` is written by
+`UTC_TIMESTAMP()` into a plain `TIMESTAMP`, both at one-second resolution, so
+two locks taken in the same second tie and the winner falls out of arbitrary
+storage order. `created_at` is stable *and* semantically right under
+last-click-wins.
+
+**A parity divergence fixed along the way.** PHP filtered candidates on both
+`expires_at > UTC_TIMESTAMP()` and the `created_at` window; Node filtered
+only on the window, with no expiry check at all. Harmless while expiry and
+the match window are the same duration, and a real difference between two
+backends documented as interchangeable the moment either is configured
+independently. Node now checks expiry too.
+
+**Verification.** The scoring suite could never have caught any of this — the
+bug was never in scoring, it was in which rows the query could see, and a
+pure-scoring suite has no database. `tests/run.php` now carries a
+database-backed section using in-memory SQLite with `UTC_TIMESTAMP()`
+shimmed via `sqliteCreateFunction()`, so it still provisions nothing: five
+reinstall cycles under five different device ids, three repeat matches
+against two competing referrers, takeover by a newer qualifying click,
+refusal of takeover by a newer non-qualifying one, and an unrelated device
+still matching nothing. Counter-checked by reintroducing only the
+`matched = 0` clause: 7 of the new assertions fail and reproduce both
+reported symptoms exactly, including the `ALICE01` → `BOB0002` flip. Worth
+noting that "a newer qualifying click takes over" *passes* against the buggy
+code — the old click is filtered out, so the new one wins by default — which
+is precisely why it isn't load-bearing on its own.
+
+**Credit.** Found downstream by real end-to-end device testing and reported
+upstream, along with the analysis that the selection logic was not the
+culprit. Independently reproduced here before the fix was written.

packages/referral-sdk-node/src/routes/referral.ts

@@ -139,16 +139,16 @@ export function referralRouter(db: Db, config: ReferralConfig): Router {
         timezone: fingerprint.timezone,
         language: fingerprint.language,
         deviceId: device_id,
-      });
+      }, storedDeviceId);
       if (!result) {
         return res.json({ matched: false, referral_code: null });
       }
 
-      // Lock atomically. If another request won the race, report no match
-      // rather than handing the same click to two devices.
-      if (!(await clicks.lockToDevice(result.clickId, storedDeviceId, 'fingerprint', result.confidence))) {
-        return res.json({ matched: false, referral_code: null });
-      }
+      // Record the binding. Unlike the claim path this rebinds rather than
+      // refusing when the click is already matched — a returning device must
+      // be able to recover a click it (or a previous install of it) already
+      // matched. See decisions.md #30.
+      await clicks.bindMatch(result.clickId, storedDeviceId, result.confidence);
 
       const token = signClickToken(result.clickId, result.expiresAt, getClickTokenSecret());
       return res.json({

packages/referral-sdk-node/src/services/clickStore.ts

@@ -62,6 +62,35 @@ export class ClickStore {
     return { clickId, expiresAt };
   }
 
+  /**
+   * Record the current fingerprint match on a click, rebinding it if it was
+   * already bound to some other device.
+   *
+   * Separate from lockToDevice() on purpose. That method's
+   * `matched = false` predicate is a security guard on the /claim path — the
+   * deterministic tier's first real use of a click, where losing the race
+   * must reject rather than proceed (decisions.md #21). Matching has the
+   * opposite requirement: a click has to be re-bindable, or a reinstall can
+   * never recover it, because on iOS the returning device presents a
+   * brand-new device id (decisions.md #30). Loosening lockToDevice() itself
+   * would have quietly removed that claim-time guard.
+   *
+   * Last write wins, which is exactly "last matched by" — and the caller has
+   * already decided this device should hold the attribution.
+   */
+  async bindMatch(clickId: string, deviceId: string, confidence: number): Promise<void> {
+    await this.db
+      .update(referralClicks)
+      .set({
+        matched: true,
+        matchedDeviceId: deviceId,
+        matchedAt: new Date(),
+        matchMethod: 'fingerprint',
+        matchConfidence: confidence,
+      })
+      .where(eq(referralClicks.clickId, clickId));
+  }
+
   /**
    * Atomically lock a click to a device so it can never be matched twice.
    * Returns true only if this call is the one that won the lock. Records

packages/referral-sdk-node/src/services/fingerprintMatcher.ts

@@ -1,4 +1,4 @@
-import { and, desc, eq, gte } from 'drizzle-orm';
+import { and, desc, gt, gte } from 'drizzle-orm';
 import type { ReferralConfig } from '../config.js';
 import type { Db } from '../db/client.js';
 import { referralClicks, referralMatchAttempts } from '../db/schema.js';
@@ -135,11 +135,34 @@ export class FingerprintMatcher {
     return weight * (1 - elapsedMs / windowMs);
   }
 
-  /** Find the best matching unclaimed click for a device, within the match window. */
-  async match(incoming: IncomingFingerprint): Promise<MatchResult | null> {
+  /**
+   * Find the best matching click for a device, within the match window.
+   *
+   * Matching deliberately does **not** consume a click. `matched` means
+   * "last matched by", not "used up" — see docs/decisions.md #30. An
+   * already-matched click stays a candidate, which is what lets a reinstall
+   * recover (on iOS the device id itself changes when the last vendor app is
+   * uninstalled, so the returning device is a *new* device as far as this
+   * table is concerned) and what stops a second match from silently falling
+   * through to a runner-up and re-attributing the user to a different
+   * referrer. Guarding against a code being redeemed twice belongs in
+   * whatever records signups, not in making the click invisible here.
+   *
+   * `deviceId` (already hashed) lets a device keep an attribution it has
+   * already been given if its fingerprint later stops clearing the
+   * confidence threshold.
+   */
+  async match(incoming: IncomingFingerprint, deviceId?: string): Promise<MatchResult | null> {
     const windowStart = new Date(Date.now() - this.config.matchWindowSeconds() * 1000);
 
-    // Only fresh, unmatched clicks. Newest first: last-click-wins on ties.
+    // No `matched = false` filter: that single clause was the whole of #17.
+    // The expiry check mirrors the PHP backend, which has always had it —
+    // the two are meant to be interchangeable, and without it this would
+    // diverge the moment expiry and the match window are configured to
+    // different durations.
+    // Newest first so ties resolve last-click-wins, and so the first row seen
+    // for this device is its most recent binding — ordering by matchedAt
+    // instead would be unstable at one-second storage resolution.
     const rows = await this.db
       .select({
         clickId: referralClicks.clickId,
@@ -152,43 +175,72 @@ export class FingerprintMatcher {
         language: referralClicks.language,
         createdAt: referralClicks.createdAt,
         expiresAt: referralClicks.expiresAt,
+        matchedDeviceId: referralClicks.matchedDeviceId,
       })
       .from(referralClicks)
       .where(
         and(
-          eq(referralClicks.matched, false),
           gte(referralClicks.createdAt, windowStart),
+          gt(referralClicks.expiresAt, new Date()),
         ),
       )
       .orderBy(desc(referralClicks.createdAt));
 
     let best: (typeof rows)[number] | null = null;
     let bestScore = 0;
+    let existing: (typeof rows)[number] | null = null;
 
     for (const row of rows) {
+      // Rows arrive newest-first, so the first one bound to this device is
+      // its newest binding; later (older) ones are superseded.
+      if (existing === null && deviceId !== undefined && row.matchedDeviceId === deviceId) {
+        existing = row;
+      }
+
       const s = this.score(row, incoming);
       if (s > bestScore) {
         bestScore = s;
         best = row;
       }
     }
 
-    const matched = best !== null && bestScore >= this.config.minConfidence;
+    // Only a candidate clearing the threshold may take an attribution over.
+    // Without this a newer but unrelated click could steal one purely by
+    // being recent.
+    const bestQualifies = best !== null && bestScore >= this.config.minConfidence;
+    const candidate = bestQualifies ? best : null;
+
+    let winner: (typeof rows)[number] | null;
+    if (candidate !== null && existing !== null) {
+      // Last-click-wins, compared by the *click's* time rather than by
+      // confidence: a stale click that happens to score higher is still the
+      // wrong answer.
+      winner = candidate.createdAt.getTime() > existing.createdAt.getTime() ? candidate : existing;
+    } else {
+      // Falling back to `existing` keeps an attribution the device already
+      // has when nothing clears the threshold this time — a retry or a
+      // relaunch on a weaker signal must not silently lose it.
+      winner = candidate ?? existing;
+    }
+
     await this.logAttempt(incoming, {
-      matched,
+      matched: winner !== null,
       candidateCount: rows.length,
       bestScore: rows.length > 0 ? bestScore : null,
       bestClickId: best?.clickId ?? null,
     });
 
-    if (!matched) return null;
+    if (winner === null) return null;
+
+    // Re-score the winner rather than reusing bestScore, which belongs to
+    // `best` and would be wrong whenever `existing` won.
+    const confidence = winner === best ? bestScore : this.score(winner, incoming);
 
     return {
-      // best is non-null here — matched only becomes true when it is.
-      clickId: best!.clickId,
-      referralCode: best!.referralCode,
-      confidence: Math.round(bestScore * 100) / 100,
-      expiresAt: best!.expiresAt,
+      clickId: winner.clickId,
+      referralCode: winner.referralCode,
+      confidence: Math.round(confidence * 100) / 100,
+      expiresAt: winner.expiresAt,
     };
   }
 

packages/referral-sdk/src/Controllers/MatchController.php

@@ -52,7 +52,7 @@ public function __invoke(Request $request): JsonResponse
         $fingerprint = $data['fingerprint'];
         $fingerprint['platform'] = $data['platform'];
 
-        $result = $this->matcher->match($fingerprint, (string) $request->ip());
+        $result = $this->matcher->match($fingerprint, (string) $request->ip(), $storedDeviceId);
 
         if ($result === null) {
             return response()->json([
@@ -61,14 +61,11 @@ public function __invoke(Request $request): JsonResponse
             ]);
         }
 
-        // Lock atomically. If another request won the race, report no match
-        // rather than handing the same click to two devices.
-        if (!$this->clicks->lockToDevice($result['click_id'], $storedDeviceId, 'fingerprint', $result['confidence'])) {
-            return response()->json([
-                'matched'       => false,
-                'referral_code' => null,
-            ]);
-        }
+        // Record the binding. Unlike the claim path this rebinds rather than
+        // refusing when the click is already matched — a returning device
+        // must be able to recover a click it (or a previous install of it)
+        // already matched. See docs/decisions.md #30.
+        $this->clicks->bindMatch($result['click_id'], $storedDeviceId, $result['confidence']);
 
         $token = ClickToken::sign($result['click_id'], $result['expires_at'], $this->config->requireClickTokenSecret());
 

packages/referral-sdk/src/Services/ClickStore.php

@@ -67,6 +67,41 @@ public function store(string $referralCode, array $fingerprint, string $ip): arr
         return ['click_id' => $clickId, 'expires_at' => $expiresAt];
     }
 
+    /**
+     * Record the current fingerprint match on a click, rebinding it if it was
+     * already bound to some other device.
+     *
+     * Separate from lockToDevice() on purpose. That method's
+     * `AND matched = 0` is a security guard on the /claim path — the
+     * deterministic tier's first real use of a click, where losing the race
+     * must reject rather than proceed (docs/decisions.md #21). Matching has
+     * the opposite requirement: a click has to be re-bindable, or a reinstall
+     * can never recover it, because on iOS the returning device presents a
+     * brand-new device id (docs/decisions.md #30). Loosening lockToDevice()
+     * itself would have quietly removed that claim-time guard.
+     *
+     * Last write wins, which is exactly "last matched by" — and the caller
+     * has already decided this device should hold the attribution.
+     */
+    public function bindMatch(
+        string $clickId,
+        string $deviceId,
+        float $confidence,
+    ): void {
+        $stmt = $this->pdo->prepare(
+            'UPDATE referral_clicks
+             SET matched = 1, matched_device_id = :device_id, matched_at = UTC_TIMESTAMP(),
+                 match_method = :method, match_confidence = :confidence
+             WHERE click_id = :click_id'
+        );
+        $stmt->execute([
+            ':device_id'  => $deviceId,
+            ':method'     => 'fingerprint',
+            ':confidence' => $confidence,
+            ':click_id'   => $clickId,
+        ]);
+    }
+
     /**
      * Atomically lock a click to a device so it can never be matched twice.
      * Returns true only if this call is the one that won the lock. Records