Skip to content

Commit cf7f739

Browse files
Merge pull request #1528 from corbitsdev/cl-10916-commit-same-second-segment-rewrites-and-in-place-tool-output
fix(session): commit same-second and in-place session edits
2 parents 4184e0d + 42f8f5b commit cf7f739

4 files changed

Lines changed: 358 additions & 46 deletions

File tree

‎scripts/context-store-perf.test.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,12 @@ import { createOptimizedContextStore } from "../src/session/optimized-context-st
55
import { createTempDir } from "../testkit/temporary-dirs.js";
66

77
/**
8-
* Pins the per-turn slope of fs.promises.access, counted the same way
8+
* Pins the slope of fs.promises.access, counted the same way
99
* scripts/context-store-perf.ts installCounters counts it (every call, and a
1010
* separate tally when the path is under .git). Synchronous fs is invisible
1111
* here, same as that script. Two turn counts keep a fixed overhead from hiding
12-
* a probe that runs once per turn.
12+
* a probe that runs once per turn. The commit path no longer pays one access
13+
* per turn, so both slopes stay flat.
1314
*/
1415
const LOW_TURNS = 4;
1516
const HIGH_TURNS = 12;
@@ -92,7 +93,7 @@ afterEach(() => {
9293
totals.gitAccess = 0;
9394
});
9495

95-
test("fs.promises.access grows by a fixed per-turn slope", async () => {
96+
test("fs.promises.access slope is flat, not per-turn", async () => {
9697
installCounters();
9798
await runScenario(LOW_TURNS);
9899
const low = { access: totals.access, gitAccess: totals.gitAccess };
@@ -101,6 +102,6 @@ test("fs.promises.access grows by a fixed per-turn slope", async () => {
101102
await runScenario(HIGH_TURNS);
102103
const high = { access: totals.access, gitAccess: totals.gitAccess };
103104
const span = HIGH_TURNS - LOW_TURNS;
104-
expect((high.access - low.access) / span).toBe(1);
105+
expect((high.access - low.access) / span).toBe(0);
105106
expect((high.gitAccess - low.gitAccess) / span).toBe(0);
106107
});

‎scripts/results/session-store-perf-baseline.json‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,51 +10,51 @@
1010
],
1111
"contextStorePerf": {
1212
"turnCount": 200,
13-
"wallMs": 744,
13+
"wallMs": 814,
1414
"loadedTurns": 250,
15-
"totalFsOps": 21396,
16-
"gitPathFsOps": 10385,
15+
"totalFsOps": 15736,
16+
"gitPathFsOps": 7717,
1717
"fsOps": {
18-
"readFile": 10881,
19-
"mkdir": 247,
20-
"writeFile": 1275,
21-
"stat": 3141,
22-
"lstat": 4249,
23-
"readdir": 1203,
24-
"access": 202,
18+
"readFile": 7878,
19+
"mkdir": 246,
20+
"writeFile": 1274,
21+
"stat": 2580,
22+
"lstat": 2948,
23+
"readdir": 611,
24+
"access": 1,
2525
"open": 198
2626
},
2727
"gitPathFsOpsByName": {
28-
"stat": 1273,
29-
"mkdir": 241,
30-
"writeFile": 1269,
31-
"lstat": 1324,
32-
"readFile": 6278
28+
"stat": 679,
29+
"mkdir": 240,
30+
"writeFile": 1268,
31+
"lstat": 1067,
32+
"readFile": 4463
3333
},
3434
"perTurn": {
35-
"wallMs": 3.72,
36-
"fsOps": 106.98
35+
"wallMs": 4.07,
36+
"fsOps": 78.68
3737
}
3838
},
3939
"turnPersistenceCensus": {
4040
"flow": "coalesced",
4141
"turnCount": 200,
42-
"wallMs": 642,
42+
"wallMs": 696,
4343
"loadedTurns": 800,
44-
"totalWriteOps": 2088,
45-
"gitPathWriteOps": 1678,
44+
"totalWriteOps": 2097,
45+
"gitPathWriteOps": 1687,
4646
"byName": {
47-
"mkdir": 246,
48-
"writeFile": 1642,
47+
"mkdir": 251,
48+
"writeFile": 1646,
4949
"appendFile": 200
5050
},
5151
"gitByPath": {
52-
"mkdir": 240,
53-
"writeFile": 1438
52+
"mkdir": 245,
53+
"writeFile": 1442
5454
},
5555
"perTurn": {
56-
"wallMs": 3.21,
57-
"writeOps": 10.44
56+
"wallMs": 3.48,
57+
"writeOps": 10.48
5858
},
5959
"persistCalls": {
6060
"writePrompt": 200,

‎src/session/optimized-context-store.test.ts‎

Lines changed: 165 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,107 @@ describe("createOptimizedContextStore checkpoint", () => {
742742
expect(new TextDecoder().decode(blob)).toContain(marker);
743743
});
744744

745+
test("a same-length rewrite of turns-0001.jsonl inside the same mtime second is in the next commit", async () => {
746+
const dir = tempDir();
747+
const store = await createOptimizedContextStore(dir);
748+
const chunk = "s".repeat(100_000);
749+
const turns: ConversationTurn[] = [];
750+
const closed = segmentFileName(TURNS_FILE, 1);
751+
const tail = segmentFileName(TURNS_FILE, 2);
752+
do {
753+
turns.push(turn(`${turns.length}-${chunk}`));
754+
await store.writeTurns(turns);
755+
} while (!fs.existsSync(path.join(dir, tail)) && turns.length < 20);
756+
expect(fs.existsSync(path.join(dir, closed))).toBe(true);
757+
await store.writeMetadata(EMPTY_CHECKPOINT_METADATA);
758+
const first = await store.commit({ message: "three segments" });
759+
760+
const closedPath = path.join(dir, closed);
761+
const before = fs.statSync(closedPath);
762+
const marker = "same-second-rewrite-marker";
763+
const original = fs.readFileSync(closedPath, "utf8");
764+
const at = original.indexOf("s".repeat(marker.length));
765+
expect(at).toBeGreaterThanOrEqual(0);
766+
const rewritten =
767+
original.slice(0, at) + marker + original.slice(at + marker.length);
768+
expect(rewritten.length).toBe(original.length);
769+
fs.writeFileSync(closedPath, rewritten);
770+
const nudged = nudgedSameSecondMs(before.mtimeMs);
771+
fs.utimesSync(closedPath, before.atime, new Date(nudged));
772+
const after = fs.statSync(closedPath);
773+
// The racy-git window: same inode, same size, same whole mtime second.
774+
// statusMatrix then trusts the index oid and misses the new bytes.
775+
expect(after.ino).toBe(before.ino);
776+
expect(after.size).toBe(before.size);
777+
expect(Math.floor(after.mtimeMs / 1000)).toBe(
778+
Math.floor(before.mtimeMs / 1000),
779+
);
780+
expect(after.mtimeMs).not.toBe(before.mtimeMs);
781+
782+
let secondHash = "";
783+
await pinLstatCtime(closedPath, before.ctimeMs, async () => {
784+
const second = await store.commit({
785+
message: "publish same-second rewrite",
786+
});
787+
secondHash = second.hash;
788+
});
789+
expect(secondHash).not.toBe(first.hash);
790+
const published = await git.readBlob({
791+
fs,
792+
dir,
793+
oid: secondHash,
794+
filepath: closed,
795+
});
796+
expect(new TextDecoder().decode(published.blob)).toContain(marker);
797+
798+
const third = await store.commit({ message: "no-op after same-second" });
799+
expect(third.hash).toBe(secondHash);
800+
const kept = await git.readBlob({
801+
fs,
802+
dir,
803+
oid: third.hash,
804+
filepath: closed,
805+
});
806+
expect(new TextDecoder().decode(kept.blob)).toContain(marker);
807+
});
808+
809+
test("an in-place rewrite of an existing tool-output file is in the next commit", async () => {
810+
const dir = tempDir();
811+
const store = await createOptimizedContextStore(dir);
812+
await store.writeBlob(
813+
"kept",
814+
new TextEncoder().encode("original"),
815+
"text/plain",
816+
);
817+
await store.writeTurns([turn("seed")]);
818+
await store.writeMetadata(EMPTY_CHECKPOINT_METADATA);
819+
const first = await store.commit({ message: "seed blob" });
820+
821+
const blobName = fs
822+
.readdirSync(path.join(dir, "tool-output"))
823+
.find((name) => name.startsWith("kept"));
824+
if (blobName === undefined) throw new Error("seeded blob missing");
825+
const blobPath = path.join(dir, "tool-output", blobName);
826+
const before = fs.statSync(blobPath);
827+
const dirBefore = fs.statSync(path.join(dir, "tool-output"));
828+
fs.writeFileSync(blobPath, "rewritten-in-place");
829+
const after = fs.statSync(blobPath);
830+
const dirAfter = fs.statSync(path.join(dir, "tool-output"));
831+
expect(after.ino).toBe(before.ino);
832+
expect(dirAfter.mtimeMs).toBe(dirBefore.mtimeMs);
833+
expect(dirAfter.ino).toBe(dirBefore.ino);
834+
835+
const second = await store.commit({ message: "publish in-place blob" });
836+
expect(second.hash).not.toBe(first.hash);
837+
const { blob } = await git.readBlob({
838+
fs,
839+
dir,
840+
oid: second.hash,
841+
filepath: `tool-output/${blobName}`,
842+
});
843+
expect(new TextDecoder().decode(blob)).toBe("rewritten-in-place");
844+
});
845+
745846
test("a turn commit does not lstat every seeded tool-output blob", async () => {
746847
const dir = tempDir();
747848
const store = await createOptimizedContextStore(dir);
@@ -788,12 +889,34 @@ describe("createOptimizedContextStore checkpoint", () => {
788889
await store.writeMetadata(EMPTY_CHECKPOINT_METADATA);
789890
await store.commit({ message: "seed blobs" });
790891

791-
const lstats = await countPathOps(
792-
"lstat",
793-
`${path.sep}tool-output${path.sep}`,
794-
() => store.commit({ message: "no writes" }),
795-
);
796-
expect(lstats).toBe(0);
892+
const walkedToolOutput: string[][] = [];
893+
const originalStatus = git.statusMatrix.bind(git);
894+
git.statusMatrix = (async (args) => {
895+
const filepaths = args.filepaths ?? [];
896+
if (
897+
filepaths.some(
898+
(filepath) =>
899+
filepath === "tool-output" || filepath.startsWith("tool-output/"),
900+
)
901+
) {
902+
walkedToolOutput.push([...filepaths]);
903+
}
904+
return originalStatus(args);
905+
}) as typeof git.statusMatrix;
906+
let lstats = 0;
907+
try {
908+
lstats = await countPathOps(
909+
"lstat",
910+
`${path.sep}tool-output${path.sep}`,
911+
() => store.commit({ message: "no writes" }),
912+
);
913+
} finally {
914+
git.statusMatrix = originalStatus;
915+
}
916+
// Known blobs are statted by fingerprint. The directory itself is not
917+
// passed to statusMatrix, so children are not walked a second time.
918+
expect(walkedToolOutput).toEqual([]);
919+
expect(lstats).toBe(blobCount);
797920
});
798921

799922
test("seeded tool-output does not commit a no-op and a fresh store still heals blob bytes", async () => {
@@ -1766,6 +1889,42 @@ function readTurnsFromFile(fullPath: string): ConversationTurn[] {
17661889
.map((line) => JSON.parse(line) as ConversationTurn);
17671890
}
17681891

1892+
function nudgedSameSecondMs(mtimeMs: number): number {
1893+
const nudged = mtimeMs + 1;
1894+
if (Math.floor(nudged / 1000) === Math.floor(mtimeMs / 1000)) return nudged;
1895+
return mtimeMs - 1;
1896+
}
1897+
1898+
/**
1899+
* isomorphic-git compareStats ignores sub-second mtime but still compares
1900+
* ctime seconds. A real rewrite bumps ctime to "now", which leaves the
1901+
* same-second window whenever the test crosses a second boundary. Report the
1902+
* pre-rewrite ctime for this one path so statusMatrix trusts the index oid.
1903+
*/
1904+
async function pinLstatCtime<T>(
1905+
filePath: string,
1906+
ctimeMs: number,
1907+
run: () => Promise<T>,
1908+
): Promise<T> {
1909+
const original = fs.promises.lstat;
1910+
fs.promises.lstat = (async (target: fs.PathLike, opts?: fs.StatOptions) => {
1911+
const stats = await original.call(fs.promises, target, opts);
1912+
if (typeof target !== "string" || target !== filePath) return stats;
1913+
const pinned = Object.assign(
1914+
Object.create(Object.getPrototypeOf(stats)),
1915+
stats,
1916+
) as fs.Stats;
1917+
pinned.ctimeMs = ctimeMs;
1918+
pinned.ctime = new Date(ctimeMs);
1919+
return pinned;
1920+
}) as typeof fs.promises.lstat;
1921+
try {
1922+
return await run();
1923+
} finally {
1924+
fs.promises.lstat = original;
1925+
}
1926+
}
1927+
17691928
async function countPathOps(
17701929
op: "readFile" | "lstat",
17711930
marker: string,

0 commit comments

Comments
 (0)