Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/commands/auth/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ const disconnect = async (deps: SetupDeps): Promise<void> => {
return;
}
await deps.revoke(links(auth.siteFqdn).auth, auth.tokens.accessToken);
deleteAuth();
await deleteAuth();
console.log('Disconnected - local credentials removed.');
};

Expand Down
72 changes: 71 additions & 1 deletion src/lib/auth/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
TransientAuthError,
} from './api.js';
import { patAuthState } from './credentials.js';
import { readAuth, type AuthState } from '../fs/config.js';
import { deleteAuth, readAuth, saveAuth, type AuthState } from '../fs/config.js';
import { links } from '../net/origins.js';
import { VERSION } from '../../utils/version.js';

Expand Down Expand Up @@ -79,6 +79,7 @@ describe('authedGet', () => {
});
vi.stubGlobal('fetch', fetchMock);
const auth = makeAuth();
saveAuth(auth); // a real refresh always runs against a session already on disk
const result = await authedGet<{ ok: boolean }>(auth, target, 'https://x.example/me');
expect(result.ok).toBe(true);
expect(auth.tokens.accessToken).toBe('new-token');
Expand Down Expand Up @@ -164,6 +165,7 @@ describe('authedPost', () => {
});
vi.stubGlobal('fetch', fetchMock);
const auth = makeAuth();
saveAuth(auth);
const res = await authedPost(auth, target, 'https://x.example/api/memories', { name: 'a' });
expect(res.status).toBe(201);
expect(auth.tokens.accessToken).toBe('new-token');
Expand Down Expand Up @@ -218,6 +220,7 @@ describe('introspectToken', () => {
});
vi.stubGlobal('fetch', fetchMock);
const auth = makeAuth();
saveAuth(auth);
const session = await introspectToken(auth, target);
expect(session.userId).toBe('u1');
expect(auth.tokens.accessToken).toBe('new-token');
Expand Down Expand Up @@ -362,3 +365,70 @@ describe('PAT-backed AuthState', () => {
expect(fetchMock).toHaveBeenCalledTimes(1);
});
});

// Issue #236: a sign-out that lands while a token refresh is in flight must stay signed out. The
// refresh may still use its fresh tokens in memory for the request already on the wire, but the
// null re-read is the authoritative "session ended" - it is never folded back onto disk.
describe('refreshOrThrow vs a concurrent sign-out', () => {
let dir: string;

beforeEach(() => {
dir = mkdtempSync(join(tmpdir(), 'agentage-api-'));
process.env['AGENTAGE_CONFIG_DIR'] = dir;
});

afterEach(() => {
delete process.env['AGENTAGE_CONFIG_DIR'];
rmSync(dir, { recursive: true, force: true });
vi.unstubAllGlobals();
});

const refreshOnce = (onTokenCall: () => Promise<void>): ReturnType<typeof vi.fn> =>
vi.fn(async (url: string) => {
if (!String(url).includes('/token')) return jsonResponse(200, {});
await onTokenCall(); // the sign-out lands while the refresh is on the wire
return jsonResponse(200, { access_token: 'new-token', expires_in: 60 });
});

it('does not resurrect auth.json when deleteAuth lands during the refresh', async () => {
const auth = makeAuth();
saveAuth(auth);
vi.stubGlobal(
'fetch',
refreshOnce(() => deleteAuth())
);
await refreshOrThrow(auth, target);
expect(readAuth()).toBeNull(); // signed out stays signed out
expect(auth.tokens.accessToken).toBe('new-token'); // in-flight request still retries
});

it('stays signed out across 50 sign-out/refresh races', async () => {
for (let i = 0; i < 50; i++) {
const auth = makeAuth();
saveAuth(auth);
let signOut: Promise<void> | undefined;
vi.stubGlobal(
'fetch',
refreshOnce(() => {
signOut = deleteAuth(); // raced, not awaited: either order must end signed out
return Promise.resolve();
})
);
await refreshOrThrow(auth, target);
await signOut;
expect(readAuth()).toBeNull();
}
});

it('still persists the refreshed tokens when no sign-out races it', async () => {
const auth = makeAuth();
saveAuth(auth);
vi.stubGlobal(
'fetch',
refreshOnce(() => Promise.resolve())
);
await refreshOrThrow(auth, target);
expect(readAuth()?.tokens.accessToken).toBe('new-token');
expect(readAuth()?.clientId).toBe('c1'); // other fields preserved
});
});
9 changes: 5 additions & 4 deletions src/lib/auth/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,12 @@ export const refreshOrThrow = async (auth: AuthState, links: Links): Promise<voi
};
auth.tokens = tokens; // the caller retries its request with this in-memory copy
// Persist under the lock, folding the new tokens onto the freshly-read state so a concurrent
// writer's other fields (clientId, siteFqdn) are not clobbered by a stale in-memory copy.
// writer's other fields (clientId, siteFqdn) are not clobbered by a stale in-memory copy. A null
// re-read means a sign-out won the race: skip the write rather than resurrect it (issue #236).
await mutateAuth((current) => {
const base = current ?? auth;
base.tokens = tokens;
return base;
if (!current) return null;
current.tokens = tokens;
return current;
});
};

Expand Down
24 changes: 21 additions & 3 deletions src/lib/fs/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
import { acquireFileLock, releaseFileLock } from './file-lock.js';
import {
deleteAuth,
ensureConfigDir,
Expand Down Expand Up @@ -57,10 +58,27 @@ describe('config store', () => {
expect(readAuth()).toBeNull();
});

it('deletes auth state idempotently', () => {
it('deletes auth state idempotently', async () => {
saveAuth(sample);
deleteAuth();
deleteAuth();
await deleteAuth();
await deleteAuth();
expect(readAuth()).toBeNull();
expect(readdirSync(getConfigDir()).filter((f) => f.endsWith('.lock'))).toEqual([]);
});

// Sign-out is serialized against mutateAuth by the same lock (issue #236): while a refresh holds
// it, deleteAuth waits instead of unlinking inside someone else's read-modify-write.
it('waits for a held lock before unlinking', async () => {
saveAuth(sample);
const target = join(getConfigDir(), 'auth.json');
expect(acquireFileLock(target)).toBe(true);
let done = false;
const pending = deleteAuth().then(() => void (done = true));
await new Promise((r) => setTimeout(r, 100));
expect(done).toBe(false);
expect(readAuth()).toEqual(sample); // untouched while the other writer holds the lock
releaseFileLock(target);
await pending;
expect(readAuth()).toBeNull();
});

Expand Down
9 changes: 7 additions & 2 deletions src/lib/fs/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,14 @@ export const saveAuth = (state: AuthState): void => {
renameSync(tmp, path);
};

export const deleteAuth = (): void => {
// Sign-out under the same lock mutateAuth takes (issue #236), so a concurrent token refresh can
// never slip its re-read+save between this existence check and the unlink.
export const deleteAuth = async (): Promise<void> => {
ensureConfigDir();
const path = authPath();
if (existsSync(path)) unlinkSync(path);
await withFileLock(path, () => {
if (existsSync(path)) unlinkSync(path);
});
};

// Cross-process-safe read-modify-write on auth.json (issue #231). Under the advisory lock, re-read
Expand Down