From 35e06073eed8ec0a71b2b55e5af1426b85d55bf7 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Mon, 26 Oct 2026 16:22:54 +0100 Subject: [PATCH] fix(server): keep diff cancellation with its requester Independent commit viewers must not cancel each other. Use request-source cancellation for commit highlighting and cache invalidation for checkout highlighting. Oversized source versions use the existing budgeted hunk reconstruction so large lockfiles retain useful syntax tokens. --- .../checkout-diff-content.e2e.test.ts | 5 +- packages/server/src/server/session.ts | 2 +- .../session/checkout/checkout-session.ts | 7 +- .../src/server/utils/diff-highlighter.test.ts | 82 +++++++++++++------ .../src/server/utils/diff-highlighter.ts | 53 +++--------- .../checkout-git.commit-file-diff.test.ts | 42 ++++++++++ packages/server/src/utils/checkout-git.ts | 59 +++---------- 7 files changed, 134 insertions(+), 116 deletions(-) diff --git a/packages/server/src/server/daemon-e2e/checkout-diff-content.e2e.test.ts b/packages/server/src/server/daemon-e2e/checkout-diff-content.e2e.test.ts index fad67ae7b9..fe83c21305 100644 --- a/packages/server/src/server/daemon-e2e/checkout-diff-content.e2e.test.ts +++ b/packages/server/src/server/daemon-e2e/checkout-diff-content.e2e.test.ts @@ -251,7 +251,10 @@ test("large source diffs stay renderable for reads, subscriptions, and commit fi expect(lines.filter((line) => line.type === "add").map((line) => line.content)).toEqual([ `const value = Math.max(${value}, 7);`, ]); - expect(lines.every((line) => line.tokens === undefined)).toBe(true); + expect(lines.find((line) => line.type === "add")?.tokens).toContainEqual({ + text: value, + style: "number", + }); }; check(initial.files[0], "43"); const subscription = ctx.client.observeCheckoutDiff(cwd, { mode: "uncommitted" }); diff --git a/packages/server/src/server/session.ts b/packages/server/src/server/session.ts index f573b1dd95..db4939aa31 100644 --- a/packages/server/src/server/session.ts +++ b/packages/server/src/server/session.ts @@ -2964,7 +2964,7 @@ export class Session { case "checkout.commits.list.request": return this.checkoutSession.handleCommitsListRequest(msg); case "checkout.commits.file_diff.request": - return this.checkoutSession.handleCommitFileDiffRequest(msg); + return this.checkoutSession.handleCommitFileDiffRequest(msg, this.delivery.requestSignal); case "validate_branch_request": return this.checkoutSession.handleValidateBranchRequest(msg); case "branch_suggestions_request": diff --git a/packages/server/src/server/session/checkout/checkout-session.ts b/packages/server/src/server/session/checkout/checkout-session.ts index 419d418050..ceb69e9623 100644 --- a/packages/server/src/server/session/checkout/checkout-session.ts +++ b/packages/server/src/server/session/checkout/checkout-session.ts @@ -290,7 +290,10 @@ export class CheckoutSession { } } - async handleCommitFileDiffRequest(msg: CheckoutCommitFileDiffRequest): Promise { + async handleCommitFileDiffRequest( + msg: CheckoutCommitFileDiffRequest, + signal: AbortSignal, + ): Promise { const { cwd, sha, path, requestId } = msg; try { @@ -298,7 +301,7 @@ export class CheckoutSession { if (path.length === 0 || isAbsolute(path) || path.split(/[\\/]/).includes("..")) { throw new Error(`Invalid path: ${path}`); } - const file = await getCommitFileDiff({ cwd: expandTilde(cwd), sha, path }); + const file = await getCommitFileDiff({ cwd: expandTilde(cwd), sha, path, signal }); this.host.emit({ type: "checkout.commits.file_diff.response", payload: { cwd, sha, path, file, error: null, requestId }, diff --git a/packages/server/src/server/utils/diff-highlighter.test.ts b/packages/server/src/server/utils/diff-highlighter.test.ts index 7599ca68e7..3ecb0372ad 100644 --- a/packages/server/src/server/utils/diff-highlighter.test.ts +++ b/packages/server/src/server/utils/diff-highlighter.test.ts @@ -4,9 +4,9 @@ import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { setImmediate } from "node:timers/promises"; import { performance } from "node:perf_hooks"; -import { highlightCode } from "@getpaseo/highlight"; +import { SessionDelivery } from "../session/owned-subscriptions/index.js"; +import { highlightCode, highlightCodeWithBudget } from "@getpaseo/highlight"; import { - DiffHighlightRequests, parseDiff, reconstructNewFile, reconstructOldFile, @@ -826,7 +826,7 @@ index 1111111..2222222 100644 describe("diff highlighting budget", () => { it.each(["supplied", "local"] as const)( - "keeps a one-line diff on a 5 MiB %s source responsive and renders it without tokens", + "keeps a one-line diff on a 5 MiB %s source responsive using hunk tokens", async (source) => { const cwd = mkdtempSync(join(tmpdir(), "diff-highlight-budget-")); const oldFileContent = "const value = Math.max(42, 7);\n".repeat( @@ -857,7 +857,7 @@ describe("diff highlighting budget", () => { await new Promise((resolve) => setTimeout(resolve, 5)); console.info({ sourceBytes: Buffer.byteLength(oldFileContent), longestGap }); expect(longestGap).toBeLessThan(50); - expect(highlighted).toEqual(file); + expect(highlighted).toEqual(highlightDiffFromHunks(file)); } finally { clearInterval(timer); rmSync(cwd, { recursive: true, force: true }); @@ -888,27 +888,29 @@ describe("diff highlighting budget", () => { }); }); -it("a newer request cancels the whole previous diff even before its sources arrive", async () => { - const requests = new DiffHighlightRequests(); - const file = parseDiff(SIMPLE_DIFF)[0]; - const source = "const value = Math.max(42, 7);\n".repeat(3_000); - const old = requests.run("checkout-target", async (signal) => { - await setImmediate(); - return highlightDiffWithFileContent(file, ".", { - oldFileContent: source, - newFileContent: source, - signal, - }); +it("disconnecting the requesting source stops a parser between highlight steps", async () => { + const delivery = new SessionDelivery(() => {}); + const source = {}; + delivery.attach(source, true); + const yielded = Promise.withResolvers(); + let result: Awaited> = []; + const pending = delivery.request(source, { type: "ping", requestId: "highlight" }, async () => { + result = await highlightCodeWithBudget( + "const value = Math.max(42, 7);\n".repeat(3_000), + "file.ts", + { + signal: delivery.requestSignal, + yieldToEventLoop: () => { + yielded.resolve(); + return setImmediate(); + }, + }, + ); }); - const current = requests.run("checkout-target", (signal) => - highlightDiffWithFileContent(file, ".", { - oldFileContent: "const bar = 2;", - newFileContent: "const bar = 3;", - signal, - }), - ); - expect(await old).toEqual(file); - expect((await current).hunks[0].lines.some((line) => line.tokens)).toBe(true); + await yielded.promise; + await delivery.detach(source); + await pending; + expect(result).toBeNull(); }); it("keeps real session.ts diffs highlighted and byte-identical under the source budget", async () => { @@ -938,3 +940,35 @@ it("keeps real session.ts diffs highlighted and byte-identical under the source rmSync(cwd, { recursive: true, force: true }); } }); + +it("highlights a one-line hunk from an over-budget JSON source without blocking the loop", async () => { + const cwd = mkdtempSync(join(tmpdir(), "diff-highlight-large-json-")); + const oldFileContent = `{\n${' "value": 42,\n'.repeat(100_000)} "last": true\n}`; + const newFileContent = oldFileContent.replace("42", "43"); + writeFileSync(join(cwd, "package-lock.json"), newFileContent); + const [file] = parseDiff(`diff --git a/package-lock.json b/package-lock.json +--- a/package-lock.json ++++ b/package-lock.json +@@ -2 +2 @@ +- "value": 42, ++ "value": 43,`); + let last = performance.now(), + longestGap = 0; + const timer = setInterval(() => { + const now = performance.now(); + longestGap = Math.max(longestGap, now - last); + last = now; + }, 1); + try { + const result = await highlightDiffWithFileContent(file, cwd, { oldFileContent }); + await new Promise((resolve) => setTimeout(resolve, 5)); + expect(longestGap).toBeLessThan(50); + expect(result.hunks[0].lines.find((line) => line.type === "add")?.tokens).toContainEqual({ + text: "43", + style: "number", + }); + } finally { + clearInterval(timer); + rmSync(cwd, { recursive: true, force: true }); + } +}); diff --git a/packages/server/src/server/utils/diff-highlighter.ts b/packages/server/src/server/utils/diff-highlighter.ts index d290155899..e3a0460322 100644 --- a/packages/server/src/server/utils/diff-highlighter.ts +++ b/packages/server/src/server/utils/diff-highlighter.ts @@ -49,29 +49,6 @@ interface ParseAndHighlightDiffOptions { getNewFileContent?: (file: ParsedDiffFile) => Promise; } -/** Owns supersession for a whole diff, including sources that arrive after cancellation. */ -export class DiffHighlightRequests { - private readonly active = new Map(); - - async run( - target: string, - operation: (signal: AbortSignal) => Promise, - parentSignal?: AbortSignal, - ): Promise { - this.active.get(target)?.abort(); - const controller = new AbortController(); - this.active.set(target, controller); - const signal = parentSignal - ? AbortSignal.any([parentSignal, controller.signal]) - : controller.signal; - try { - return await operation(signal); - } finally { - if (this.active.get(target) === controller) this.active.delete(target); - } - } -} - /** * Parse a unified diff into structured data */ @@ -315,23 +292,23 @@ async function buildFullFileTokenLookup( return new Map(highlighted.map((tokens, index) => [index + 1, tokens])); } -// null means over budget; undefined means unavailable and permits reconstruction. -async function readHighlightSource(path: string): Promise { +async function readHighlightSource(path: string): Promise { let handle; try { handle = await open(path, constants.O_RDONLY | constants.O_NONBLOCK); const stat = await handle.stat(); if (!stat.isFile() || stat.size > HIGHLIGHT_MAX_SOURCE_BYTES) return null; - const buffer = Buffer.alloc(HIGHLIGHT_MAX_SOURCE_BYTES + 1); + const buffer = Buffer.alloc(Math.min(stat.size, HIGHLIGHT_MAX_SOURCE_BYTES) + 1); let offset = 0; while (offset < buffer.length) { const { bytesRead } = await handle.read(buffer, offset, buffer.length - offset, null); if (bytesRead === 0) break; offset += bytesRead; } - return offset > HIGHLIGHT_MAX_SOURCE_BYTES ? null : buffer.toString("utf8", 0, offset); + // A full buffer includes the extra byte: the source grew after stat. + return offset === buffer.length ? null : buffer.toString("utf8", 0, offset); } catch { - return undefined; + return null; } finally { await handle?.close(); } @@ -418,16 +395,16 @@ export async function highlightDiffWithFileContent( typeof options.newFileContent === "string" ? options.newFileContent : await readHighlightSource(resolve(cwd, file.path)); - if (newContent === null || options.signal?.aborted) return file; + if (options.signal?.aborted) return file; const oldTokens = await highlightAvailableSource( options.oldFileContent, file.path, options.signal, ); - if (oldTokens === null || options.signal?.aborted) return file; + if (options.signal?.aborted) return file; const newTokens = await highlightAvailableSource(newContent, file.path, options.signal); - if (newTokens === null || options.signal?.aborted) return file; + if (options.signal?.aborted) return file; if (oldTokens && newTokens) return applyTokensToHunks(file, newTokens, oldTokens); const reconstructed = await buildBudgetedReconstructedTokens(file, options.signal); if (!reconstructed || options.signal?.aborted) return file; @@ -442,23 +419,17 @@ function isDiffHighlightEligible( file: ParsedDiffFile, options: HighlightDiffWithFileContentOptions, ): boolean { - return ( - !options.signal?.aborted && - isLanguageSupported(file.path) && - !hasOversizedDiffLine(file) && - ![options.oldFileContent, options.newFileContent].some( - (content) => typeof content === "string" && exceedsSourceBudget(content), - ) - ); + return !options.signal?.aborted && isLanguageSupported(file.path) && !hasOversizedDiffLine(file); } async function highlightAvailableSource( content: string | null | undefined, path: string, signal?: AbortSignal, -): Promise | null | undefined> { +): Promise | null> { // Preserve the existing hunk-context fallback for oversized individual lines. - if (typeof content !== "string" || hasOversizedLine(content)) return undefined; + if (typeof content !== "string" || exceedsSourceBudget(content) || hasOversizedLine(content)) + return null; return buildFullFileTokenLookup(content, path, signal); } diff --git a/packages/server/src/utils/checkout-git.commit-file-diff.test.ts b/packages/server/src/utils/checkout-git.commit-file-diff.test.ts index 576e9b0c80..efda5be92c 100644 --- a/packages/server/src/utils/checkout-git.commit-file-diff.test.ts +++ b/packages/server/src/utils/checkout-git.commit-file-diff.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "os"; import { join } from "path"; import { afterEach, describe, expect, it } from "vitest"; import { getCommitFileDiff, listCheckoutCommits } from "./checkout-git.js"; +import { SessionDelivery } from "../server/session/owned-subscriptions/index.js"; import { startGitCommandMetrics, stopGitCommandMetrics } from "./run-git-command.js"; const tempDirs: string[] = []; @@ -302,3 +303,44 @@ describe("getCommitFileDiff", () => { expect(file?.deletions).toBe(0); }); }); + +it("identical commit-file requests from two requesters both retain syntax tokens", async () => { + const repoDir = initRepo(); + const original = "const value = Math.max(42, 7);\n".repeat(3_000); + commitFile(repoDir, "file.ts", original, "initial"); + commitFile(repoDir, "file.ts", original.replace("42", "43"), "edit"); + const input = { cwd: repoDir, sha: headSha(repoDir), path: "file.ts" }; + const results = await Promise.all([getCommitFileDiff(input), getCommitFileDiff(input)]); + for (const file of results) { + expect( + file?.hunks[0].lines + .find((line) => line.type === "add") + ?.tokens?.some((token) => token.style !== null), + ).toBe(true); + } +}); + +it("a disconnected requester cancels commit highlighting before its sources arrive", async () => { + const repoDir = initRepo(); + commitFile(repoDir, "file.ts", "const value = 42;\n", "initial"); + commitFile(repoDir, "file.ts", "const value = 43;\n", "edit"); + const delivery = new SessionDelivery(() => {}); + const source = {}; + delivery.attach(source, true); + const request = { + type: "checkout.commits.file_diff.request" as const, + cwd: repoDir, + sha: headSha(repoDir), + path: "file.ts", + requestId: "disconnect", + }; + const results: Awaited>[] = []; + const pending = delivery.request(source, request, async () => { + results.push(await getCommitFileDiff({ ...request, signal: delivery.requestSignal })); + }); + await delivery.detach(source); + await pending; + expect( + results[0]?.hunks.flatMap((hunk) => hunk.lines).every((line) => line.tokens === undefined), + ).toBe(true); +}); diff --git a/packages/server/src/utils/checkout-git.ts b/packages/server/src/utils/checkout-git.ts index 293a5dcc4e..a39b6445e2 100644 --- a/packages/server/src/utils/checkout-git.ts +++ b/packages/server/src/utils/checkout-git.ts @@ -11,7 +11,6 @@ import { maxSecuredPlaintextByteLength } from "@getpaseo/relay"; import type { Logger } from "pino"; import type { ParsedDiffFile } from "../server/utils/diff-highlighter.js"; import { - DiffHighlightRequests, highlightDiffWithFileContent, parseAndHighlightDiff, parseDiff, @@ -2704,8 +2703,6 @@ async function readCommitRenames(cwd: string, sha: string): Promise { - return diffHighlightRequests.run( - JSON.stringify(["commit", resolve(input.cwd), input.sha, input.path]), - (signal) => readCommitFileDiff(input, signal), - ); -} - -async function readCommitFileDiff( - { - cwd, - sha, - path, - }: { - cwd: string; - sha: string; - path: string; - }, - signal: AbortSignal, -): Promise { const worktreeRoot = await requireGitWorktreeRoot(cwd); const renames = await getCommitRenames(worktreeRoot, sha); const oldPath = renames.get(path); @@ -3150,7 +3135,7 @@ export function warmCheckoutShortstatInBackground( } interface AppendStructuredTrackedDiffsInput { - signal: AbortSignal; + signal?: AbortSignal; cwd: string; trackedChanges: CheckoutFileChange[]; trackedNumstatByPath: Map; @@ -3166,7 +3151,7 @@ interface AppendStructuredTrackedDiffsInput { } async function buildHighlightedTrackedDiffFile(input: { - signal: AbortSignal; + signal?: AbortSignal; cwd: string; change: CheckoutFileChange; parsedFile: ParsedDiffFile; @@ -3317,7 +3302,7 @@ async function appendStructuredTrackedDiffs( } interface ProcessUntrackedChangeInput { - signal: AbortSignal; + signal?: AbortSignal; cwd: string; change: CheckoutFileChange; ignoreWhitespace: boolean; @@ -3560,31 +3545,11 @@ async function prepareAllChangesIndex(cwd: string, baseRef: string) { } } -export function getCheckoutDiff( +export async function getCheckoutDiff( cwd: string, compare: CheckoutDiffCompare, context?: CheckoutContext, signal?: AbortSignal, -): Promise { - return diffHighlightRequests.run( - JSON.stringify([ - "checkout", - resolve(cwd), - compare.mode, - compare.mode !== "uncommitted" ? (compare.baseRef ?? null) : null, - compare.ignoreWhitespace === true, - compare.includeStructured === true, - ]), - (highlightSignal) => loadCheckoutDiff(cwd, compare, context, highlightSignal), - signal, - ); -} - -async function loadCheckoutDiff( - cwd: string, - compare: CheckoutDiffCompare, - context: CheckoutContext | undefined, - signal: AbortSignal, ): Promise { cwd = await requireGitWorktreeRoot(cwd); @@ -3614,7 +3579,7 @@ async function readCheckoutDiff( cwd: string, compare: CheckoutDiffCompare, refsForDiff: CheckoutDiffRefs, - signal: AbortSignal, + signal?: AbortSignal, ): Promise { const ignoreWhitespace = compare.ignoreWhitespace === true; let effectiveRefsForDiff = refsForDiff;