feat(sync): add result history and last-result log#110
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughintroduces a robust ndjson-based sync history system with concurrency control and retry logic. codex cli sync and live account sync now record operations to persistent history. new async loader reconstructs last sync run from history, enabling state recovery across process restarts. Changes
Sequence Diagram(s)sequenceDiagram
participant CodexCli as Codex CLI Sync
participant LiveSync as Live Account Sync
participant History as Sync History
participant FileSystem as Filesystem
CodexCli->>History: publishCodexCliSyncRun()
History->>History: withHistoryLock()
History->>FileSystem: append entry (ndjson)
History->>FileSystem: update latest snapshot
History->>FileSystem: trimHistoryIfNeeded()
FileSystem-->>History: done
History-->>CodexCli: entry recorded
LiveSync->>History: recordHistory(reason, outcome)
History->>History: toHistorySnapshot()
History->>History: withHistoryLock()
History->>FileSystem: append entry (ndjson)
History->>FileSystem: update latest snapshot
FileSystem-->>History: done
History-->>LiveSync: recorded
CodexCli->>History: loadLastCodexCliSyncRun()
History->>FileSystem: readLatestSyncHistory()
FileSystem-->>History: latest entry
History->>History: hydrateLastCodexCliSyncRunFromHistory()
History-->>CodexCli: CodexCliSyncRun | null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes notes on reviewconcurrency risks flagged:
missing regression coverage:
windows edge cases:
other observations:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds persisted sync result history (NDJSON + latest snapshot) and integrates history logging into Codex CLI sync and live account sync, with tests validating bounded tail reads and recovery behavior.
Changes:
- Introduces
lib/sync-history.tsfor appending, trimming, and reading sync history (including a tail-reader for bounded reads). - Logs Codex CLI sync runs and live-account sync outcomes to history and hydrates “last run” from history when missing.
- Extends existing Vitest suites and adds a new sync-history-focused test file.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/sync-history.test.ts | New tests validating bounded tail reads and recovery of last Codex CLI sync run from history. |
| test/live-account-sync.test.ts | Updates tests to configure/reset history and assert live sync history entries are recorded. |
| test/codex-cli-sync.test.ts | Updates tests to configure/reset history and assert Codex CLI sync history persistence/hydration. |
| lib/sync-history.ts | New sync history module (append/read/latest snapshot, tail read, trimming, test hooks). |
| lib/live-account-sync.ts | Records live sync successes/errors into sync history with a snapshot tailored for history. |
| lib/codex-cli/sync.ts | Appends Codex CLI sync runs to history and hydrates last run from latest/tail history. |
| lib/index.ts | Re-exports sync-history from the library entrypoint. |
Comments suppressed due to low confidence (1)
lib/live-account-sync.ts:1
basenamedoes not acceptnull; ifsummarizeWatchPathis ever called withnullthis will throw at runtime (and should also fail typecheck unless the call sites guarantee non-null). Add a null guard (returning a placeholder like"<unknown>") or change the function signature topath: stringand ensure callers never pass null.
import { type FSWatcher, promises as fs, watch as fsWatch } from "node:fs";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| export function configureSyncHistoryForTests(directory: string | null): void { | ||
| historyDirOverride = directory ? directory.trim() : null; |
| export * from "./parallel-probe.js"; | ||
| export * from "./session-affinity.js"; | ||
| export * from "./live-account-sync.js"; | ||
| export * from "./sync-history.js"; |
| const reload = vi.fn(async () => { | ||
| throw new Error("reload failed"); | ||
| }); | ||
| const appendSpy = vi.spyOn(syncHistory, "appendSyncHistoryEntry"); |
| sync.stop(); | ||
| appendSpy.mockRestore(); |
| async function trimHistoryFileIfNeeded(paths: SyncHistoryPaths): Promise<void> { | ||
| const content = await fs.readFile(paths.historyPath, "utf8").catch((error) => { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === "ENOENT") { | ||
| return ""; | ||
| } | ||
| throw error; | ||
| }); | ||
| if (!content) { | ||
| return; | ||
| } | ||
| const lines = content.split(/\r?\n/).filter(Boolean); | ||
| if (lines.length <= MAX_HISTORY_ENTRIES) { | ||
| return; | ||
| } | ||
| const trimmedContent = `${lines.slice(-MAX_HISTORY_ENTRIES).join("\n")}\n`; | ||
| await fs.writeFile(paths.historyPath, trimmedContent, { | ||
| encoding: "utf8", | ||
| mode: 0o600, | ||
| }); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-cli/sync.ts`:
- Around line 242-272: The race comes from checking
lastCodexCliSyncHistoryLoadAttempted then setting it after a small window; to
fix, make the check-and-set atomic inside loadLastCodexCliSyncRun by assigning
lastCodexCliSyncHistoryLoadAttempted = true immediately before (or as part of)
creating lastCodexCliSyncRunLoadPromise so no two callers can both pass the
check; update the function so the guard is: if
(!lastCodexCliSyncHistoryLoadAttempted) { lastCodexCliSyncHistoryLoadAttempted =
true; lastCodexCliSyncRunLoadPromise = (async () => { ... })().catch(() =>
null).finally(() => { lastCodexCliSyncRunLoadPromise = null; }); } and leave the
rest of the logic (awaiting lastCodexCliSyncRunLoadPromise and cloning via
cloneCodexCliSyncRun/hydrateLastCodexCliSyncRunFromHistory) unchanged.
In `@lib/sync-history.ts`:
- Around line 300-314: Remove the dead, blocking function
readLatestSyncHistorySync from lib/sync-history.ts: delete the entire function
definition and any export referencing readLatestSyncHistorySync, confirm there
are zero usages (search for readLatestSyncHistorySync) and run typechecks/tests
to ensure nothing else imports it; since it performs synchronous fs IO, do not
replace it—simply remove the symbol and related references to avoid blocking the
event loop.
In `@test/codex-cli-sync.test.ts`:
- Around line 1332-1342: Add a regression test that simulates transient
filesystem contention by mocking the appendFile call used by
appendSyncHistoryEntry (lib/sync-history.ts) to throw an EBUSY/EPERM/EAGAIN
error for the first N attempts and succeed thereafter, then invoke
commitPendingCodexCliSyncRun (lib/codex-cli/sync.ts) so the call path goes
through retryHistoryWrite (lib/sync-history.ts) and finally assert the history
was persisted/recoverable (e.g., use readSyncHistory/loadLastCodexCliSyncRun as
in the existing test); specifically, spy/mock fs.promises.appendFile (or the
wrapper used by appendSyncHistoryEntry) to fail twice with {code: 'EBUSY'} then
resolve, run the code that calls appendSyncHistoryEntry, and assert the retry
logic retried and the final persisted run outcome/summary matches expectations.
In `@test/sync-history.test.ts`:
- Around line 174-211: Add three regression tests: (1) for retry behavior, mock
node fs operations used by retryHistoryWrite to throw transient errors
EBUSY/EAGAIN/EPERM on the first N attempts and succeed thereafter, assert that
retryHistoryWrite was retried the expected number of times and that exponential
backoff delays (or at least increasing retry intervals) occurred; (2) for
concurrency serialization, fire multiple concurrent appendSyncHistoryEntry calls
and assert via withHistoryLock or by checking persisted history order/length
that appends were serialized (no lost/raced entries) and that withHistoryLock
was used to serialize; (3) for Windows filesystem edge cases, simulate
Windows-specific EBUSY/EAGAIN behavior during file removal in
__resetSyncHistoryForTests and during appends (mocking fs.unlink/fs.writeFile to
throw platform-specific errors) and assert cleanup and append logic handle
retries and do not leave corrupt state. Use spies/mocks for the specific
functions (retryHistoryWrite, withHistoryLock, appendSyncHistoryEntry,
__resetSyncHistoryForTests) to inject errors and verify expected
retries/serialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44978562-2ee0-49c2-a1bd-33d7a688275e
📒 Files selected for processing (7)
lib/codex-cli/sync.tslib/index.tslib/live-account-sync.tslib/sync-history.tstest/codex-cli-sync.test.tstest/live-account-sync.test.tstest/sync-history.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/index.tslib/live-account-sync.tslib/sync-history.tslib/codex-cli/sync.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-cli-sync.test.tstest/sync-history.test.tstest/live-account-sync.test.ts
🔇 Additional comments (23)
lib/index.ts (1)
20-20: lgtm, but note that test-only helpers are now public api.the wildcard re-export exposes
configureSyncHistoryForTests,__resetSyncHistoryForTests,__getLastSyncHistoryErrorForTests, and__getLastSyncHistoryPathsForTestsfromlib/sync-history.ts:322-361. the__prefix convention signals internal/test use, so this is acceptable. tests intest/codex-cli-sync.test.ts,test/live-account-sync.test.ts, andtest/sync-history.test.tsdepend on these exports.test/codex-cli-sync.test.ts (2)
77-78: proper test isolation for sync history.configuring and resetting sync history in
beforeEachensures each test starts with a clean slate. good practice.
111-112: cleanup correctly resets sync history state.teardown properly resets history and clears the directory override. this prevents test pollution.
lib/live-account-sync.ts (4)
115-122: good path redaction before persisting history.
toHistorySnapshotreduces the full path tobasename, preventing leakage of sensitive directory structures in history entries. this aligns with the coding guidelines about not logging tokens or emails.
243-264: fire-and-forget history recording is intentional but worth noting.
recordHistorycatches errors and logs at debug level, which is appropriate for non-critical telemetry. the fire-and-forget pattern ensures history failures don't break the main reload flow. concurrency is safe becauseappendSyncHistoryEntryuseswithHistoryLockinternally (lib/sync-history.ts:72-79).test coverage exists at
test/live-account-sync.test.ts:229-271.
348-348: history recorded on successful reload.the
voidprefix makes this fire-and-forget, which is correct since history recording shouldn't block the reload flow.
358-362: history recorded on reload failure with error message.captures the error message in the history entry for debugging purposes. the error message comes from the caught error, not from sensitive fields.
test/live-account-sync.test.ts (4)
67-68: proper test isolation for sync history in live-account-sync tests.configuring and resetting sync history ensures each test starts clean.
81-82: cleanup correctly resets sync history state.proper teardown prevents test pollution.
229-271: good coverage for history recording on reload failure.the test spies on
appendSyncHistoryEntry, triggers a reload failure, and verifies the history entry has the correctkind,outcome,reason,path, andsnapshot.pathfields. spy is properly restored at line 271.
388-395: verifies success outcome in history after multiple reloads.checks that the latest history entry has
outcome: "success"after the follow-up reload completes.lib/codex-cli/sync.ts (3)
178-186: fire-and-forget history append with proper error handling.the
.catch()logs errors at debug level without breaking the main flow. no sensitive fields are logged. concurrency is safe becauseappendSyncHistoryEntryuseswithHistoryLockinternally.
315-316: test reset properly clears new state variables.resetting
lastCodexCliSyncHistoryLoadAttemptedandlastCodexCliSyncRunLoadPromiseensures test isolation for the new loading logic.
691-691: preview now awaits async history load for cold-start recovery.this adds I/O to the preview path but is necessary to populate
lastSyncfrom persisted history when the in-memory state is empty. acceptable tradeoff for the recovery feature.test/sync-history.test.ts (3)
47-63: proper test setup and teardown with temp directory isolation.creates a temp directory, configures sync history to use it, and cleans up after each test. good practice for deterministic tests.
65-136: good test for bounded tail-reading optimization.verifies that
readSyncHistorywithlimit: 1usesreadHistoryTailinstead of loading the entire file viareadFile. the spy assertion at line 135 confirms the optimization works.
138-172: good coverage for cold-start recovery path.tests that
loadLastCodexCliSyncRuncan recover the last run from the ndjson history file when the latest snapshot file is deleted. this covers the fallback path inlib/codex-cli/sync.ts:259-263.lib/sync-history.ts (6)
72-79: correct single-process mutex implementation.
withHistoryLockchains promises to serialize operations within a single node process. this doesn't protect against multi-process races, but that's acceptable for history logs where occasional duplicates are tolerable.
90-105: good retry logic for windows transient locks.
retryHistoryWritehandles EBUSY/EPERM/EAGAIN with exponential backoff (25ms → 50ms → 100ms → 200ms, 5 attempts). this aligns with the coding guidelines for windows filesystem IO.per pr known issues, this path lacks explicit vitest coverage. see
test/sync-history.test.tsreview comment.
133-186: efficient bounded tail-reading for history.
readHistoryTailreads 8KB chunks from the end of the file, avoiding full-file reads for large histories. the implementation correctly handles:
- empty files (line 141-143)
- partial lines spanning chunks (lines 155-157)
- kind filtering (line 164)
- early exit when limit is reached (lines 166-168)
188-211: history trimming to bound file size.
trimHistoryFileIfNeededrewrites the history file to the last 200 entries when it exceeds the cap. the trim logic reads 201 entries to detect overflow and only rewrites if necessary.one note: if
retryHistoryWritefails on the rewrite (line 205-210), the history file could be left in an inconsistent state. consider catching and logging the error without throwing, since trim is best-effort.
213-247: correct append flow with lock, retry, trim, and latest update.the sequence is:
- acquire lock
- ensure directory exists
- append entry with retry
- trim if oversized
- write latest snapshot with retry
pendingHistoryWritestracking enableswaitForPendingHistoryWritesto wait for in-flight writes before reads.
86-88: file permissions note for cross-platform.
mode: 0o700for directories andmode: 0o600for files are good security defaults on unix. on windows, these modes are largely ignored, but node doesn't throw. no issue here, just noting the cross-platform behavior.
| export function readLatestSyncHistorySync(): SyncHistoryEntry | null { | ||
| try { | ||
| const content = readFileSync(getSyncHistoryPaths().latestPath, "utf8"); | ||
| const parsed = parseEntry(content); | ||
| return parsed ? cloneEntry(parsed) : null; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.debug("Failed to read latest sync history", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if readLatestSyncHistorySync is used anywhere
rg -n "readLatestSyncHistorySync" --type=ts lib/ test/Repository: ndycode/codex-multi-auth
Length of output: 162
🏁 Script executed:
# Additional check: verify no indirect calls via string lookups
rg -n "\"readLatestSyncHistorySync\"|'readLatestSyncHistorySync'" --type=ts lib/ test/Repository: ndycode/codex-multi-auth
Length of output: 50
remove readLatestSyncHistorySync from lib/sync-history.ts:300-314—it's dead code with zero usages.
verified this function isn't called anywhere in lib/ or test/. it also blocks the event loop via readFileSync, which violates guidelines for lib/** around filesystem io patterns. no initialization logic depends on it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sync-history.ts` around lines 300 - 314, Remove the dead, blocking
function readLatestSyncHistorySync from lib/sync-history.ts: delete the entire
function definition and any export referencing readLatestSyncHistorySync,
confirm there are zero usages (search for readLatestSyncHistorySync) and run
typechecks/tests to ensure nothing else imports it; since it performs
synchronous fs IO, do not replace it—simply remove the symbol and related
references to avoid blocking the event loop.
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); | ||
| const lastHistory = history.at(-1); | ||
| expect(lastHistory?.kind).toBe("codex-cli-sync"); | ||
| if (lastHistory?.kind === "codex-cli-sync") { | ||
| expect(lastHistory.run.outcome).toBe("changed"); | ||
| expect(lastHistory.run.summary.addedAccountCount).toBe(1); | ||
| } | ||
| __resetLastCodexCliSyncRunForTests(); | ||
| const persisted = await loadLastCodexCliSyncRun(); | ||
| expect(persisted?.outcome).toBe("changed"); | ||
| expect(persisted?.summary.addedAccountCount).toBe(1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if any test explicitly covers retryHistoryWrite with EBUSY/EPERM injection
rg -n "retryHistoryWrite|EBUSY.*append|EPERM.*append" --type=ts test/Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Find where retryHistoryWrite is defined
rg -n "retryHistoryWrite" --type=ts lib/Repository: ndycode/codex-multi-auth
Length of output: 339
🏁 Script executed:
# Find appendSyncHistoryEntry and commitPendingCodexCliSyncRun functions
rg -n "appendSyncHistoryEntry|commitPendingCodexCliSyncRun" --type=ts lib/Repository: ndycode/codex-multi-auth
Length of output: 750
🏁 Script executed:
# Check test file context around lines 1332-1342
sed -n '1320,1350p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1262
🏁 Script executed:
# Search for any EBUSY or EPERM error handling in lib
rg -n "EBUSY|EPERM" --type=ts lib/Repository: ndycode/codex-multi-auth
Length of output: 1946
🏁 Script executed:
# Get the retryHistoryWrite function implementation
sed -n '90,120p' lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 858
🏁 Script executed:
# Search for any test error injection helpers or mocks for appendFile
rg -n "appendFile.*mock|mock.*appendFile|vi\.mock|afterEach.*jest" --type=ts test/ | head -20Repository: ndycode/codex-multi-auth
Length of output: 1627
🏁 Script executed:
# Check if there are any tests that inject filesystem errors
rg -n "reject|throws|EBUSY|EPERM" --type=ts test/codex-cli-sync.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 434
solid integration test for history persistence and recovery, but add regression test for retry logic.
verifies that appendSyncHistoryEntry (lib/sync-history.ts:213) is called during commitPendingCodexCliSyncRun (lib/codex-cli/sync.ts:274) and that loadLastCodexCliSyncRun can recover the run from persisted history after resetting in-memory state. good coverage for the cold-start recovery path.
however, retryHistoryWrite (lib/sync-history.ts:90) wraps all history writes with exponential backoff retry logic for EBUSY/EPERM/EAGAIN, but this test never exercises that retry path. add a regression test that injects transient EBUSY during the appendFile call inside appendSyncHistoryEntry to confirm the retry logic handles transient fs contention, especially on windows where EPERM can occur during concurrent file access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-cli-sync.test.ts` around lines 1332 - 1342, Add a regression test
that simulates transient filesystem contention by mocking the appendFile call
used by appendSyncHistoryEntry (lib/sync-history.ts) to throw an
EBUSY/EPERM/EAGAIN error for the first N attempts and succeed thereafter, then
invoke commitPendingCodexCliSyncRun (lib/codex-cli/sync.ts) so the call path
goes through retryHistoryWrite (lib/sync-history.ts) and finally assert the
history was persisted/recoverable (e.g., use
readSyncHistory/loadLastCodexCliSyncRun as in the existing test); specifically,
spy/mock fs.promises.appendFile (or the wrapper used by appendSyncHistoryEntry)
to fail twice with {code: 'EBUSY'} then resolve, run the code that calls
appendSyncHistoryEntry, and assert the retry logic retried and the final
persisted run outcome/summary matches expectations.
| export async function readLatestSyncHistory(): Promise<SyncHistoryEntry | null> { | ||
| await waitForPendingHistoryWrites(); | ||
| try { | ||
| const content = await fs.readFile(getSyncHistoryPaths().latestPath, "utf8"); | ||
| const parsed = parseEntry(content); | ||
| return parsed ? cloneEntry(parsed) : null; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.debug("Failed to read latest sync history", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
readLatestSyncHistory silently swallows EBUSY/EPERM with no retry
on windows, an antivirus scan or the os indexer briefly grabbing an exclusive lock on sync-history-latest.json right after a write will cause fs.readFile to throw EBUSY or EPERM. the catch here treats every non-ENOENT error identically — it logs at debug and returns null. the caller in loadLastCodexCliSyncRun then falls through to readSyncHistory (which does retry), so the data isn't permanently lost, but the silent null return triggers an unnecessary second full tail-read of the ndjson file on every transient lock.
readSyncHistory already wraps its reads with retryHistoryWrite; the same wrapper should be applied here:
| export async function readLatestSyncHistory(): Promise<SyncHistoryEntry | null> { | |
| await waitForPendingHistoryWrites(); | |
| try { | |
| const content = await fs.readFile(getSyncHistoryPaths().latestPath, "utf8"); | |
| const parsed = parseEntry(content); | |
| return parsed ? cloneEntry(parsed) : null; | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.debug("Failed to read latest sync history", { | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| return null; | |
| } | |
| } | |
| export async function readLatestSyncHistory(): Promise<SyncHistoryEntry | null> { | |
| await waitForPendingHistoryWrites(); | |
| try { | |
| const content = await retryHistoryWrite(() => | |
| fs.readFile(getSyncHistoryPaths().latestPath, "utf8"), | |
| ); | |
| const parsed = parseEntry(content); | |
| return parsed ? cloneEntry(parsed) : null; | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.debug("Failed to read latest sync history", { | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| return null; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/sync-history.ts
Line: 253-268
Comment:
**`readLatestSyncHistory` silently swallows `EBUSY`/`EPERM` with no retry**
on windows, an antivirus scan or the os indexer briefly grabbing an exclusive lock on `sync-history-latest.json` right after a write will cause `fs.readFile` to throw `EBUSY` or `EPERM`. the catch here treats every non-`ENOENT` error identically — it logs at debug and returns `null`. the caller in `loadLastCodexCliSyncRun` then falls through to `readSyncHistory` (which does retry), so the data isn't permanently lost, but the silent null return triggers an unnecessary second full tail-read of the ndjson file on every transient lock.
`readSyncHistory` already wraps its reads with `retryHistoryWrite`; the same wrapper should be applied here:
```suggestion
export async function readLatestSyncHistory(): Promise<SyncHistoryEntry | null> {
await waitForPendingHistoryWrites();
try {
const content = await retryHistoryWrite(() =>
fs.readFile(getSyncHistoryPaths().latestPath, "utf8"),
);
const parsed = parseEntry(content);
return parsed ? cloneEntry(parsed) : null;
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
if (code !== "ENOENT") {
log.debug("Failed to read latest sync history", {
error: error instanceof Error ? error.message : String(error),
});
}
return null;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
#84fresh/06-docs-frontdoor-cleanupWhat Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
44bb909andd57449fAdditional Notes
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds bounded ndjson sync-result history (
sync-history.ndjson+sync-history-latest.json) shared by both the codex-cli sync and live-account-sync paths, plus an async cold-start hydration oflastCodexCliSyncRunfrom that history. the chunked tail-reader avoids full-file reads for bounded lookups, writes are serialized by a promise-chain mutex, andEBUSY/EPERMretries are applied on the write paths for windows compatibility.lib/sync-history.ts— new module: ndjson append, 8 KB chunked tail-reader, 200-entry trim-on-overflow,retryHistoryWritewrapper, and async-history-latest.jsonfast-read shortcutlib/codex-cli/sync.ts—publishCodexCliSyncRunnow firesappendSyncHistoryEntry(fire-and-forget with.catch), andloadLastCodexCliSyncRunhydrateslastCodexCliSyncRunon cold start with promise deduplicationlib/live-account-sync.ts—recordHistoryadded torunReload; path is redacted to basename before writinglib/index.ts— re-exports bothsync-historyand the updatedlive-account-syncsurfacereadLatestSyncHistory(line 253–268) is the only read path that lacksretryHistoryWrite, making it silently fall back to the more expensive tail-read on a transient windowsEBUSYlock__resetLastCodexCliSyncRunForTestshelper nullslastCodexCliSyncRunLoadPromisewithout guarding against an in-flight promise's.then()callback overwritinglastCodexCliSyncRunLoadedFromHistoryafter the reset — a potential source of cross-test state leakageloadLastCodexCliSyncRuncall deduplication or thehydrateLastCodexCliSyncRunFromHistoryguard against clobbering a fresher in-memory runConfidence Score: 3/5
readLatestSyncHistoryis missing the same retry guard applied everywhere else — a windows antivirus lock silently forces the more expensive fallback path on every cold start. the__resetLastCodexCliSyncRunForTestsstale-closure bug can produce flaky test state. the concurrency paths (promise deduplication, hydrate guard) have no dedicated vitest coverage despite being the most novel and risky additions in the PR.lib/sync-history.ts(missing retry inreadLatestSyncHistory) andlib/codex-cli/sync.ts(stale-closure in test reset helper) need the most attention.Important Files Changed
readLatestSyncHistorylacks retry onEBUSY/EPERMunlike every other read pathpublishCodexCliSyncRunpersistence;__resetLastCodexCliSyncRunForTestshas a stale-closure bug that can leaklastCodexCliSyncRunLoadedFromHistoryacross testsrecordHistorycalls on reload success/error via fire-and-forgetvoid; path is redacted to basename before writing; serialized correctly bywithHistoryLocksync-history.tsandlive-account-sync.ts; test helpers (__reset*,__get*) are exported alongside production API, consistent with the existing pattern in this libraryloadLastCodexCliSyncRundeduplication and the hydrate-guard raceloadLastCodexCliSyncRunappendSyncHistoryEntryspy correctly confirmed viawaitForPendingHistoryWritesinsidereadSyncHistorySequence Diagram
sequenceDiagram participant Caller participant sync.ts participant sync-history.ts participant Disk Note over sync.ts: publishCodexCliSyncRun() sync.ts->>sync-history.ts: appendSyncHistoryEntry() [void, fire-and-forget] sync-history.ts->>sync-history.ts: withHistoryLock() — serialize writes sync-history.ts->>Disk: fs.appendFile(sync-history.ndjson) [retried on EBUSY/EPERM] sync-history.ts->>sync-history.ts: trimHistoryFileIfNeeded() [chunked tail-read, rewrite if >200] sync-history.ts->>Disk: fs.writeFile(sync-history-latest.json) [retried] Note over Caller: loadLastCodexCliSyncRun() — cold start Caller->>sync.ts: loadLastCodexCliSyncRun() alt lastCodexCliSyncRun already set sync.ts-->>Caller: clone of in-memory run else load promise in-flight sync.ts->>sync.ts: await lastCodexCliSyncRunLoadPromise (shared) sync.ts-->>Caller: clone of loaded run else first call sync.ts->>sync-history.ts: readLatestSyncHistory() sync-history.ts->>sync-history.ts: waitForPendingHistoryWrites() sync-history.ts->>Disk: fs.readFile(sync-history-latest.json) [no retry ⚠️] alt latest entry is codex-cli-sync sync-history.ts-->>sync.ts: entry sync.ts->>sync.ts: hydrateLastCodexCliSyncRunFromHistory() [guard: only sets if null] sync.ts-->>Caller: clone else fallback sync.ts->>sync-history.ts: readSyncHistory(kind=codex-cli-sync, limit=1) sync-history.ts->>Disk: readHistoryTail() [chunked, retried on EBUSY] sync-history.ts-->>sync.ts: entries sync.ts->>sync.ts: hydrateLastCodexCliSyncRunFromHistory() sync.ts-->>Caller: clone end end Note over LiveAccountSync: runReload() success/error LiveAccountSync->>sync-history.ts: appendSyncHistoryEntry() [void] sync-history.ts->>sync-history.ts: withHistoryLock() sync-history.ts->>Disk: append + trim + write latestComments Outside Diff (1)
lib/codex-cli/sync.ts, line 322-330 (link).then()callback overwrites state after test reset__resetLastCodexCliSyncRunForTestssetslastCodexCliSyncRunLoadedFromHistory = nullandlastCodexCliSyncRunLoadPromise = nullsynchronously, but ifloadLastCodexCliSyncRunkicked off a history read that is still awaiting i/o at the moment of reset, the queued.then()callback (line 273-277 in the load chain) will fire after the reset and write the stale loaded run back intolastCodexCliSyncRunLoadedFromHistory. this leaves module-level state from the previous test in place, which can cause subsequent tests that checkloadLastCodexCliSyncRun()to return a non-null value when they expect null — a classic flaky-test vector.the standard fix is a generation counter that the in-flight closure captures and checks before writing:
Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "fix(sync): close his..."