Sign up
View stargazersSign up

perf(server): batch checkpoint object flushes on macOS and Windows#21291

merged6
main
GitHub fork: fix/checkpoint-fsync-batch
opened23d agobyJjosephv123
4 files changed
View on GitHubTwo-way sync enabledPR last imported 21d ago

Checkpoint capture flushed every new Git object separately, making large turns slow on macOS. This lets Git 2.37+ batch object flushes on macOS and Windows while keeping object/ref flushing enabled. Linux and other platforms retain explicit per-object fsync because Git documents batch equivalence for HFS+/APFS and NTFS/ReFS, not arbitrary Linux filesystems.

On macOS and Windows, commands set core.fsyncMethod=fsync before core.fsyncMethod=batch. Git 2.36 ignores the unknown second value while retaining full flushing. This avoids a version probe or capability cache. The existing macOS fallback for unsupported full flushes recognizes both settings and replaces both on its single retry.

Rebuilt from the proof of concept by @josephv123 in #21291, preserving the contributor commits and merging main. This changes checkpoint durability/performance defaults and still needs maintainer approval before merging.

Verification on Linux with Git 2.43.0:

  • GitVcsDriver.test.ts, VcsProcess.test.ts, and CheckpointStore.test.ts: 113 passed (62 + 39 + 12). Driver/process coverage went from 97 to 101 tests. The existing capture test now covers Linux, macOS, and Windows selection; two batch fallback cases assert that both settings are rewritten. Duplicate fallback cases were removed.
  • Regression proof: restoring the reviewed implementation without the platform gate fails the Linux capture case; the final implementation passes all three platform cases. Restoring the original implementation also exposes the missing batch fallback.
  • Targeted lint and formatting, and server-scoped typecheck passed.
  • Real service integration captures, diffs, and restores ran against temporary repositories. A final Linux service capture traced with strace selected fsync on every checkpoint write and issued no sync_file_range calls.
  • The ordered-option compatibility relies on Git 2.36 configuration parsing and Git 2.37 configuration parsing. A current-Git smoke with repository config set to writeout-only confirmed explicit fsync followed by an unknown method retains full flushing. This is not execution of a Git 2.36 binary.
  • See Git batch-mode documentation for the filesystem-specific safety expectation.

Native macOS, Windows, old Git binaries, unusual filesystems/network mounts, and actual crash/power-loss durability were not verified. Platform selection and the macOS fallback are covered through the existing host-platform/process test boundaries. Electron, iOS, and Android clients were not exercised. The earlier raw Linux batch benchmark is not evidence of a speedup delivered by this revision; Linux retains its prior method. No new latency claim is made.

Independent full-diff review: two rounds by Claude Opus 5.5 and Codex GPT-6 Sol. The first round raised Linux durability and duplicate-test concerns; both were addressed. Both second-round reviews found no new code blockers at a3cc039f5332d54bef05e373e9b793f79497cae4. CI and human approval remain separate gates.

Implemented with Codex gpt-6.1-sol; focused tests by Codex gpt-6-astra; independent reviews by Claude claude-opus-5-5 and Codex gpt-6-sol, through T3 Code.

Checks

21 successful · 10 skipped
All checks passed
970a024
MmacroscopeappBot23d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR changes the existing checkpoint capture path for every command, replacing per-object fsync with Git batch flushing while retaining ref flushing. Because it changes the default durability/performance behavior rather than introducing an opt-in path, human review is warranted.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot23d ago
Synced from GitHub

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d7088bd7-2f71-4897-a110-c385e89dc17a
📥 Commits

Reviewing files that changed from the base of the PR and between a3cc039f5332d54bef05e373e9b793f79497cae4 and 970a02403cbaeeece45c2e05cb221f9af210fd35.

📒 Files selected for processing: 4
  • apps/server/src/vcs/GitVcsDriver.test.ts
  • apps/server/src/vcs/GitVcsDriver.ts
  • apps/server/src/vcs/VcsProcess.test.ts
  • apps/server/src/vcs/VcsProcess.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 113c6f95-1e6c-4fa0-b7c6-7b324f2ca466
📥 Commits

Reviewing files that changed from the base of the PR and between 8f53d6d0745da85737e5dfd9d5d3fea757fa7194 and a3cc039f5332d54bef05e373e9b793f79497cae4.

📒 Files selected for processing: 4
  • apps/server/src/vcs/GitVcsDriver.test.ts
  • apps/server/src/vcs/GitVcsDriver.ts
  • apps/server/src/vcs/VcsProcess.test.ts
  • apps/server/src/vcs/VcsProcess.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Checkpoint-writing commands select fsync settings by host platform. macOS retry handling now recognizes batch-mode fsync failures and can retry with writeout-only. Tests cover platform-specific command settings and fallback behavior.

Changes: Checkpoint durability

Layer / File(s)Summary
Configure platform-specific checkpoint fsync
apps/server/src/vcs/GitVcsDriver.ts, apps/server/src/vcs/GitVcsDriver.test.ts		Checkpoint-writing commands retain core.fsync=objects,reference and default to fsync. Darwin and Windows also use batch. Tests check settings on four Git write commands and verify the final update-ref arguments.
Retry unsupported checkpoint fsync
apps/server/src/vcs/VcsProcess.ts, apps/server/src/vcs/VcsProcess.test.ts		On macOS, retry handling recognizes both fsync and batch when the existing conditions match. It replaces the configured method with writeout-only. Tests cover unsupported batch flushes and reference updates.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: mnigos, mdshzb04

Merge Risk: ⚪ Minimal · up to a3cc0

This change speeds up checkpoint captures on macOS and Windows by using batched fsync, and it keeps a fallback for macOS volumes where batch flushing is unsupported. No actionable merge-blocking risk was identified. The author did not test SMB, NFS, AFP, Windows, or older Git on macOS, so that behavior remains unconfirmed.

Security architecture risk: 🔵 Low · up to a3cc0

The change preserves checkpoint publication order, reference flushing, and bounded recovery. No newly reachable privileged operation was identified. Remaining uncertainty concerns crash durability across different Git versions and storage backends.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected persistence sink is the selected repository’s Git object store and checkpoint references. Temporary-index placement uses its common Git directory, so failure containment matters beyond an individual working-tree file. The inspected changes do not add credential access or network operations.

Security Findings and Attack Paths

  • inferred — Fallback classification trusts matching subprocess stderr rather than proving diagnostic provenance. That condition already existed at the merge base for the same checkpoint commands and guards, where it could select writeout-only from fsync. Extending recognition to batch does not establish a new attack path. Attacker control of repository filter configuration in the deployed environment was not established.

Trust Boundaries and Controls

  • observed — The fallback requires Darwin, the internal checkpoint operation, Git, a nonzero exit, disallowed nonzero results, a recognized flush setting, and a matching diagnostic. It preserves command, working directory, environment, and other arguments while replacing the flush-method arguments.

Resilience and Maintainability Implications

  • observed — The transition check found one fallback attempt per invocation, unchanged semaphore protection, and at most two additional retries for classified transient checkpoint failures. Each retry starts from the original input. Final failure still propagates, reference publication remains last, and temporary-index cleanup remains attached to capture completion or interruption.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description thoroughly covers the problem, implementation, verification, limitations, and agent usage. However, it does not provide the required explicit maintainer approval or approval comment, a…Add the required template headings and link the triaged issue or discussion containing explicit maintainer approval, including the approval comment. If no prior approval is required, explain why this focused configuration qualifies for the …
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title clearly identifies the main change: enabling batch checkpoint object flushing on macOS and Windows.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description thoroughly covers the problem, implementation, verification, limitations, and agent usage. However, it does not provide the required explicit maintainer approval or approval comment, and it states that approval is still needed.

Resolution

Add the required template headings and link the triaged issue or discussion containing explicit maintainer approval, including the approval comment. If no prior approval is required, explain why this focused configuration qualifies for the exemption.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Jjosephv12323d ago
Synced from GitHub

Note

Claude Opus 5.5 in Claude Code, writing on behalf of @josephv123.

The failing checks are inherited from main. The CI run on this PR's base, e2cc34f1e4 (run 39162451347), fails the same jobs for the same reasons:

  • Lint and Test Server 7: Identifier 'Path' has already been declared in GrokAdapterV2.test.ts.
  • Test Server 4: ThreadFork.execution.test.ts › a persisted fork leaves PR watching with its parent until explicitly opted in.
  • Build and Typecheck: Cannot find module 'react-test-renderer' in two web test files.

The suites this PR touches passed in this run: src/vcs/GitVcsDriver.test.ts (60 tests, Test Server 4) and src/checkpointing/CheckpointStore.test.ts (12 tests, Test Server 7).

Added 1 commit

  • perf(server): checkpoints flush new files to disk once per command instead of once per file8f53d6d
JJulius Marminge23d ago

Taking over this one as maintainer for batch server-persistence-01. I will preserve author credit, verify batch flushing compatibility with older Git, and run focused tests plus independent reviews. This takeover will remain unmerged.

Added 5 commits

  • Merge remote-tracking branch 'origin/main' into fix/checkpoint-fsync-batch4739322
  • test(server): cover checkpoint batch flush compatibilitye2ec86c
  • fix(server): preserve checkpoint durability with batch flushing68fa527
  • fix(server): limit checkpoint batch flushing to macOS and Windowsa3cc039
  • Merge remote-tracking branch 'origin/main' into pr-21291970a024