Skip to content
Merged
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
200 changes: 200 additions & 0 deletions test/rotation-proxy-state.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { AccountManager } from "../lib/accounts.js";
import type { RotationProxyStateInit } from "../lib/runtime/rotation-proxy-state.js";
import type { AccountStorageV3 } from "../lib/storage.js";

const { recordRuntimeResetMock, recordRuntimeReloadMock } = vi.hoisted(() => ({
recordRuntimeResetMock: vi.fn(),
recordRuntimeReloadMock: vi.fn(),
}));

vi.mock("../lib/runtime/runtime-observability.js", async (importOriginal) => {
const actual = await importOriginal<
typeof import("../lib/runtime/runtime-observability.js")
>();
return {
...actual,
recordRuntimeReset: recordRuntimeResetMock,
recordRuntimeReload: recordRuntimeReloadMock,
};
});

const { createRotationProxyState, recoverStaleRuntimeState } = await import(
"../lib/runtime/rotation-proxy-state.js"
);

const NOW = Date.now();

function storageWith(count: number): AccountStorageV3 {
return {
version: 3,
activeIndex: 0,
activeIndexByFamily: {},
accounts: Array.from({ length: count }, (_unused, index) => ({
email: `account-${index + 1}@example.com`,
accountId: `acc_${index + 1}`,
refreshToken: `refresh-${index + 1}`,
accessToken: `access-${index + 1}`,
expiresAt: NOW + 3_600_000,
addedAt: NOW - 60_000,
lastUsed: NOW - 60_000,
enabled: true,
})),
};
}

function stateInit(): RotationProxyStateInit {
return {
activeAccountManager: new AccountManager(undefined, storageWith(1)),
routingMutexMode: "enabled",
schedulingStrategy: "hybrid",
fetchImpl: fetch,
upstreamBaseUrl: "https://upstream.example",
clientApiKey: "key",
now: () => NOW,
tokenRefreshSkewMs: 30_000,
networkErrorCooldownMs: 10_000,
serverErrorCooldownMs: 10_000,
tokenInvalidationCooldownMs: 300_000,
minRotationIntervalMs: 0,
pidOffsetEnabled: false,
fetchTimeoutMs: 30_000,
streamStallTimeoutMs: 30_000,
maxRuntimeAccountAttempts: 3,
maxRequestBodyBytes: 1024,
quotaRemainingPercentThreshold: 0,
sessionAffinityStore: null,
lastObservedAffinityGeneration: 0,
};
}

let loadFromDisk: ReturnType<typeof vi.spyOn>;

beforeEach(() => {
vi.clearAllMocks();
loadFromDisk = vi.spyOn(AccountManager, "loadFromDisk");
});

afterEach(() => {
loadFromDisk.mockRestore();
});

describe("createRotationProxyState", () => {
it("seeds the known-manager set and a zeroed status from the injected clock", () => {
const init = stateInit();

const state = createRotationProxyState(init);

expect([...state.knownAccountManagers]).toEqual([
init.activeAccountManager,
]);
expect(state.status).toStrictEqual({
startedAt: NOW,
totalRequests: 0,
upstreamRequests: 0,
retries: 0,
rotations: 0,
streamsStarted: 0,
lastError: null,
lastAccountIndex: null,
lastAccountLabel: null,
lastAccountId: null,
lastAccountUpdatedAt: null,
});
Comment thread
greptile-apps[bot] marked this conversation as resolved.
expect(state.lastGlobalAccountIndex).toBeNull();
expect(state.lastStaleRuntimeReloadAt).toBe(0);
});
});

describe("recoverStaleRuntimeState", () => {
it("reloads from disk, swaps the active manager, and records observability", async () => {
const state = createRotationProxyState(stateInit());
const previousManager = state.activeAccountManager;
const reloaded = new AccountManager(undefined, storageWith(2));
loadFromDisk.mockResolvedValue(reloaded);

const result = await recoverStaleRuntimeState(state);

expect(result).toBe(reloaded);
expect(state.activeAccountManager).toBe(reloaded);
// The previous manager stays known so in-flight requests can finish.
Comment on lines +108 to +120

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 AccountManager.resetVolatileRuntimeState() runs for real in these tests

only loadFromDisk is spied on; resetVolatileRuntimeState is not mocked. that means resetTrackers() and resetAllCircuitBreakers() execute against the module-level health-tracker and circuit-breaker singletons on every happy-path and concurrent test invocation. within a single vitest worker the state is isolated per file, so this doesn't contaminate sibling test files, but it does silently mutate global state inside this worker. if a future test in this same file relies on a pre-seeded tracker or circuit-breaker state (e.g. to test recovery when an account is already cooling down), the reset will silently erase that setup. adding a vi.spyOn(AccountManager, 'resetVolatileRuntimeState').mockReturnValue(undefined) in beforeEach would make the seam explicit and match the spirit of the PR description ("nothing touches disk").

Prompt To Fix With AI
This is a comment left during a code review.
Path: test/rotation-proxy-state.test.ts
Line: 105-117

Comment:
**`AccountManager.resetVolatileRuntimeState()` runs for real in these tests**

only `loadFromDisk` is spied on; `resetVolatileRuntimeState` is not mocked. that means `resetTrackers()` and `resetAllCircuitBreakers()` execute against the module-level health-tracker and circuit-breaker singletons on every happy-path and concurrent test invocation. within a single vitest worker the state is isolated per file, so this doesn't contaminate sibling test files, but it does silently mutate global state inside this worker. if a future test in this same file relies on a pre-seeded tracker or circuit-breaker state (e.g. to test recovery when an account is already cooling down), the reset will silently erase that setup. adding a `vi.spyOn(AccountManager, 'resetVolatileRuntimeState').mockReturnValue(undefined)` in `beforeEach` would make the seam explicit and match the spirit of the PR description ("nothing touches disk").

How can I resolve this? If you propose a fix, please make it concise.

Fix in Codex

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining this one: the resetVolatileRuntimeState() call inside recoverStaleRuntimeState is part of the recovery contract under test (the pool-exhausted reset of trackers and breakers is deliberate product behavior, paired with the recordRuntimeReset marker the test does assert). Stubbing the static would also stub that in-recovery call and hollow out the function being tested. The "nothing touches disk" claim still holds — these singletons are in-memory — and cross-test isolation is already guaranteed by the forks pool plus the explicit beforeEach reset. If a future test in this file needs pre-seeded tracker state, scoping a spy inside that one test is the right tool at that point.


Generated by Claude Code

expect(state.knownAccountManagers.has(previousManager)).toBe(true);
expect(state.knownAccountManagers.has(reloaded)).toBe(true);
// The configured mutex mode carries over to the reloaded pool.
expect(reloaded.getRoutingMutexMode()).toBe("enabled");
expect(state.lastStaleRuntimeReloadAt).toBeGreaterThan(0);
expect(recordRuntimeResetMock).toHaveBeenCalledExactlyOnceWith(
"pool-exhausted-no-account",
);
expect(recordRuntimeReloadMock).toHaveBeenCalledExactlyOnceWith(
"pool-exhausted-no-account",
);
});

it("dedupes within the 1s window and reloads again once it expires", async () => {
// The dedupe guard runs on the real wall clock (deliberately not the
// injected now()), so fake timers make the window deterministic.
vi.useFakeTimers();
try {
const state = createRotationProxyState(stateInit());
const reloaded = new AccountManager(undefined, storageWith(2));
loadFromDisk.mockResolvedValue(reloaded);

await recoverStaleRuntimeState(state);
const second = await recoverStaleRuntimeState(state);

// The second call lands inside the 1s dedupe window: no second reload.
expect(second).toBe(reloaded);
expect(loadFromDisk).toHaveBeenCalledTimes(1);

// Once the window expires, the next call reloads from disk again.
vi.advanceTimersByTime(1_001);
const freshest = new AccountManager(undefined, storageWith(3));
loadFromDisk.mockResolvedValue(freshest);
await expect(recoverStaleRuntimeState(state)).resolves.toBe(freshest);
expect(loadFromDisk).toHaveBeenCalledTimes(2);
} finally {
vi.useRealTimers();
}
});

it("shares one in-flight reload between concurrent callers", async () => {
const state = createRotationProxyState(stateInit());
const reloaded = new AccountManager(undefined, storageWith(2));
let releaseReload!: () => void;
const gate = new Promise<void>((resolve) => {
releaseReload = resolve;
});
loadFromDisk.mockImplementation(async () => {
await gate;
return reloaded;
});

const firstPending = recoverStaleRuntimeState(state);
const secondPending = recoverStaleRuntimeState(state);
releaseReload();
const [first, second] = await Promise.all([firstPending, secondPending]);

expect(first).toBe(reloaded);
expect(second).toBe(reloaded);
expect(loadFromDisk).toHaveBeenCalledTimes(1);
});

it("reports a failed reload and allows the next attempt to retry", async () => {
const state = createRotationProxyState(stateInit());
loadFromDisk.mockRejectedValueOnce(new Error("disk exploded"));

const failed = await recoverStaleRuntimeState(state);

expect(failed).toBeNull();
Comment thread
greptile-apps[bot] marked this conversation as resolved.
expect(state.status.lastError).toBe("disk exploded");
// The failure must not arm the dedupe window.
expect(state.lastStaleRuntimeReloadAt).toBe(0);
// The failure does not arm the dedupe window or leak a stale promise:
// the next call retries the reload.
const reloaded = new AccountManager(undefined, storageWith(2));
loadFromDisk.mockResolvedValue(reloaded);
await expect(recoverStaleRuntimeState(state)).resolves.toBe(reloaded);
expect(loadFromDisk).toHaveBeenCalledTimes(2);
});
});