From 71013c7f9c5513540e5a7bc6ed0733386c95b5f7 Mon Sep 17 00:00:00 2001 From: lforst <8118419+lforst@users.noreply.github.com> Date: Wed, 23 Sep 2026 14:26:56 +0000 Subject: [PATCH 1/2] fix: Fix dataset snapshots silently including deleted rows --- js/src/logger.test.ts | 170 ++++++++++++++++++++++++++++++++---------- js/src/logger.ts | 10 ++- 2 files changed, 135 insertions(+), 45 deletions(-) diff --git a/js/src/logger.test.ts b/js/src/logger.test.ts index 84781b5b8..dd35b6111 100644 --- a/js/src/logger.test.ts +++ b/js/src/logger.test.ts @@ -1286,6 +1286,7 @@ test("dataset.toEvalData preserves dataset_environment", async () => { state, }); + await expect(dataset.version()).resolves.toBe("123"); await expect(dataset.toEvalData()).resolves.toEqual({ dataset_id: "00000000-0000-0000-0000-000000000002", dataset_environment: "production", @@ -1328,6 +1329,7 @@ test("dataset.toEvalData preserves dataset_snapshot_name", async () => { state, }); + await expect(dataset.version()).resolves.toBe("456"); await expect(dataset.toEvalData()).resolves.toEqual({ dataset_id: "00000000-0000-0000-0000-000000000002", dataset_snapshot_name: "123", @@ -1408,22 +1410,96 @@ test("dataset.version preserves pinned-version fast path", async () => { vi.restoreAllMocks(); }); -test("dataset.createSnapshot forwards update when requested", async () => { - const state = await _exportsForTestingOnly.simulateLoginForTests(); - vi.spyOn(state, "login").mockResolvedValue(state as any); - const postJson = vi - .spyOn(state.appConn(), "post_json") - .mockResolvedValueOnce({ - project: { - id: "00000000-0000-0000-0000-000000000001", - name: "test-project", - }, - dataset: { - id: "00000000-0000-0000-0000-000000000002", - name: "test-dataset", - }, - }) - .mockResolvedValueOnce({ +test.each([ + ["surviving rows", false, false], + ["all rows deleted", true, false], + ["cached rows", true, true], +])( + "dataset.createSnapshot uses a server transaction after deletes (%s)", + async (_name, allDeleted, cacheRows) => { + const state = await _exportsForTestingOnly.simulateLoginForTests(); + vi.spyOn(state, "login").mockResolvedValue(state as any); + const postJson = vi + .spyOn(state.appConn(), "post_json") + .mockResolvedValueOnce({ + project: { id: "project-id", name: "test-project" }, + dataset: { id: "dataset-id", name: "test-dataset" }, + }) + .mockResolvedValueOnce({ + dataset_snapshot: { + id: "00000000-0000-0000-0000-000000000004", + dataset_id: "00000000-0000-0000-0000-000000000002", + name: "after-delete", + description: null, + xact_id: "1000197874946873171", + created: "2026-03-31T00:00:00.000Z", + }, + found_existing: false, + }); + vi.spyOn(state.apiConn(), "post").mockResolvedValue( + new Response( + JSON.stringify({ + data: + allDeleted && !cacheRows + ? [] + : [{ id: "a", input: "a", _xact_id: "100" }], + }), + ), + ); + const get = vi + .spyOn(state.apiConn(), "get") + .mockResolvedValue(new Response("1000197874946873171")); + const dataset = initDataset({ + project: "test-project", + dataset: "test-dataset", + state, + }); + const flush = vi.spyOn(dataset, "flush").mockResolvedValue(); + const version = vi.spyOn(dataset, "version"); + + try { + if (cacheRows) { + await dataset.fetchedData(); + } + await dataset.createSnapshot({ name: "after-delete" }); + expect(version).not.toHaveBeenCalled(); + expect(get).toHaveBeenCalledWith("xact-id"); + expect(flush.mock.invocationCallOrder[0]).toBeLessThan( + get.mock.invocationCallOrder[0], + ); + expect(postJson).toHaveBeenLastCalledWith( + "api/dataset_snapshot/register", + expect.objectContaining({ xact_id: "1000197874946873171" }), + ); + } finally { + _exportsForTestingOnly.simulateLogoutForTests(); + vi.restoreAllMocks(); + } + }, +); + +test.each([ + { version: "123" }, + { snapshotName: "saved" }, + { environment: "production" }, +])( + "dataset.createSnapshot preserves pins and forwards update (%j)", + async (pin) => { + const state = await _exportsForTestingOnly.simulateLoginForTests(); + vi.spyOn(state, "login").mockResolvedValue(state as any); + const postJson = vi + .spyOn(state.appConn(), "post_json") + .mockResolvedValueOnce({ + project: { + id: "00000000-0000-0000-0000-000000000001", + name: "test-project", + }, + dataset: { + id: "00000000-0000-0000-0000-000000000002", + name: "test-dataset", + }, + }); + const snapshot = { dataset_snapshot: { id: "00000000-0000-0000-0000-000000000004", dataset_id: "00000000-0000-0000-0000-000000000002", @@ -1433,37 +1509,49 @@ test("dataset.createSnapshot forwards update when requested", async () => { created: "2026-03-31T00:00:00.000Z", }, found_existing: true, + }; + if (pin.snapshotName) { + postJson.mockResolvedValueOnce([snapshot.dataset_snapshot]); + } + postJson.mockResolvedValueOnce(snapshot); + vi.spyOn(state.apiConn(), "get_json").mockResolvedValue({ + object_version: "123", }); + const get = vi + .spyOn(state.apiConn(), "get") + .mockRejectedValue(new Error("Unexpected transaction request")); - const dataset = initDataset({ - project: "test-project", - dataset: "test-dataset", - version: "123", - state, - }); + const dataset = initDataset({ + project: "test-project", + dataset: "test-dataset", + ...pin, + state, + }); - await expect( - dataset.createSnapshot({ - name: "snapshot", + await expect( + dataset.createSnapshot({ + name: "snapshot", + description: "updated description", + update: true, + }), + ).resolves.toMatchObject({ + id: "00000000-0000-0000-0000-000000000004", + xact_id: "123", + }); + + expect(get).not.toHaveBeenCalled(); + expect(postJson).toHaveBeenLastCalledWith("api/dataset_snapshot/register", { + dataset_id: "00000000-0000-0000-0000-000000000002", + dataset_snapshot_name: "snapshot", description: "updated description", + xact_id: "123", update: true, - }), - ).resolves.toMatchObject({ - id: "00000000-0000-0000-0000-000000000004", - xact_id: "123", - }); - - expect(postJson).toHaveBeenNthCalledWith(2, "api/dataset_snapshot/register", { - dataset_id: "00000000-0000-0000-0000-000000000002", - dataset_snapshot_name: "snapshot", - description: "updated description", - xact_id: "123", - update: true, - }); + }); - _exportsForTestingOnly.simulateLogoutForTests(); - vi.restoreAllMocks(); -}); + _exportsForTestingOnly.simulateLogoutForTests(); + vi.restoreAllMocks(); + }, +); test("dataset.getSnapshot looks up snapshots by name", async () => { const state = await _exportsForTestingOnly.simulateLoginForTests(); diff --git a/js/src/logger.ts b/js/src/logger.ts index e18756048..dfd96db48 100644 --- a/js/src/logger.ts +++ b/js/src/logger.ts @@ -9099,10 +9099,12 @@ export class Dataset< await this.flush(); const state = await this.getState(); const datasetId = await this.id; - const currentVersion = await this.version(); - if (currentVersion === undefined) { - throw new Error("Cannot create snapshot: dataset has no version"); - } + // Live-row versions omit deletions. After flushing, get a server transaction + // boundary that includes every completed write, even if no live rows remain. + // getState() above also resolves versions pinned by snapshot or environment. + const currentVersion = + this.getPinnedVersion() ?? + (await (await state.apiConn().get("xact-id")).text()); const response = await state .appConn() .post_json("api/dataset_snapshot/register", { From d4e1901f30d688364c0aca60f9c0f2acdb91ceb0 Mon Sep 17 00:00:00 2001 From: lforst <8118419+lforst@users.noreply.github.com> Date: Wed, 23 Sep 2026 14:37:46 +0000 Subject: [PATCH 2/2] Update PR #2512 --- .changeset/wise-canyons-stare.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/wise-canyons-stare.md diff --git a/.changeset/wise-canyons-stare.md b/.changeset/wise-canyons-stare.md new file mode 100644 index 000000000..b499a281f --- /dev/null +++ b/.changeset/wise-canyons-stare.md @@ -0,0 +1,5 @@ +--- +"braintrust": patch +--- + +fix: Fix dataset snapshots silently including deleted rows