From 9fefdd6e5ffe73726677eabf3150e42da6b4d69a Mon Sep 17 00:00:00 2001 From: Yaacov Date: Wed, 11 Nov 2026 23:49:59 +0200 Subject: [PATCH] fix(server): overlapping status reads share one untracked listing Follow-ups to the untracked backoff and Codex kill deadline, both now merged in T3: overlapping reads share one running listing, the remembered failure keeps the listing error as its cause, and the kill-deadline test waits for its stand-in's ready line. Co-Authored-By: Claude Opus 5.5 --- UPSTREAM.md | 10 ++- .../Adapters/CodexAdapterV2.scient.test.ts | 9 ++- .../src/vcs/GitVcsDriverCore.scient.test.ts | 61 +++++++++++++++++++ apps/server/src/vcs/GitVcsDriverCore.ts | 31 +++++++--- 4 files changed, 100 insertions(+), 11 deletions(-) diff --git a/UPSTREAM.md b/UPSTREAM.md index 16d7b8cf11..9b8a0bdd3f 100644 --- a/UPSTREAM.md +++ b/UPSTREAM.md @@ -2078,10 +2078,14 @@ Marked blocks in T3-owned files, to offer upstream and then drop: `GitVcsDriverCore.scient.test.ts`. Asked on pingdotgg/t3code#34042, which reworks these fetches. - `GitVcsDriverCore.ts`: branch change totals remember a failed untracked-file listing per folder and back off (1 min doubling to 30 min) until the untracked roots change, and keep the failure's - real reason. Tested in `GitVcsDriverCore.scient.test.ts`. Offered as pingdotgg/t3code#34460 - (issue #34457). + real reason. Tested in `GitVcsDriverCore.scient.test.ts`. Merged in T3 as + pingdotgg/t3code#34460 (issue #34457). Follow-up in the same blocks: overlapping status reads + share one running listing, and the error keeps the listing's `GitCommandError` as `cause`; + offered as pingdotgg/t3code#34574. - `CodexAdapterV2.ts`: the app-server gets a 2 s force-kill deadline. Tested in - `CodexAdapterV2.scient.test.ts`. Offered as pingdotgg/t3code#34461 (issue #34458). + `CodexAdapterV2.scient.test.ts`, which waits for the stand-in's `ready` line before stopping it. + Merged in T3 as pingdotgg/t3code#34461 (issue #34458); the test's wait offered as + pingdotgg/t3code#34575. - `ClaudeAdapterV2.ts` (`spawnClaudeProcessGroup`): descendants that left the CLI's process group (its detached Bash tool shells and helpers) stop with it. While the server runs, the CLI's tree is sampled and stopped after the CLI exits (`@t3tools/shared/scientOwnedProcessTree`); if the diff --git a/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.scient.test.ts b/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.scient.test.ts index 9b937461a0..0260dd4a12 100644 --- a/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.scient.test.ts +++ b/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.scient.test.ts @@ -11,6 +11,7 @@ import * as CodexReplay from "effect-codex-app-server/replay"; import * as DateTime from "effect/DateTime"; import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Option from "effect/Option"; import * as Predicate from "effect/Predicate"; import * as FileSystem from "effect/FileSystem"; import * as Fiber from "effect/Fiber"; @@ -4685,7 +4686,13 @@ describe("Codex restored Policy", () => { } catch {} }; yield* Effect.addFinalizer(() => Effect.sync(killGroup)); - yield* child.stdout.pipe(Stream.take(1), Stream.runDrain); + // Stop it only once its SIGTERM trap is set; an early exit would pass without testing it. + const firstLine = yield* child.stdout.pipe( + Stream.decodeText(), + Stream.splitLines, + Stream.runHead, + ); + assert.deepStrictEqual(firstLine, Option.some("ready")); // Scope.close cannot be interrupted, so a regression is ended by this rescue instead of // hanging the test; the deadline under test must make it unnecessary. let rescued = false; diff --git a/apps/server/src/vcs/GitVcsDriverCore.scient.test.ts b/apps/server/src/vcs/GitVcsDriverCore.scient.test.ts index b69bbdc6b7..2d42029482 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.scient.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.scient.test.ts @@ -2,7 +2,9 @@ import * as NodeCrypto from "@effect/platform-node/NodeCrypto"; import * as NodeServices from "@effect/platform-node/NodeServices"; import { assert, it } from "@effect/vitest"; import * as HostProcess from "@t3tools/shared/HostProcess"; +import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Path from "effect/Path"; @@ -161,6 +163,65 @@ it.layer(layerTest)("branch change totals", (it) => { }), ); + // A slow listing would otherwise be repeated by every read that a file change starts meanwhile. + it.effect("share one listing between overlapping status reads", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const outer = yield* GitVcsDriver.GitVcsDriver; + const git = (cwd: string, args: ReadonlyArray) => + outer.execute({ operation: "GitVcsDriver.test.git", cwd, args, timeoutMs: 10_000 }); + const cwd = yield* fs.makeTempDirectoryScoped({ prefix: "git-untracked-shared-" }); + yield* git(cwd, ["init", "--quiet", "--initial-branch=main"]); + yield* git(cwd, ["config", "user.email", "test@test.com"]); + yield* git(cwd, ["config", "user.name", "Test"]); + yield* fs.writeFileString(path.join(cwd, "README.md"), "# test\n"); + yield* git(cwd, ["add", "."]); + yield* git(cwd, ["commit", "--quiet", "-m", "initial"]); + yield* fs.writeFileString(path.join(cwd, "notes.txt"), "one\ntwo\n"); + + const firstListing = yield* Deferred.make(); + const release = yield* Deferred.make(); + const secondReadAtListing = yield* Deferred.make(); + let listings = 0; + let branchChecks = 0; + const delegate = yield* ChildProcessSpawner.ChildProcessSpawner; + const spawner = ChildProcessSpawner.make((command) => + Effect.gen(function* () { + if (isUntrackedListing(command) && ++listings === 1) { + yield* Deferred.succeed(firstListing, undefined); + yield* Deferred.await(release); + } + const handle = yield* delegate.spawn(command); + // A read's branch check is its last Git command before the listing. + const args = ChildProcess.isStandardCommand(command) ? command.args.map(String) : []; + if (!args.includes("refs/heads/main") || ++branchChecks !== 2) return handle; + return ChildProcessSpawner.makeHandle({ + ...handle, + exitCode: handle.exitCode.pipe( + Effect.tap(() => Deferred.succeed(secondReadAtListing, undefined)), + ), + }); + }), + ); + const driver = yield* makeGitVcsDriverCore().pipe( + Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), + Effect.provide(layerServerConfig), + ); + const read = driver.statusDetailsLocal(cwd, { includeBranchChanges: true }); + + const first = yield* Effect.forkChild(read); + yield* Deferred.await(firstListing); + const second = yield* Effect.forkChild(read); + yield* Deferred.await(secondReadAtListing); + yield* Deferred.succeed(release, undefined); + const totals = { baseRef: null, insertions: 2, deletions: 0 }; + assert.deepStrictEqual((yield* Fiber.join(first)).branchChanges, totals); + assert.deepStrictEqual((yield* Fiber.join(second)).branchChanges, totals); + assert.strictEqual(listings, 1, "the second read used the listing already running"); + }), + ); + // Git ends a porcelain fetch with `maintenance run --auto`, detached by default: outside the // server's process group, it survives a quit, and on some Git versions several repack one // repository at once. The repository below would repack on its next fetch (two packs, limit diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index f6cbad4a53..b77a872b75 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -1669,6 +1669,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* readonly count: number; readonly retryAt: number; readonly detail: string; + readonly cause: GitCommandError | undefined; } >(); // SCIENT-FORK:END @@ -3199,21 +3200,34 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* }); // SCIENT-FORK:START — branch totals stop rescanning untracked files after a failed scan. + // Overlapping reads of the same paths share one listing instead of each walking the tree; the + // result is never kept, so a later read lists again. + const untrackedListings = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", ...pathArgs] = key.split("\0"); + return executeGit( + "GitVcsDriver.review.listUntracked", + cwd, + ["ls-files", "--others", "--exclude-standard", "-z", "--", ...pathArgs], + { maxOutputBytes: REVIEW_METADATA_MAX_OUTPUT_BYTES }, + ); + }, + { capacity: STATUS_UPSTREAM_REFRESH_CACHE_CAPACITY, timeToLive: () => Duration.zero }, + ); + // `unavailable` says why the untracked files could not be indexed. const prepareUntrackedReviewIndexOrReason = Effect.fn("prepareUntrackedReviewIndex")(function* ( cwd: string, pathArgs: ReadonlyArray, onlyPath?: string, ) { - const untracked = yield* executeGit( - "GitVcsDriver.review.listUntracked", - cwd, - ["ls-files", "--others", "--exclude-standard", "-z", "--", ...pathArgs], - { maxOutputBytes: REVIEW_METADATA_MAX_OUTPUT_BYTES }, - ).pipe( + const untracked = yield* Cache.get(untrackedListings, [cwd, ...pathArgs].join("\0")).pipe( Effect.catchTags({ GitCommandError: (error) => - Effect.succeed({ unavailable: `Could not list untracked files: ${error.detail}` }), + Effect.succeed({ + unavailable: `Could not list untracked files: ${error.detail}`, + cause: error, + }), }), ); if ("unavailable" in untracked) return untracked; @@ -3338,6 +3352,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* command: "git ls-files", cwd, detail: remembered.detail, + ...(remembered.cause === undefined ? {} : { cause: remembered.cause }), }); } // Empty status can mean untracked files are hidden; retain the full scan. @@ -3353,6 +3368,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* (yield* Clock.currentTimeMillis) + Math.min(UNTRACKED_TOTALS_RETRY_MIN_MS * 2 ** (count - 1), UNTRACKED_TOTALS_RETRY_MAX_MS), detail: untracked.unavailable, + cause: "cause" in untracked ? untracked.cause : undefined, }); if (untrackedTotalsFailures.size > STATUS_UPSTREAM_REFRESH_CACHE_CAPACITY) { const oldestKey = untrackedTotalsFailures.keys().next().value; @@ -3363,6 +3379,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* command: "git ls-files", cwd, detail: untracked.unavailable, + ...("cause" in untracked ? { cause: untracked.cause } : {}), }); } untrackedTotalsFailures.delete(cwd);