Skip to content

Commit e861d3d

Browse files
ndycodendycode
committed
fix: address remaining PR 387 bug findings
1 parent a15fcdc commit e861d3d

8 files changed

Lines changed: 114 additions & 77 deletions

‎lib/auto-update-checker.ts‎

Lines changed: 58 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,52 @@ interface ParsedSemver {
3030
const RETRYABLE_WRITE_ERRORS = new Set(["EBUSY", "EPERM"]);
3131
let updateCacheWriteQueue: Promise<void> = Promise.resolve();
3232

33+
function enqueueUpdateCacheWrite(writeTask: () => void): Promise<void> {
34+
const queued = updateCacheWriteQueue.catch(() => undefined).then(writeTask);
35+
updateCacheWriteQueue = queued.then(
36+
() => undefined,
37+
() => undefined,
38+
);
39+
return queued;
40+
}
41+
42+
function writeCacheContents(serialized: string): void {
43+
let tempPath: string | null = null;
44+
let wroteTemp = false;
45+
try {
46+
if (!existsSync(CACHE_DIR)) {
47+
mkdirSync(CACHE_DIR, { recursive: true });
48+
}
49+
tempPath = `${CACHE_FILE}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`;
50+
let lastError: Error | null = null;
51+
for (let attempt = 0; attempt < 4; attempt++) {
52+
try {
53+
writeFileSync(tempPath, serialized, "utf8");
54+
renameSync(tempPath, CACHE_FILE);
55+
wroteTemp = false;
56+
return;
57+
} catch (error) {
58+
const code = (error as NodeJS.ErrnoException).code ?? "";
59+
lastError = error as Error;
60+
wroteTemp = true;
61+
if (!RETRYABLE_WRITE_ERRORS.has(code) || attempt >= 3) {
62+
throw error;
63+
}
64+
sleepSync(15 * (2 ** attempt));
65+
}
66+
}
67+
if (lastError) throw lastError;
68+
} finally {
69+
if (wroteTemp && tempPath) {
70+
try {
71+
unlinkSync(tempPath);
72+
} catch {
73+
// Best effort temp cleanup.
74+
}
75+
}
76+
}
77+
}
78+
3379
function sleepSync(ms: number): void {
3480
const delay = Math.max(0, Math.floor(ms));
3581
if (delay === 0) return;
@@ -64,52 +110,13 @@ function loadCache(): UpdateCheckCache | null {
64110
}
65111

66112
async function saveCache(cache: UpdateCheckCache): Promise<void> {
67-
const writeTask = (): void => {
68-
let tempPath: string | null = null;
69-
let wroteTemp = false;
113+
await enqueueUpdateCacheWrite(() => {
70114
try {
71-
if (!existsSync(CACHE_DIR)) {
72-
mkdirSync(CACHE_DIR, { recursive: true });
73-
}
74-
const serialized = JSON.stringify(cache, null, 2);
75-
tempPath = `${CACHE_FILE}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`;
76-
let lastError: Error | null = null;
77-
for (let attempt = 0; attempt < 4; attempt++) {
78-
try {
79-
writeFileSync(tempPath, serialized, "utf8");
80-
renameSync(tempPath, CACHE_FILE);
81-
wroteTemp = false;
82-
return;
83-
} catch (error) {
84-
const code = (error as NodeJS.ErrnoException).code ?? "";
85-
lastError = error as Error;
86-
wroteTemp = true;
87-
if (!RETRYABLE_WRITE_ERRORS.has(code) || attempt >= 3) {
88-
throw error;
89-
}
90-
sleepSync(15 * (2 ** attempt));
91-
}
92-
}
93-
if (lastError) throw lastError;
115+
writeCacheContents(JSON.stringify(cache, null, 2));
94116
} catch (error) {
95117
log.warn("Failed to save update cache", { error: (error as Error).message });
96-
} finally {
97-
if (wroteTemp && tempPath) {
98-
try {
99-
unlinkSync(tempPath);
100-
} catch {
101-
// Best effort temp cleanup.
102-
}
103-
}
104118
}
105-
};
106-
107-
const queued = updateCacheWriteQueue.catch(() => undefined).then(writeTask);
108-
updateCacheWriteQueue = queued.then(
109-
() => undefined,
110-
() => undefined,
111-
);
112-
await queued;
119+
});
113120
}
114121

115122
function parseSemver(version: string): ParsedSemver {
@@ -280,11 +287,13 @@ export async function checkAndNotify(
280287
}
281288

282289
export function clearUpdateCache(): void {
283-
try {
284-
if (existsSync(CACHE_FILE)) {
285-
writeFileSync(CACHE_FILE, "{}", "utf8");
286-
}
287-
} catch {
288-
// Ignore errors
289-
}
290+
void enqueueUpdateCacheWrite(() => {
291+
try {
292+
if (existsSync(CACHE_FILE)) {
293+
writeCacheContents("{}");
294+
}
295+
} catch {
296+
// Ignore errors
297+
}
298+
});
290299
}

‎lib/codex-manager.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -780,10 +780,11 @@ function resolveMenuQuotaProbeInput(
780780
if (account.enabled === false) return null;
781781
if (!hasUsableAccessToken(account, now)) return null;
782782

783-
const existing = getQuotaCacheEntryForAccount(
783+
const existing = getPersistedQuotaViewForAccount(
784784
cache,
785785
account,
786786
accounts,
787+
now,
787788
emailFallbackState,
788789
);
789790
if (
@@ -946,6 +947,7 @@ function mapAccountStatus(
946947
index: number,
947948
activeIndex: number,
948949
now: number,
950+
persistedQuotaStatus?: number,
949951
): ExistingAccountInfo["status"] {
950952
if (account.enabled === false) return "disabled";
951953
if (
@@ -954,6 +956,7 @@ function mapAccountStatus(
954956
) {
955957
return "cooldown";
956958
}
959+
if (persistedQuotaStatus === 429) return "rate-limited";
957960
const rateLimit = formatRateLimitEntry(account, now, "codex");
958961
if (rateLimit) return "rate-limited";
959962
if (index === activeIndex) return "active";
@@ -1114,7 +1117,7 @@ function toExistingAccountInfo(
11141117
email: account.email,
11151118
addedAt: account.addedAt,
11161119
lastUsed: account.lastUsed,
1117-
status: mapAccountStatus(account, index, activeIndex, now),
1120+
status: mapAccountStatus(account, index, activeIndex, now, entry?.status),
11181121
quotaSummary:
11191122
(displaySettings.menuShowQuotaSummary ?? true) && entry
11201123
? formatAccountQuotaSummary(entry)

‎lib/config.ts‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1842,16 +1842,8 @@ export function getPluginConfigExplainReport(): ConfigExplainReport {
18421842
const stored = resolveStoredPluginConfigRecord();
18431843
const storedRecord = stored.record ?? null;
18441844
const entries = CONFIG_EXPLAIN_ENTRIES.map((entry) => {
1845-
const rawValue = entry.getValue(pluginConfig);
1846-
const rawDefaultValue = DEFAULT_PLUGIN_CONFIG[entry.key];
1847-
const value =
1848-
entry.key === "retryAllAccountsMaxRetries" && rawValue === 0
1849-
? Number.POSITIVE_INFINITY
1850-
: rawValue;
1851-
const defaultValue =
1852-
entry.key === "retryAllAccountsMaxRetries" && rawDefaultValue === 0
1853-
? Number.POSITIVE_INFINITY
1854-
: rawDefaultValue;
1845+
const value = entry.getValue(pluginConfig);
1846+
const defaultValue = DEFAULT_PLUGIN_CONFIG[entry.key];
18551847
return {
18561848
key: entry.key,
18571849
value: normalizeConfigExplainValue(value),

‎lib/request/fetch-helpers.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,9 +1004,14 @@ function mapUsageLimit404WithBody(response: Response, bodyText: string): Respons
10041004
}
10051005

10061006
// Only structured quota-limit codes should be remapped from 404 to 429.
1007-
// Free-text 404 bodies and generic rate_limit_* codes are too ambiguous and
1008-
// degrade downstream rate-limit reason classification to "unknown".
1009-
if (!normalizedSignals.some((value) => value.includes("usage_limit"))) {
1007+
// Free-text 404 bodies remain untouched, but known quota/rate-limit codes
1008+
// should still preserve retry semantics for callers.
1009+
if (
1010+
!normalizedSignals.some(
1011+
(value) =>
1012+
value.includes("usage_limit") || value.includes("rate_limit_exceeded"),
1013+
)
1014+
) {
10101015
return null;
10111016
}
10121017

‎test/auto-update-checker.test.ts‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -602,14 +602,20 @@ describe("auto-update-checker", () => {
602602
it("writes empty object when cache exists", () => {
603603
vi.mocked(fs.existsSync).mockReturnValue(true);
604604
vi.mocked(fs.writeFileSync).mockClear();
605+
vi.mocked(fs.renameSync).mockClear();
605606

606607
clearUpdateCache();
607608

608-
expect(fs.writeFileSync).toHaveBeenCalledWith(
609-
expect.stringContaining("update-check-cache.json"),
610-
"{}",
611-
"utf8"
612-
);
609+
return Promise.resolve().then(() => {
610+
return Promise.resolve().then(() => {
611+
expect(fs.writeFileSync).toHaveBeenCalledWith(
612+
expect.stringContaining("update-check-cache.json."),
613+
"{}",
614+
"utf8"
615+
);
616+
expect(fs.renameSync).toHaveBeenCalled();
617+
});
618+
});
613619
});
614620

615621
it("does nothing when cache does not exist", () => {
@@ -620,5 +626,24 @@ describe("auto-update-checker", () => {
620626

621627
expect(fs.writeFileSync).not.toHaveBeenCalled();
622628
});
629+
630+
it("keeps clearUpdateCache ordered after the latest save", async () => {
631+
vi.mocked(fs.existsSync).mockReturnValue(true);
632+
vi.mocked(fs.writeFileSync).mockClear();
633+
vi.mocked(fs.renameSync).mockClear();
634+
vi.mocked(globalThis.fetch).mockResolvedValue({
635+
ok: true,
636+
json: async () => ({ version: "5.0.0" }),
637+
} as Response);
638+
639+
await checkForUpdates(true);
640+
clearUpdateCache();
641+
await Promise.resolve();
642+
await Promise.resolve();
643+
644+
const writes = vi.mocked(fs.writeFileSync).mock.calls;
645+
expect(writes).toHaveLength(2);
646+
expect(writes.at(-1)?.[1]).toBe("{}");
647+
});
623648
});
624649
});

‎test/codex-manager-cli.test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7189,11 +7189,14 @@ describe("codex manager cli commands", () => {
71897189
quotaRateLimited?: boolean;
71907190
quota5hResetAtMs?: number;
71917191
quotaSummary?: string;
7192+
status?: string;
71927193
}>;
71937194
expect(firstCallAccounts[0]?.email).toBe("rate-limited@example.com");
71947195
expect(firstCallAccounts[0]?.quotaRateLimited).toBe(true);
71957196
expect(firstCallAccounts[0]?.quota5hResetAtMs).toBe(now + 60_000);
71967197
expect(firstCallAccounts[0]?.quotaSummary).toBe("rate-limited");
7198+
expect(firstCallAccounts[0]?.status).toBe("rate-limited");
7199+
expect(fetchCodexQuotaSnapshotMock).not.toHaveBeenCalled();
71977200
});
71987201

71997202
it("treats accounts with no quota windows as the lowest ready-first floor", async () => {

‎test/config-explain.test.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,21 +128,21 @@ describe("getPluginConfigExplainReport", () => {
128128
expect(fallback?.source).not.toBe("file");
129129
});
130130

131-
it("normalizes non-finite values for json-safe output", async () => {
131+
it("reports retry-all max retries honestly for json-safe output", async () => {
132132
const { getPluginConfigExplainReport } = await import("../lib/config.js");
133133
const report = getPluginConfigExplainReport();
134134
const entry = expectEntry(report, "retryAllAccountsMaxRetries");
135-
expect(entry?.value).toBe("Infinity");
136-
expect(entry?.defaultValue).toBe("Infinity");
135+
expect(entry?.value).toBe(0);
136+
expect(entry?.defaultValue).toBe(0);
137137
const serialized = JSON.parse(JSON.stringify(report)) as {
138138
entries: Array<{ key: string; value: unknown; defaultValue: unknown }>;
139139
};
140140
const serializedEntry = serialized.entries.find(
141141
(item) => item.key === "retryAllAccountsMaxRetries",
142142
);
143143
expect(serializedEntry).toMatchObject({
144-
value: "Infinity",
145-
defaultValue: "Infinity",
144+
value: 0,
145+
defaultValue: 0,
146146
});
147147
});
148148

‎test/fetch-helpers.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,7 +1036,7 @@ describe('createEntitlementErrorResponse', () => {
10361036
expect(rateLimit).toBeUndefined();
10371037
});
10381038

1039-
it('does not remap 404s with generic rate_limit_exceeded codes', async () => {
1039+
it('remaps structured 404s with rate_limit_exceeded codes', async () => {
10401040
const response = new Response(
10411041
JSON.stringify({
10421042
error: {
@@ -1049,8 +1049,8 @@ describe('createEntitlementErrorResponse', () => {
10491049

10501050
const { response: result, rateLimit } = await handleErrorResponse(response);
10511051

1052-
expect(result.status).toBe(404);
1053-
expect(rateLimit).toBeUndefined();
1052+
expect(result.status).toBe(429);
1053+
expect(rateLimit?.retryAfterMs).toBeGreaterThan(0);
10541054
});
10551055

10561056
it('does not treat non-429 rate-limit text as a cooldown signal', async () => {

0 commit comments

Comments
 (0)