Skip to content

Commit ba1a1b8

Browse files
committed
fix: make deduplicateAccounts converge to a fixpoint; add identity property suite
fast-check found the bug on its 11th generated pool: a newest-wins merge in the email tier can install an account that duplicates an earlier survivor through a different tier (accountId + refresh token), so a single dedup pass could return a list still containing two records for the same identity. deduplicateAccountsByIdentity now re-runs the pass until the length stabilizes; every merge strictly shrinks the array, so the loop terminates after at most accounts.length passes. The new test/property/account-identity.property.test.ts pins: - normalizeEmailKey idempotence and case/whitespace insensitivity - findMatchingAccountIndex order-invariance under pool permutation, candidate-spelling invariance, facet-sharing soundness, foreign candidates never matching, and the email-ambiguity veto - deduplicateAccounts subset + idempotence properties, plus a deterministic replay of the discovered counterexample https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
1 parent 6ede089 commit ba1a1b8

2 files changed

Lines changed: 259 additions & 2 deletions

File tree

lib/storage.ts

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,8 +1052,8 @@ export function resolveAccountSelectionIndex<
10521052
return clampIndex(fallbackIndex, accounts.length);
10531053
}
10541054

1055-
function deduplicateAccountsByIdentity<T extends AccountLike>(
1056-
accounts: T[],
1055+
function deduplicateAccountsByIdentityPass<T extends AccountLike>(
1056+
accounts: readonly T[],
10571057
): T[] {
10581058
const deduplicated: T[] = [];
10591059
for (const account of accounts) {
@@ -1071,6 +1071,23 @@ function deduplicateAccountsByIdentity<T extends AccountLike>(
10711071
return deduplicated;
10721072
}
10731073

1074+
function deduplicateAccountsByIdentity<T extends AccountLike>(
1075+
accounts: T[],
1076+
): T[] {
1077+
// A single pass is not a fixpoint: a newest-wins merge can replace a kept
1078+
// entry with an account that itself duplicates an earlier survivor through
1079+
// a different matching tier (e.g. an email-tier merge installs an account
1080+
// whose accountId + refresh token already match a record that carried no
1081+
// email). Re-run until stable; every merge strictly shrinks the array, so
1082+
// this terminates after at most accounts.length passes.
1083+
let current = deduplicateAccountsByIdentityPass(accounts);
1084+
for (;;) {
1085+
const next = deduplicateAccountsByIdentityPass(current);
1086+
if (next.length === current.length) return next;
1087+
current = next;
1088+
}
1089+
}
1090+
10741091
/**
10751092
* Removes duplicate accounts, keeping the most recently used entry for each
10761093
* safely matched identity.
Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
import { describe, expect, it } from "vitest";
2+
import * as fc from "fast-check";
3+
import {
4+
deduplicateAccounts,
5+
findMatchingAccountIndex,
6+
normalizeEmailKey,
7+
} from "../../lib/storage.js";
8+
9+
// Small identity alphabets so generated pools actually collide on facets;
10+
// the matcher's interesting behavior (ambiguity vetoes, newest-wins, tier
11+
// precedence) only shows up when identities overlap.
12+
const ACCOUNT_IDS = ["acc-1", "acc-2", "acc-3"];
13+
const EMAIL_NAMES = ["alice", "bob", "carol"];
14+
const REFRESH_TOKENS = ["rt-1", "rt-2", "rt-3"];
15+
16+
type TestAccount = {
17+
accountId?: string;
18+
email?: string;
19+
refreshToken?: string;
20+
lastUsed?: number;
21+
addedAt?: number;
22+
};
23+
24+
// A spelled variant of an email that must normalize back to the same key:
25+
// arbitrary casing plus surrounding whitespace.
26+
const arbSpelledEmail = fc
27+
.tuple(
28+
fc.constantFrom(...EMAIL_NAMES),
29+
fc.boolean(),
30+
fc.constantFrom("", " ", "\t", " "),
31+
fc.constantFrom("", " ", "\t"),
32+
)
33+
.map(([name, upper, pad, trail]) => {
34+
const address = `${name}@example.com`;
35+
return `${pad}${upper ? address.toUpperCase() : address}${trail}`;
36+
});
37+
38+
const arbAccount: fc.Arbitrary<TestAccount> = fc.record(
39+
{
40+
accountId: fc.option(fc.constantFrom(...ACCOUNT_IDS), { nil: undefined }),
41+
email: fc.option(arbSpelledEmail, { nil: undefined }),
42+
refreshToken: fc.option(fc.constantFrom(...REFRESH_TOKENS), {
43+
nil: undefined,
44+
}),
45+
addedAt: fc.nat(1_000_000),
46+
},
47+
{ requiredKeys: [] },
48+
);
49+
50+
// Pools get strictly distinct lastUsed values so "newest wins" has a unique
51+
// answer; ties would otherwise be broken by array position, which is exactly
52+
// the order-dependence the permutation property must not trip over. Each
53+
// account also carries a random sort key so pools can be re-shuffled into a
54+
// wide range of permutations.
55+
const arbPoolWithPermutation = fc
56+
.array(fc.tuple(arbAccount, fc.nat(1_000_000)), {
57+
minLength: 1,
58+
maxLength: 8,
59+
})
60+
.map((entries) => {
61+
const pool = entries.map(([account, permKey], index) => ({
62+
account: { ...account, lastUsed: (index + 1) * 1_000 },
63+
permKey,
64+
}));
65+
const permuted = [...pool]
66+
.sort((a, b) => a.permKey - b.permKey)
67+
.map((entry) => entry.account);
68+
return { pool: pool.map((entry) => entry.account), permuted };
69+
});
70+
71+
const arbCandidate: fc.Arbitrary<TestAccount> = arbAccount;
72+
73+
describe("normalizeEmailKey properties", () => {
74+
it("is idempotent and insensitive to casing and surrounding whitespace", () => {
75+
fc.assert(
76+
fc.property(arbSpelledEmail, (spelled) => {
77+
const key = normalizeEmailKey(spelled);
78+
expect(key).toBe(spelled.trim().toLowerCase());
79+
expect(normalizeEmailKey(key)).toBe(key);
80+
expect(normalizeEmailKey(spelled.toUpperCase())).toBe(key);
81+
expect(normalizeEmailKey(` ${spelled} `)).toBe(key);
82+
}),
83+
);
84+
});
85+
86+
it("returns undefined for missing or blank input", () => {
87+
fc.assert(
88+
fc.property(fc.constantFrom("", " ", "\t", " \t "), (blank) => {
89+
expect(normalizeEmailKey(blank)).toBeUndefined();
90+
}),
91+
);
92+
expect(normalizeEmailKey(undefined)).toBeUndefined();
93+
});
94+
});
95+
96+
describe("findMatchingAccountIndex properties", () => {
97+
it("matches the same account regardless of pool order", () => {
98+
fc.assert(
99+
fc.property(arbPoolWithPermutation, arbCandidate, ({ pool, permuted }, candidate) => {
100+
const originalIndex = findMatchingAccountIndex(pool, candidate);
101+
const permutedIndex = findMatchingAccountIndex(permuted, candidate);
102+
if (originalIndex === undefined) {
103+
expect(permutedIndex).toBeUndefined();
104+
return;
105+
}
106+
expect(permutedIndex).not.toBeUndefined();
107+
// Same account object, not just same identity: with distinct
108+
// lastUsed values the newest-wins rule has a unique answer.
109+
expect(permuted[permutedIndex as number]).toBe(pool[originalIndex]);
110+
}),
111+
);
112+
});
113+
114+
it("is insensitive to candidate email spelling", () => {
115+
fc.assert(
116+
fc.property(
117+
arbPoolWithPermutation,
118+
arbCandidate,
119+
fc.boolean(),
120+
({ pool }, candidate, upper) => {
121+
fc.pre(candidate.email !== undefined);
122+
const respelled = {
123+
...candidate,
124+
email: upper
125+
? ` ${(candidate.email as string).toUpperCase()}`
126+
: `${(candidate.email as string).toLowerCase()} `,
127+
};
128+
expect(findMatchingAccountIndex(pool, respelled)).toBe(
129+
findMatchingAccountIndex(pool, candidate),
130+
);
131+
},
132+
),
133+
);
134+
});
135+
136+
it("only ever matches an account sharing an identity facet with the candidate", () => {
137+
fc.assert(
138+
fc.property(arbPoolWithPermutation, arbCandidate, ({ pool }, candidate) => {
139+
const index = findMatchingAccountIndex(pool, candidate);
140+
if (index === undefined) return;
141+
const matched = pool[index] as TestAccount;
142+
const sharesAccountId =
143+
candidate.accountId !== undefined &&
144+
matched.accountId === candidate.accountId;
145+
const sharesEmail =
146+
normalizeEmailKey(candidate.email) !== undefined &&
147+
normalizeEmailKey(matched.email) === normalizeEmailKey(candidate.email);
148+
const sharesRefreshToken =
149+
candidate.refreshToken !== undefined &&
150+
matched.refreshToken === candidate.refreshToken;
151+
expect(sharesAccountId || sharesEmail || sharesRefreshToken).toBe(true);
152+
}),
153+
);
154+
});
155+
156+
it("never matches a candidate whose facets are all foreign to the pool", () => {
157+
fc.assert(
158+
fc.property(arbPoolWithPermutation, ({ pool }) => {
159+
const foreign = {
160+
accountId: "acc-foreign",
161+
email: "nobody@elsewhere.test",
162+
refreshToken: "rt-foreign",
163+
};
164+
expect(findMatchingAccountIndex(pool, foreign)).toBeUndefined();
165+
}),
166+
);
167+
});
168+
169+
it("refuses email-only matches when the email maps to multiple account ids", () => {
170+
fc.assert(
171+
fc.property(
172+
arbPoolWithPermutation,
173+
arbSpelledEmail,
174+
fc.boolean(),
175+
({ pool }, email, upper) => {
176+
const ambiguous: TestAccount[] = [
177+
...pool,
178+
{ accountId: "acc-1", email, lastUsed: 999_001 },
179+
{
180+
accountId: "acc-2",
181+
email: upper ? email.toUpperCase() : email.toLowerCase(),
182+
lastUsed: 999_002,
183+
},
184+
];
185+
expect(
186+
findMatchingAccountIndex(ambiguous, { email }),
187+
).toBeUndefined();
188+
},
189+
),
190+
);
191+
});
192+
});
193+
194+
describe("deduplicateAccounts properties", () => {
195+
// Deterministic replay of the counterexample this suite originally found:
196+
// the email tier merges the last record into the carol entry, leaving the
197+
// emailless acc-1/rt-1 record as a separate survivor that a second pass
198+
// would merge. The fixpoint loop in deduplicateAccountsByIdentity now
199+
// converges before returning.
200+
it("merges duplicates that only become adjacent after a newest-wins replacement", () => {
201+
const emaillessOriginal = {
202+
accountId: "acc-1",
203+
refreshToken: "rt-1",
204+
lastUsed: 1_000,
205+
};
206+
const carolWithoutId = {
207+
email: "carol@example.com",
208+
refreshToken: "rt-2",
209+
lastUsed: 3_000,
210+
};
211+
const relogin = {
212+
accountId: "acc-1",
213+
email: "carol@example.com",
214+
refreshToken: "rt-1",
215+
lastUsed: 7_000,
216+
};
217+
const deduplicated = deduplicateAccounts([
218+
emaillessOriginal,
219+
carolWithoutId,
220+
relogin,
221+
]);
222+
expect(deduplicated).toStrictEqual([relogin]);
223+
});
224+
225+
it("returns a subset of the input accounts and is idempotent", () => {
226+
fc.assert(
227+
fc.property(arbPoolWithPermutation, ({ pool }) => {
228+
const deduplicated = deduplicateAccounts([...pool]);
229+
expect(deduplicated.length).toBeLessThanOrEqual(pool.length);
230+
for (const account of deduplicated) {
231+
// Object identity: dedup picks survivors, it never synthesizes.
232+
expect(pool).toContain(account);
233+
}
234+
expect(deduplicateAccounts([...deduplicated])).toStrictEqual(
235+
deduplicated,
236+
);
237+
}),
238+
);
239+
});
240+
});

0 commit comments

Comments
 (0)