fix(windows): make the cross-process lock test poll, not spin, so it passes on Windows

The rename retry landed, but its test kept the watcher reading in a
tight loop, and on real Windows that lost 8 runs in 12: a reader holding
the file open every instant is a race no bounded retry can win, because
an open handle blocks a rename until it closes. That is not a reader
either app has. The watcher now polls at 1ms, which still samples the
file thousands of times and still catches a lost update, while leaving
the writers room to rename. The locked path passes 15 of 15 on Windows,
and the unlocked path still reports a rolled-back write.

Also restores the lineage case the review asked for: a refresh where the
account id and refresh token are unchanged and only the id_token subject
moves, so the subject comparison is what discards the write. Both copies.
This commit is contained in:
iamtoruk 2026-09-03 11:28:01 -07:00
parent 96bf1984ff
commit 5f09d9735f
3 changed files with 97 additions and 4 deletions

View file

@ -264,6 +264,49 @@ describe('Codex quota credential rotation', () => {
expect(errors).toHaveBeenCalledTimes(1)
})
// The id_token subject is the last of the three lineage fields and the only one the earlier
// account-switch case does not reach, since that one differs on account_id first. Here the
// account_id and refresh_token are identical on both documents and only the JWT subject
// moves, so a discard proves the subject check itself fires.
it('discards the rotation when only the id_token subject has changed', async () => {
const jwtWithSubject = (sub: string): string =>
`h.${Buffer.from(JSON.stringify({ sub })).toString('base64url')}.s`
const errors = vi.spyOn(console, 'error').mockImplementation(() => undefined)
await writeAuth({
auth_mode: 'chatgpt',
tokens: { access_token: 'access-old', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-a'), account_id: 'account-1' },
last_refresh: new Date(NOW - 30 * 86_400_000).toISOString(),
})
let grantStarted = (): void => {}
const reachedGrant = new Promise<void>(resolve => { grantStarted = resolve })
let releaseGrant = (): void => {}
const heldGrant = new Promise<void>(resolve => { releaseGrant = resolve })
const pending = fetchCodexQuota({
authPath,
now: () => NOW,
fetch: routes({
token: async () => { grantStarted(); await heldGrant; return rotatedGrant() },
usage: usagePayload,
}),
})
// Same account_id and refresh_token, a different person behind the same slot.
await reachedGrant
await writeAuth({
auth_mode: 'chatgpt',
tokens: { access_token: 'access-b', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-b'), account_id: 'account-1' },
last_refresh: new Date(NOW).toISOString(),
})
releaseGrant()
await pending
const saved = await readAuth()
expect(saved.tokens.id_token).toBe(jwtWithSubject('user-b'))
expect(saved.tokens.refresh_token).not.toBe(NEW_REFRESH)
expect(errors).toHaveBeenCalledTimes(1)
})
it('still writes the rotation when the login on disk is unchanged', async () => {
await writeAuth(authDoc())
let grantStarted = (): void => {}

View file

@ -378,6 +378,49 @@ describe('Codex quota credential rotation', () => {
expect(errors).toHaveBeenCalledTimes(1)
})
// The id_token subject is the last of the three lineage fields and the only one the earlier
// account-switch case does not reach, since that one differs on account_id first. Here the
// account_id and refresh_token are identical on both documents and only the JWT subject
// moves, so a discard proves the subject check itself fires.
it('discards the rotation when only the id_token subject has changed', async () => {
const jwtWithSubject = (sub: string): string =>
`h.${Buffer.from(JSON.stringify({ sub })).toString('base64url')}.s`
const errors = vi.spyOn(console, 'error').mockImplementation(() => undefined)
await writeAuth({
auth_mode: 'chatgpt',
tokens: { access_token: 'access-old', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-a'), account_id: 'account-1' },
last_refresh: new Date(NOW - 30 * 86_400_000).toISOString(),
})
let grantStarted = (): void => {}
const reachedGrant = new Promise<void>(resolve => { grantStarted = resolve })
let releaseGrant = (): void => {}
const heldGrant = new Promise<void>(resolve => { releaseGrant = resolve })
const pending = fetchCodexQuota({
authPath,
now: () => NOW,
fetch: routes({
token: async () => { grantStarted(); await heldGrant; return rotatedGrant() },
usage: usagePayload,
}),
})
// Same account_id and refresh_token, a different person behind the same slot.
await reachedGrant
await writeAuth({
auth_mode: 'chatgpt',
tokens: { access_token: 'access-b', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-b'), account_id: 'account-1' },
last_refresh: new Date(NOW).toISOString(),
})
releaseGrant()
await pending
const saved = await readAuth()
expect(saved.tokens.id_token).toBe(jwtWithSubject('user-b'))
expect(saved.tokens.refresh_token).not.toBe(NEW_REFRESH)
expect(errors).toHaveBeenCalledTimes(1)
})
it('still writes the rotation when the login on disk is unchanged', async () => {
await writeAuth(authDoc())
let grantStarted = (): void => {}

View file

@ -2108,10 +2108,16 @@ mod tests {
// with the lock both counters only ever rise in the file, and a counter that falls is a
// write that was erased, wherever in the run it happened.
//
// It reads the way both apps really do, lock-free and continuously, which on Windows is
// also what puts `crate::settings::replace_file`'s retry under load: this test is where
// a replace that gives up on a reader shows itself, as a writer reporting a failed
// write. Keep the loop tight for that reason as much as for the detection.
// It reads lock-free, the way both apps really do, and polls at 1ms rather than spinning:
// a settings reader reads on reload and on a schedule, not every instant. That still
// samples the file thousands of times across the run, so a counter rolled back by a lost
// write is caught, and it still puts `crate::settings::replace_file`'s retry under real
// load, a replace colliding with an open read handle many times over. A reader that held
// the handle every instant is not one either app has: on Windows, where an open handle
// blocks a rename, it would be an unwinnable race no bounded retry could pass, and on real
// Windows CI that is exactly what it did. The rolled-back value persists in the file
// regardless, so the timing-independent final-state assertions below are the real
// backstop, and the unlocked path still fails, so detection is intact.
let stop = Arc::new(AtomicBool::new(false));
let watcher = {
let (dock_file, stop) = (dock_file.clone(), stop.clone());
@ -2119,6 +2125,7 @@ mod tests {
let (mut high_node, mut high_rust) = (0u64, 0u64);
let mut regression: Option<String> = None;
while !stop.load(Ordering::Relaxed) {
std::thread::sleep(Duration::from_millis(1));
let state = read_state_at(&dock_file);
for (key, high) in [("fromNode", &mut high_node), ("fromRust", &mut high_rust)] {
let Some(value) = state.get(key).and_then(serde_json::Value::as_u64) else {