Skip to content

perf(server): avoid rereading unchanged files in review previews - #13395

Merged
juliusmarminge merged 3 commits into
mainfrom
shivam/review-index-cache-seconds
Sep 25, 2026
Merged

juliusmarminge merged 3 commits into
mainfrom
shivam/review-index-cache-seconds

Conversation

@shivamhwp

@shivamhwp shivamhwp commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Review previews can reread thousands of unchanged files from the preceding second when Git's index timestamp falls on a whole second. In a constructed 30,000-file repository, this reduces median preview time from 1,305 ms to 167 ms, about 7.8× faster. It preserves the missing-edit fix from #12613. This is a timestamp-boundary improvement, not a general 7.8× speedup for every repository.

Remove the extra one-millisecond subtraction when setting the temporary index timestamp. The existing Node FileSystem service already reads bigint stats and truncates to whole milliseconds before constructing its Date; flooring that Date preserves the source second. There is no new native import, diagnostic suppression, or error-handling path. Regular- and split-index tests verify that unchanged preceding-second files do not run their clean filter and that the real index stays unchanged.

Validation

Updated with main on September 25 at 9c41485408623e339b799e417be8e7e6f52999e8. Reran all 104 focused tests, server typecheck, formatting, and 32 live API requests on this integration commit; all passed. Targeted lint still has the same preexisting warning.

Detailed matrix and benchmark head: 4a0fd4fcb165dc61eaa088e4e23d30a50b64e57f. Baseline: b2b43bef73447c483ceae486890cb79f01c369cb, main immediately after #12613. No uncommitted code was included.

  • All 104 focused core tests pass with installed Git 2.39.5 and with a locally built Git 2.39.5 using USE_NSEC. The latter uses system Git helpers for submodule commands.
  • All 62 live API scenarios pass in each of three configurations: Node 24.21 / installed Git, Node 24.21 / USE_NSEC Git, and Node 22.16 / installed Git. These cover index versions 2/4, split indexes, ctime trust on/off, timestamp boundaries, partial staging, sparse indexes, untracked cache, assume-unchanged/skip-worktree, binary content, symlinks, executable mode, unusual filenames, linked worktrees, concurrent previews, and preservation of the real index.
  • Another 32 API requests pass for repeated previews, individual file previews, file contents, regular/split indexes, linked worktrees, and whole/fractional timestamps.
  • Additional exact-nanosecond/year-2038 and read-only/unreadable/corrupt/locked/symlinked/missing-index and nested-directory checks ran against both Git builds. Each passed 17 of 19 scenarios; the two failures are the same inherited first-Unix-epoch-second limitation described below. Permission/corruption checks expect an error rather than an empty successful preview.
  • Server typecheck and formatting pass. Targeted lint reports one preexisting warning. The two new cache tests fail on the baseline and pass here; original same-size-edit regressions remain passing.

Performance

Linux, 30,000 files / 469 MiB, warm filesystem cache, forced timestamps. Times are medians of complete WebSocket requests; correctness and original-index preservation were asserted throughout.

Scenario Baseline This PR Requests per version
One ambiguous edit; 29,999 unchanged preceding-second files 1,305 ms 167 ms 12
All 30,000 files in the ambiguous second 1,298.5 ms 1,308 ms 12
Ordinary timestamps, short benchmark 151.5 ms 163 ms 12
Ordinary timestamps, longer alternating run 137 ms 134 ms 80
No untracked files, changed function bypassed 97 ms 94 ms 80

The short benchmark used baseline/PR/PR/baseline order with one warmup per batch. The longer run used baseline/PR/PR/baseline/PR/baseline/baseline/PR with five warmups and 20 measurements per batch. Small ordinary-case differences change direction between runs, including variation in the bypass control. The earlier 163 → 182 ms slowdown is not a stable result; these measurements do not prove zero overhead. Files that actually need checking still cost about 1.3 seconds in this fixture.

Why the existing timestamp is precise enough

The pinned @effect/platform-node-shared adapter calls fs.stat(path, { bigint: true }) and returns its mtime Date. Node's BigIntStats computes mtimeMs = mtimeNs / 1000000n before constructing that Date. Ordinary non-bigint stats can round across a second; this bigint-backed path does not. We verified the installed adapter, Node 22/24 runtime behavior, and exact final-nanosecond fixtures. See Node 22.16 BigIntStats.

The previous revision added a separate native stat call unnecessarily. This revision keeps the existing service and its error handling. The existing fractional-timestamp regression tests protect this dependency on the Node adapter's conversion behavior.

Raw test and benchmark results

Limits

Timestamps within the first second after the Unix epoch still reproduce a missed edit with both baseline and PR, because a zero-second index disables Git's racy check. This PR fixes the baseline failure at exactly epoch second 1, but does not claim to repair all epoch-zero behavior. Missing-index behavior is also inherited.

Windows, macOS, controlled cold-disk tests, concurrent external index rewrites, and real-world frequency remain unmeasured. Both T3 Browser status and open report no automation host, so the requested before/after UI recording could not be captured. No frontend, provider, or wire-contract code changes are included.

Implemented and tested with GPT-6 in the Codex harness. Claude Opus 5.5 reviewed the earlier native-stat version; the simplification was verified separately in this pass.

Summary by CodeRabbit

  • Bug Fixes
    • Improved review previews for repositories using standard or split Git indexes. Previews now correctly include untracked files without unnecessarily running clean filters or changing the existing index.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 24, 2026
@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.5 KiB 13.5 KiB −36 B (−0.3%) 15.1 KiB ✅
Codex Thread snapshot wire 7.1 KiB 7.1 KiB −2 B (−0.0%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.5 KiB 6.4 KiB −34 B (−0.5%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 56.3 KiB 56.2 KiB −44 B (−0.1%) 66.4 KiB ✅
Codex Live turn messages 10 9 −1 (−10.0%) 21 ✅
Claude Total thread wire 13.5 KiB 13.5 KiB +5 B (+0.0%) 15.1 KiB ✅
Claude Thread snapshot wire 7.1 KiB 7.1 KiB +3 B (+0.0%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.4 KiB 6.4 KiB +2 B (+0.0%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 57.0 KiB 57.0 KiB 0 B (0.0%) 66.4 KiB ✅
Claude Live turn messages 9 9 0 (0.0%) 21 ✅

Baseline: e5a46d6 · PR result: 9c41485 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 114.0 KiB
  • Claude decoded thread snapshot: 114.7 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: dc77bc6d-2f7b-4508-a8b1-76f4973645a4

📥 Commits

Reviewing files that changed from the base of the PR and between 4a0fd4f and 9c41485.

📒 Files selected for processing (1)
  • apps/server/src/vcs/GitVcsDriverCore.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

prepareReviewIndex timestamps the copied temporary index using the index modification time floored to whole Unix seconds and clamped to zero. Tests cover review previews with split indexes enabled and disabled.

Changes

Review index timestamp handling

Layer / File(s) Summary
Read the index timestamp and verify preview behavior
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts
prepareReviewIndex floors the index modification time to whole Unix seconds and clamps it to zero, using zero when the time is absent. Tests verify that previews include only the untracked file and leave the clean filter and original index unchanged for both split-index settings.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 9c414

Preview tests cover both avoiding unnecessary reads of unchanged files and preserving edited tracked files at the timestamp boundary. No merge-blocking preview risk is evident.

Architecture Summary

Architecture risk: 🔵 Low · up to 9c414

The change affects 1 system.

Changed systems: apps/server

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.test.ts: Adds the same preceding-second review-preview test for both split-index settings. It asserts that only untracked.txt appears in the working-tree preview, while the clean filter remains unrun and the index bytes and modification time remain unchanged.
  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.ts: prepareReviewIndex updates the timestamp comments and removes the one-millisecond subtraction before flooring the index modification time; the resulting time remains clamped to zero, with zero used when the time is absent.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main performance change: avoiding rereads of unchanged files in server review previews.
Description check ✅ Passed The description clearly explains what changed, why it changed, validation results, performance measurements, scope, and known limitations. It does not use the template headings or include the checklis…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

@macroscopeapp

macroscopeapp Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4a0fd4f

Macroscope's review found this PR approvable — This is a small, test-covered adjustment to temporary Git index timestamps that avoids rereading unchanged files during review previews while preserving racy-edit detection and the real index. It introduces no product-default, schema, sensitive-code, or static-analysis configuration changes.

No code changes detected at 9c41485. Prior analysis still applies.

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

@github-actions github-actions Bot added size:XS 0-9 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Sep 25, 2026
@juliusmarminge
juliusmarminge merged commit 4293433 into main Sep 25, 2026
24 checks passed
@juliusmarminge
juliusmarminge deleted the shivam/review-index-cache-seconds branch September 25, 2026 20:17
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Sep 25, 2026
## What's Changed
* feat(usage): read cursor, opencode, and antigravity history by @maria-rcks in pingdotgg/t3code#10409
* fix(sqlite): retry failed statement preparations by @yashranaway in pingdotgg/t3code#10584
* fix(mobile): scale Android controls with appearance text size by @none23 in pingdotgg/t3code#13356
* fix(web): return focus to the composer after saving a citation note by @mackinleysmith in pingdotgg/t3code#13450
* feat(observability): honor the standard OTLP endpoint, headers, and protocol variables by @yordis in pingdotgg/t3code#13492
* fix(terminal): settling a thread closes its idle shells by @t3dotgg in pingdotgg/t3code#13673
* fix(server): load Cursor keyring with createRequire by @Yash-Singh1 in pingdotgg/t3code#13678
* perf(server): avoid rereading unchanged files in review previews by @shivamhwp in pingdotgg/t3code#13395


**Full Changelog**: pingdotgg/t3code@v0.0.43-nightly.20260925.2251...v0.0.43-nightly.20260925.2269

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.43-nightly.20260925.2269
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants