fix(timeline): write timeline.json atomically

[puri-adityakumar]

Description

Closes #1130.

TimelineManager.saveIndex() overwrote the live timeline.json in place, so a crash mid-save could truncate the index. loadIndex() then discards an unparseable index and starts empty, silently losing every checkpoint of the session.

What changed:

• Added async atomicWriteFile(filePath, data, options?: {mode?: number}) to the shared source/utils/atomic-write.ts (with spec coverage) and switched saveIndex() to it: write to a temp file, rename into place. Readers only ever see the complete old or complete new index
• On a failed save, the unpersisted cache entry is dropped and the index reloaded from disk, so later reads report what was actually persisted
• Temp file is unlinked when the write or rename fails, so no .tmp litter

This covers process death mid-save; rename without fsync does not cover power loss (noted in a code comment).

Not changed on purpose: before-image files in capture() stay non-atomic. They land in a fresh per-entry UUID dir before the index is saved, so a torn write there only leaves an orphan that pruneStaleSessions cleans up later. The index is the single point that needed atomicity. Migrating the two remaining private async copies in session-manager.ts and semantic-memory-manager.ts to the shared util is left as follow-up material.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Changeset

• Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

• Bug fix includes passing tests in .spec.ts/tsx files
• All existing tests pass (pnpm test:all completes successfully)

Note: 4 git-tool tests (git-diff, getGitStatusSummarySync) fail on a clean main checkout too. They spawn temp repos and the repo's own commit-msg hook rejects their fixture commits in this environment. Unrelated to this PR.

• Tests cover both success and error scenarios

New tests:

• timeline-manager.spec.ts: saves atomically over a read-only live file (the discriminating test, fails on the old implementation), keeps the previous index intact on disk and in memory when a save fails, recovers from a corrupted index
• atomic-write.spec.ts (new file): rename-based write, replace-existing, mode honored, no .tmp left on failure
• The chmod-based tests skip on Windows and as root, where mode bits do not fail writes

Manual Testing

Verified failure modes via read-only file/directory injection (EACCES) and truncated-JSON corruption. Biome, tsc and knip all clean.

Checklist

• If this was for an open issue, I was assigned to it (assignment requested in [Bug] Non-atomic write of timeline.json #1130)
• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No breaking changes (or clearly documented)
• Appropriate logging added using structured logging (see CONTRIBUTING.md)

[will-lamerton]

Thanks for this - the diagnosis is right and the fix is correctly scoped. I verified locally: the spec passes on your branch (20/20), and reverting only timeline-manager.ts to the parent commit fails exactly one test (saves the index atomically over a read-only live file) with EACCES ... open timeline.json, so the discriminating test really does discriminate. biome and tsc are clean.

One change needed before merge:

Use the shared atomic-write util instead of a fourth copy. source/utils/atomic-write.ts already exists (atomicWriteFileSync / atomicWriteJson), and there are two private async duplicates in source/session/session-manager.ts:64 and source/memory/semantic-memory-manager.ts:111. The helper added at source/services/timeline-manager.ts:41 is a byte-identical third async copy. Please add the async variant to source/utils/atomic-write.ts and import it here:

export async function atomicWriteFile(
	filePath: string,
	data: string,
	options?: {mode?: number},
): Promise<void>

Migrating the two existing copies can be a follow-up; a new call site just shouldn't fork the pattern again.

Two optional notes:

• saveIndex assigns this.index = index before the write, and loadIndex returns the cached value, so a failed save leaves the process holding an index that never reached disk. Your "keeps the previous index intact" test only asserts the on-disk bytes, so it passes while the in-memory guarantee it names does not hold. Restoring the previous this.index in a catch would make the claim true end to end. Pre-existing behaviour, so your call.
• Both new permission tests rely on POSIX mode bits: as root (Docker dev containers) the t.throwsAsync case will not throw, and on Windows chmod(dir, 0o555) is a no-op. CI is non-root ubuntu so this is green today, but a process.platform === 'win32' || process.getuid?.() === 0 skip guard would save contributors some confusion.

Nit: the changeset says a crash or kill mid-save "can no longer leave a truncated index behind". True for process death, which is what #1130 is about, but rename without fsync does not cover power loss. Not worth adding fsync on every capture, just worth softening the wording.

[puri-adityakumar]

Thanks for verifying the discriminating test, and for pointing at the shared util I missed.

[puri-adityakumar]

Hi, @will-lamerton I have made the required changes as request could you verify once again please.
Thank you

[will-lamerton]

Great work @puri-adityakumar :)