Skip to content

fix(server): release oversized pull request diff cache entries - #12523

Merged
juliusmarminge merged 1 commit into
mainfrom
fix/pr-diff-cache-memory-main-20260918-180825
Sep 19, 2026
Merged

juliusmarminge merged 1 commit into
mainfrom
fix/pr-diff-cache-memory-main-20260918-180825

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 19, 2026

Copy link
Copy Markdown
Member

Large pull-request diff slices remained strongly referenced by both the normal diff cache and its stale fallback after the request finished. Those caches admitted up to 128 results without accounting for patch size.

Skip retention when a patch exceeds 512 KiB of estimated UTF-16 storage. Return the complete patch and pagination metadata unchanged, retain caching for small results, and guard invalidation against removing a replacement entry. Each persistent cache is consequently limited to 64 MiB of patch text. Repeated server requests for oversized slices fetch them again.

Before/after evidence against this PR's main base: an isolated GC check through the actual service read 16 synthetic diff results totaling 8 MiB. The base retained all 16 after GC (54 MiB heap); this revision retained zero (46 MiB heap). This demonstrates this retention defect; it does not establish the retaining root of the separately observed 3.86 GiB backend crash.

Validation: 165 focused PullRequestService tests passed, including large regular, paginated, and commit slices and small replacements; server package typecheck, scoped lint, formatting, and diff checks passed. The shared service applies to all source-control providers and web, desktop, and mobile connections without a contract change.

Prepared with Codex.


Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Improved diff caching behavior for very large responses.
    • Large diffs are still returned when requested but are no longer retained in the cache.
    • Smaller diff responses continue to be cached and reused.
    • Updated viewed-file handling to preserve cached diffs when file marks are invalidated.

Return complete large diff slices without retaining them in the normal or stale cache. Preserve small-result caching and guard replacement entries during invalidation.
@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 19, 2026
@macroscopeapp

This comment has been minimized.

@github-actions

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 +2 B (+0.0%) 15.1 KiB
Codex Thread snapshot wire 7.1 KiB 7.1 KiB +7 B (+0.1%) 7.3 KiB
Codex Live turn WebSocket wire 6.4 KiB 6.4 KiB −5 B (−0.1%) 7.8 KiB
Codex Live turn WebSocket decoded 56.2 KiB 56.2 KiB 0 B (0.0%) 66.4 KiB
Codex Live turn messages 9 9 0 (0.0%) 21
Claude Total thread wire 13.5 KiB 13.5 KiB −4 B (−0.0%) 15.1 KiB
Claude Thread snapshot wire 7.1 KiB 7.1 KiB −4 B (−0.1%) 7.3 KiB
Claude Live turn WebSocket wire 6.4 KiB 6.4 KiB 0 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: e74c668 · PR result: 04bbab1 · 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: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

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

@macroscopeapp

macroscopeapp Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 04bbab1

Macroscope's review found this PR approvable — This is a small, isolated cache-retention fix that preserves complete diff responses and existing small-diff caching while preventing oversized patches from remaining in either cache. Focused tests cover regular, paginated, and commit diff paths, including replacement behavior.

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

@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

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: Team

Run ID: 69db5a83-0161-4829-8ccc-67d7a3ea9c26

📥 Commits

Reviewing files that changed from the base of the PR and between e74c668 and 04bbab1.

📒 Files selected for processing (2)
  • apps/server/src/pullRequest/PullRequestService.test.ts
  • apps/server/src/pullRequest/PullRequestService.ts

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


📝 Walkthrough

Walkthrough

The pull request limits cached diffs to 512 KiB of UTF-16 storage. Oversized diffs are returned but not retained. Matching oversized cached values are invalidated, and tests cover repeated large responses and later small cached responses.

Changes

Diff cache handling

Layer / File(s) Summary
Cache size policy
apps/server/src/pullRequest/PullRequestService.ts
The service permits caching only diffs within the 512 KiB UTF-16 limit and skips oversized values during diff recording.
Oversized read behavior
apps/server/src/pullRequest/PullRequestService.ts, apps/server/src/pullRequest/PullRequestService.test.ts
Diff reads invalidate matching oversized cached values before stale-diff handling. Tests cover repeated large responses, later small-response reuse, and cached diffs remaining after file-mark invalidation.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: maria-rcks

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: releasing oversized pull-request diff cache entries.
Description check ✅ Passed The description explains the problem, the caching change, expected behavior, validation, and scope. It does not use the template headings or checklist, but it provides the required information and is …
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.
✨ 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.

@juliusmarminge
juliusmarminge merged commit cb3d95c into main Sep 19, 2026
24 of 25 checks passed
@juliusmarminge
juliusmarminge deleted the fix/pr-diff-cache-memory-main-20260918-180825 branch September 19, 2026 02:04
aorwall added a commit to aorwall/t3code that referenced this pull request Sep 19, 2026
Merges `pingdotgg/t3code` at `5378f87f9` into the fork, 51 commits from
base `994654198`.

`4232` files landed against `4233` in the upstream range; the gap of one
is `apps/server/src/cli/pair.ts`, which this fork deletes on purpose.
Fork delta afterwards: `777` files.

## Usable as-is

Nothing here needs Moatless backend or deployment work.

- **Client spans reach the trace proxy again** (pingdotgg#12332). Upstream
rebuilt the fork's own `ClientTracingLive` as
`apps/web/src/observability/clientTracer.ts` — same behaviour,
upstream's name — so the fork delta retired into it. `clientTracing.ts`
and `lib/runtime.ts` are byte-identical to upstream again.
- **Sidebar search matches message content** (pingdotgg#11761), with a new
`ThreadSearchMatch` component and the logic moved out of the command
palette.
- **A file-to-symlink type change no longer crashes the diff view**
(pingdotgg#11075).
- **Obsolete code removed** (pingdotgg#9917). This deleted `SidebarGroupLabel`
from `components/ui/sidebar.tsx`; the fork's `SettingsSidebarNav` was
its only caller, so the label is now inlined there rather than
re-exported from an upstream-owned file.
- **Build fixes**: executable imports parsed without matching source
strings (pingdotgg#12488), and multiple license notices retained for one package
(pingdotgg#12489) — the second sits on the `vp build` path this fork's image
workflow runs.
- **Dependencies**: Effect rc.115 and Alchemy beta.78 with their
reference sync (pingdotgg#12326, pingdotgg#12327), plus two security bumps of vulnerable
transitives (pingdotgg#12417, pingdotgg#12411).
- **`test-t3-app` rewritten around the desktop Browser panel** (pingdotgg#12414).
Taken whole with the fork's scope note re-applied.

Not applicable rather than usable, listed so the next merge does not
re-derive them: the relay deploy and client-config work (pingdotgg#12401, pingdotgg#12484,
pingdotgg#12518, pingdotgg#12519) and the CI label/report automation (pingdotgg#12517, pingdotgg#12492)
belong to infrastructure this fork does not run — every inherited
workflow here is `disabled_manually`.

## Unsupported in Moatless / needs implementation

- **Sort pull requests by what is blocked on me** (pingdotgg#12508,
`apps/web/src/components/pullRequest/pullRequestList.logic.ts`). Needs
`pullRequests.list`, `detail` and `activity`, which the backend does not
dispatch. `FEATURES.pullRequestSurface` is `false`, so the route this
lands in is not reachable here; Moatless serves `pullRequests.summary`
and nothing else in the family. The server half of the same surface is
pingdotgg#11825, below.
- **View and control agent devices from mobile** (pingdotgg#12531,
`apps/mobile/src/features/devices/`). A device panel driven by a device
stream brokered by the bundled server between a client and a registered
device. Moatless has no device registry and device pairing is decided
out in this fork, so the whole path — registration, stream transport,
control commands — is backend work.
- **The mobile client generally.** Twenty-two further mobile changes
landed in this range — pull-to-refresh, native settings and snooze
controls, model favourites, project search, platform header and menu
splits, Live Activity and Material You import isolation, notification
and permission delegate synchronization, copy-thread-id. They are in the
tree and typecheck, but whether this fork's mobile client can reach a
Moatless backend at all is still unverified; see `docs/fork/gaps.md`,
_Mobile testing against Moatless is undocumented because it is
unverified_, which this merge extended.
- **ACP SDK elicitation requests** (pingdotgg#11294,
`packages/effect-acp/src/{client,protocol,rpc}.ts`). Elicitation is an
agent-to-client request: the agent asks the user for input mid-turn and
blocks on the answer. Moatless drives its own agents rather than hosting
upstream's ACP adapters, so the round-trip has to exist on the backend
before any client surface can render it.

## Backend behavior to consider reproducing in Moatless

Nine server-side fixes, all recorded in `docs/fork/gaps.md` under
_Runtime fixes upstream made to its own server_ with the file each lives
in:

- **An oversized pull request diff should not be cached** (pingdotgg#12523) — 512
KiB cap on cached patch text, with invalidation of an entry already
held. A capacity-bounded cache with no size bound is how one enormous PR
pins memory.
- **Checkpoint git commands should be retried on a transient failure**
(pingdotgg#11665) — `…lock: file exists` and `no such file or directory`
classified as retryable and retried twice at 75 ms. The race is an agent
writing files while a checkpoint is captured, which a sandbox makes more
likely.
- **A failed settings write should roll its secret changes back**
(pingdotgg#12487) — otherwise a persistence failure leaves a provider key removed
with nothing to restore it from, and nothing says so until the provider
is next used.
- **A fetch failure should be explained without echoing the remote**
(pingdotgg#12485) — four recognised stderr shapes mapped to fixed sentences,
anything else left generic, because fetch stderr can carry credentials
from the remote URL into a persisted error.
- **A branch switch should not be readable as a path checkout** (pingdotgg#10574)
— one `--` appended to `git checkout <ref>`, with losing uncommitted
work behind it.
- **Rate limits from a tolerated read should still be recorded**
(pingdotgg#12486). Bitbucket is not a fork target; the shape is — the budget was
spent whether or not the caller wanted the answer.
- **An evicted preview host should be able to register again** (pingdotgg#12535)
— completes the RPC stream instead of shutting the queue down, so a
desktop that was merely slow can re-register. Follows pingdotgg#11381 from the
2026-09-16 merge. The client half landed here in
`packages/client-runtime`.
- **A server should export log records, not only traces and metrics**
(pingdotgg#12493) — `otlpLogsUrl` plus a shared `otlpResource`, which is what
makes the three signals joinable at the collector. The fork already
exports client spans.
- **Pull request reads should be batched rather than fanned out**
(pingdotgg#11825) — far fewer GitHub requests per preview, with a measurement
script. Moatless does its own GitHub reads behind
`pullRequests.summary`.

## Merge notes

Five conflicts, each resolved with the verdict `preflight.mjs` printed.
The one that needed thought was `apps/web/src/lib/runtime.ts`: pingdotgg#12332
reimplemented the fork's tracer layer upstream and, in the same change,
removed the `activeDelegate` binding the fork's layer read — so the fork
block auto-merged into `clientTracing.ts` referencing a symbol that no
longer existed. Resolved by converging onto upstream rather than
repairing the fork copy.

Two inventory gaps this merge closed: `apps/server/src/bin.ts` had no
path-policy entry despite holding the only references to the deleted
`cli/pair.ts` (now `server-cli-entrypoint`, `converged`), and the fork's
own `typecheck.yml` was missing from `offRepo.allowedActiveWorkflows`,
which made `tripwires.mjs` report it as an inherited workflow switched
back on.

`unsupported-methods.mjs` reported ADD 0 / DROP 0 — no change to
`packages/contracts/src/rpc.ts`.

`verify.mjs`: all 10 checks green on the final full pass, tests included
— 334 files, 5144 tests. Tripwires: Clerk 4, pairing 96, session
bootstrap 8, 5 known deletions, 4 active workflows.

Tracker entry: `docs/fork/upstream-merge-log.md`, 2026-09-19.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/0a5d08b0-0bd4-412e-a837-782ac67e5a13
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Sep 19, 2026
## What's Changed
* fix(mobile): use singular label for one settings environment by @juliusmarminge in pingdotgg/t3code#12282
* feat(mobile): add copy thread ID to thread list actions by @jakeleventhal in pingdotgg/t3code#12228
* fix(mobile): remove Android input underline backgrounds by @juliusmarminge in pingdotgg/t3code#12394
* chore(deps): upgrade Effect to rc.115 and Alchemy to beta.78 by @juliusmarminge in pingdotgg/t3code#12326
* chore(refs): sync Effect and Alchemy references to rc.115 and beta.78 by @juliusmarminge in pingdotgg/t3code#12327
* chore(relay): deploy with the Alchemy CLI and publish client config through an Action by @juliusmarminge in pingdotgg/t3code#12401
* chore(deps): bump the npm_and_yarn group across 1 directory with 3 updates by @dependabot[bot] in pingdotgg/t3code#12411
* fix(git): prevent stale branch selections from restoring files by @yashranaway in pingdotgg/t3code#10574
* chore(deps): bump parents that carry vulnerable transitive dependencies by @juliusmarminge in pingdotgg/t3code#12417
* fix(web): keep a file-to-symlink type change from crashing the diff view by @Mnigos in pingdotgg/t3code#11075
* Use T3 Device panel for mobile testing by @juliusmarminge in pingdotgg/t3code#12414
* fix(web): client spans reach the trace proxy again by @yordis in pingdotgg/t3code#12332
* fix(bitbucket): preserve rate limits from optional PR reads by @juliusmarminge in pingdotgg/t3code#12486
* fix(mobile): synchronize native permission registry access by @juliusmarminge in pingdotgg/t3code#12482
* fix(build): retain multiple license notices for one package by @juliusmarminge in pingdotgg/t3code#12489
* fix(build): parse executable imports without matching source strings by @juliusmarminge in pingdotgg/t3code#12488
* fix(mobile): synchronize native notification delegates by @juliusmarminge in pingdotgg/t3code#12483
* fix(relay): accept delegated thread IDs in activity routes by @juliusmarminge in pingdotgg/t3code#12484
* fix(git): explain fetch failures without exposing remote output by @juliusmarminge in pingdotgg/t3code#12485
* fix(web): sidebar search matches message content by @koushikxd in pingdotgg/t3code#11761
* fix(server): restore secrets when settings persistence fails by @juliusmarminge in pingdotgg/t3code#12487
* fix(ci): accept V2 transfer reports without cross-scenario comparisons by @juliusmarminge in pingdotgg/t3code#12492
* fix(web): speed up PR previews with fewer GitHub requests by @dominic-r in pingdotgg/t3code#11825
* fix(server): retry transient git failures during checkpoint capture by @saphid in pingdotgg/t3code#11665
* fix(mobile): keep archived threads visible during iOS search by @juliusmarminge in pingdotgg/t3code#12420
* perf(mobile): isolate Material You conversion on Android by @juliusmarminge in pingdotgg/t3code#12379
* perf(mobile): isolate iOS Live Activity imports by @juliusmarminge in pingdotgg/t3code#12380
* refactor(mobile): split home headers by platform by @juliusmarminge in pingdotgg/t3code#12381
* refactor(mobile): split native menus by platform by @juliusmarminge in pingdotgg/t3code#12382
* refactor(mobile): isolate thread row appearance by platform by @juliusmarminge in pingdotgg/t3code#12383
* refactor(mobile): split settings selection rows by platform by @juliusmarminge in pingdotgg/t3code#12384
* refactor(mobile): centralize platform header rendering by @juliusmarminge in pingdotgg/t3code#12388
* refactor(mobile): configure thread headers through the shared core by @juliusmarminge in pingdotgg/t3code#12389
* refactor(mobile): share file header actions and search configuration by @juliusmarminge in pingdotgg/t3code#12390
* refactor(mobile): share terminal header and menu configuration by @juliusmarminge in pingdotgg/t3code#12391
* refactor(mobile): share archived thread header configuration by @juliusmarminge in pingdotgg/t3code#12399
* refactor(mobile): compose review menus through the shared header by @juliusmarminge in pingdotgg/t3code#12400
* feat(mobile): search projects when starting a task by @juliusmarminge in pingdotgg/t3code#12496
* fix(mobile): preserve multiple model favorites by @juliusmarminge in pingdotgg/t3code#12505
* feat(server): export log records over OTLP by @yordis in pingdotgg/t3code#12493
* fix(mobile): use native settings and snooze controls by @juliusmarminge in pingdotgg/t3code#12512
* feat(web): sort pull requests by what is blocked on me by @flamboh in pingdotgg/t3code#12508
* fix(mobile): prefer pull-to-refresh on list screens by @juliusmarminge in pingdotgg/t3code#12515
* fix(acp): accept SDK elicitation requests by @shivamhwp in pingdotgg/t3code#11294
* fix(release): read relay configuration without loading deployment providers by @juliusmarminge in pingdotgg/t3code#12518
* fix(ci): reconcile native change labels against pinned commits by @juliusmarminge in pingdotgg/t3code#12517
* fix(release): strip Alchemy progress before parsing relay state by @juliusmarminge in pingdotgg/t3code#12519
* refactor: remove obsolete code by @t3dotgg in pingdotgg/t3code#9917
* fix(server): release oversized pull request diff cache entries by @juliusmarminge in pingdotgg/t3code#12523
* feat(mobile): view and control agent devices by @juliusmarminge in pingdotgg/t3code#12531
* fix(preview): recover host registration after request timeouts by @juliusmarminge in pingdotgg/t3code#12535
* fix(mobile): align built-in theme colors with desktop by @juliusmarminge in pingdotgg/t3code#12534
* feat(desktop): export main process telemetry over OTLP by @yordis in pingdotgg/t3code#12520
* fix(codex): surface app permission requests as approvable by @Exotic209093 in pingdotgg/t3code#7861
* chore(desktop): leave main process metrics export off until a metric exists by @juliusmarminge in pingdotgg/t3code#12540
* fix(release): drop placeholder allowBuilds entry that broke desktop builds by @juliusmarminge in pingdotgg/t3code#12544

## New Contributors
* @dependabot[bot] made their first contribution in pingdotgg/t3code#12411
* @koushikxd made their first contribution in pingdotgg/t3code#11761

**Full Changelog**: pingdotgg/t3code@v0.0.43-nightly.20260918.1895...v0.0.43-nightly.20260919.1948

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.43-nightly.20260919.1948
AIdoesmyjob pushed a commit to AIdoesmyjob/t3code that referenced this pull request Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 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.

1 participant