From 5817bd1def537b43d5cd20de9cd08537a33d8f35 Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Sun, 16 Aug 2026 23:15:51 +0200 Subject: [PATCH 1/3] Anchor the async local storage instances to global symbols (#97255) --- .../action-async-storage-instance.ts | 4 +- .../after-task-async-storage-instance.ts | 4 +- .../server/app-render/async-local-storage.ts | 37 ++++++ .../console-async-storage-instance.ts | 4 +- .../dynamic-access-async-storage-instance.ts | 4 +- .../app-render/work-async-storage-instance.ts | 4 +- .../work-unit-async-storage-instance.ts | 4 +- .../lib/trace/request-insights-identity.ts | 13 +-- .../dev-warmup.util.ts | 110 +++++++----------- .../cache-components-tasks.test.ts | 28 +---- 10 files changed, 91 insertions(+), 121 deletions(-) diff --git a/packages/next/src/server/app-render/action-async-storage-instance.ts b/packages/next/src/server/app-render/action-async-storage-instance.ts index 64aae6f9b720..7212566beac7 100644 --- a/packages/next/src/server/app-render/action-async-storage-instance.ts +++ b/packages/next/src/server/app-render/action-async-storage-instance.ts @@ -1,5 +1,5 @@ import type { ActionAsyncStorage } from './action-async-storage.external' -import { createAsyncLocalStorage } from './async-local-storage' +import { getOrCreateGlobalAsyncLocalStorage } from './async-local-storage' export const actionAsyncStorageInstance: ActionAsyncStorage = - createAsyncLocalStorage() + getOrCreateGlobalAsyncLocalStorage('action-async-storage') diff --git a/packages/next/src/server/app-render/after-task-async-storage-instance.ts b/packages/next/src/server/app-render/after-task-async-storage-instance.ts index 0e5ecbeb2606..f5e54a97e13e 100644 --- a/packages/next/src/server/app-render/after-task-async-storage-instance.ts +++ b/packages/next/src/server/app-render/after-task-async-storage-instance.ts @@ -1,5 +1,5 @@ import type { AfterTaskAsyncStorage } from './after-task-async-storage.external' -import { createAsyncLocalStorage } from './async-local-storage' +import { getOrCreateGlobalAsyncLocalStorage } from './async-local-storage' export const afterTaskAsyncStorageInstance: AfterTaskAsyncStorage = - createAsyncLocalStorage() + getOrCreateGlobalAsyncLocalStorage('after-task-async-storage') diff --git a/packages/next/src/server/app-render/async-local-storage.ts b/packages/next/src/server/app-render/async-local-storage.ts index 664337b51456..804be49c0862 100644 --- a/packages/next/src/server/app-render/async-local-storage.ts +++ b/packages/next/src/server/app-render/async-local-storage.ts @@ -45,6 +45,43 @@ export function createAsyncLocalStorage< return new FakeAsyncLocalStorage() } +/** + * Returns the storage registered under `name`, and creates it on first use. + * + * These storages must be singletons within a realm. A store that is entered + * through one reference to a storage must be readable through every other + * reference to it. If it is not, code that runs inside the scope sees no store + * at all. Module identity does not guarantee this. A realm can evaluate the + * same `next` file more than once if the package is reachable through more than + * one path, and then each evaluation creates a storage of its own. A global + * symbol keeps the singleton intact for any number of copies. Worker threads + * and edge sandboxes still get separate storages, because each of them has its + * own `globalThis`. + * + * Module identity broke this way in `next dev`. A bug in Node's + * `fs.realpathSync` can return a path with its symlinks unresolved, and the + * module loader keys the module cache on that path. On a pnpm install it then + * resolves `next/dist/...` through the `node_modules/next` symlink instead of + * the real path, so the file is evaluated a second time. See + * https://github.com/nodejs/node/pull/65113. Node versions without that fix + * stay affected. + * + * The key includes the Next.js version, so two different versions of Next.js in + * the same realm keep separate storages. Their store shapes might not be + * compatible. + */ +export function getOrCreateGlobalAsyncLocalStorage( + name: string +): AsyncLocalStorage { + const key = Symbol.for(`@next/${name}@${process.env.__NEXT_VERSION}`) + + const globalStore = globalThis as typeof globalThis & { + [key: symbol]: AsyncLocalStorage | undefined + } + + return (globalStore[key] ??= createAsyncLocalStorage()) +} + export function bindSnapshot( // WARNING: Don't pass a named function to this argument! See: https://github.com/facebook/react/pull/34911 fn: T diff --git a/packages/next/src/server/app-render/console-async-storage-instance.ts b/packages/next/src/server/app-render/console-async-storage-instance.ts index 7ef5d8871288..e507f153992c 100644 --- a/packages/next/src/server/app-render/console-async-storage-instance.ts +++ b/packages/next/src/server/app-render/console-async-storage-instance.ts @@ -1,5 +1,5 @@ -import { createAsyncLocalStorage } from './async-local-storage' +import { getOrCreateGlobalAsyncLocalStorage } from './async-local-storage' import type { ConsoleAsyncStorage } from './console-async-storage.external' export const consoleAsyncStorageInstance: ConsoleAsyncStorage = - createAsyncLocalStorage() + getOrCreateGlobalAsyncLocalStorage('console-async-storage') diff --git a/packages/next/src/server/app-render/dynamic-access-async-storage-instance.ts b/packages/next/src/server/app-render/dynamic-access-async-storage-instance.ts index 4364e4447ae2..fa8ed7d81412 100644 --- a/packages/next/src/server/app-render/dynamic-access-async-storage-instance.ts +++ b/packages/next/src/server/app-render/dynamic-access-async-storage-instance.ts @@ -1,5 +1,5 @@ -import { createAsyncLocalStorage } from './async-local-storage' +import { getOrCreateGlobalAsyncLocalStorage } from './async-local-storage' import type { DynamicAccessStorage } from './dynamic-access-async-storage.external' export const dynamicAccessAsyncStorageInstance: DynamicAccessStorage = - createAsyncLocalStorage() + getOrCreateGlobalAsyncLocalStorage('dynamic-access-async-storage') diff --git a/packages/next/src/server/app-render/work-async-storage-instance.ts b/packages/next/src/server/app-render/work-async-storage-instance.ts index b3e352daa09e..6722b3e222a4 100644 --- a/packages/next/src/server/app-render/work-async-storage-instance.ts +++ b/packages/next/src/server/app-render/work-async-storage-instance.ts @@ -1,5 +1,5 @@ import type { WorkAsyncStorage } from './work-async-storage.external' -import { createAsyncLocalStorage } from './async-local-storage' +import { getOrCreateGlobalAsyncLocalStorage } from './async-local-storage' export const workAsyncStorageInstance: WorkAsyncStorage = - createAsyncLocalStorage() + getOrCreateGlobalAsyncLocalStorage('work-async-storage') diff --git a/packages/next/src/server/app-render/work-unit-async-storage-instance.ts b/packages/next/src/server/app-render/work-unit-async-storage-instance.ts index c30ae013142e..acfa603b2573 100644 --- a/packages/next/src/server/app-render/work-unit-async-storage-instance.ts +++ b/packages/next/src/server/app-render/work-unit-async-storage-instance.ts @@ -1,5 +1,5 @@ -import { createAsyncLocalStorage } from './async-local-storage' +import { getOrCreateGlobalAsyncLocalStorage } from './async-local-storage' import type { WorkUnitAsyncStorage } from './work-unit-async-storage.external' export const workUnitAsyncStorageInstance: WorkUnitAsyncStorage = - createAsyncLocalStorage() + getOrCreateGlobalAsyncLocalStorage('work-unit-async-storage') diff --git a/packages/next/src/server/lib/trace/request-insights-identity.ts b/packages/next/src/server/lib/trace/request-insights-identity.ts index 2da05e0fea48..d931eef43d92 100644 --- a/packages/next/src/server/lib/trace/request-insights-identity.ts +++ b/packages/next/src/server/lib/trace/request-insights-identity.ts @@ -1,6 +1,6 @@ import type { AsyncLocalStorage } from 'async_hooks' import type { RequestInsightKind } from '../../../next-devtools/shared/request-insights' -import { createAsyncLocalStorage } from '../../app-render/async-local-storage' +import { getOrCreateGlobalAsyncLocalStorage } from '../../app-render/async-local-storage' export type RequestInsightsIdentity = { requestId: string @@ -12,17 +12,8 @@ export type RequestInsightsIdentity = { // This storage covers the part of BaseServer request handling that runs before // App Render creates workAsyncStorage. Once available, workStore remains the // primary identity source for locally recorded spans. -const REQUEST_INSIGHTS_IDENTITY_STORAGE_KEY = Symbol.for( - '@next/request-insights-identity-storage' -) - function getRequestInsightsIdentityStorage(): AsyncLocalStorage { - const globalStore = globalThis as typeof globalThis & { - [REQUEST_INSIGHTS_IDENTITY_STORAGE_KEY]?: AsyncLocalStorage - } - - return (globalStore[REQUEST_INSIGHTS_IDENTITY_STORAGE_KEY] ??= - createAsyncLocalStorage()) + return getOrCreateGlobalAsyncLocalStorage('request-insights-identity-storage') } export function runWithRequestInsightsIdentity( diff --git a/test/development/app-dir/cache-components-dev-warmup/dev-warmup.util.ts b/test/development/app-dir/cache-components-dev-warmup/dev-warmup.util.ts index 338b8bbf9d8b..fb91224ae6d3 100644 --- a/test/development/app-dir/cache-components-dev-warmup/dev-warmup.util.ts +++ b/test/development/app-dir/cache-components-dev-warmup/dev-warmup.util.ts @@ -25,7 +25,7 @@ export function runDevWarmupTests({ : 'fixtures/without-prefetch-config' describe(`cache-components-dev-warmup - ${description}`, () => { - const { next, isTurbopack } = nextTestSetup({ + const { next } = nextTestSetup({ files: nodePath.join(__dirname, fixturePath), }) @@ -102,19 +102,6 @@ export function runDevWarmupTests({ 'AbortError: This operation was aborted' ) - if (isTurbopack) { - // FIXME: - // In Turbopack, requests to the /revalidate route seem to occasionally crash - // due to some HMR or compilation issue. `revalidatePath` throws this error: - // - // Invariant: static generation store missing in revalidatePath - // - // This is unrelated to the logic being tested here, so for now, we skip the assertions - // that require us to revalidate. - console.log('WARNING: skipping revalidation assertions in turbopack') - return - } - // After a revalidation the subsequent render must discard the stale cache // entries. This should not affect the environment labels once the caches // are warm again. @@ -171,19 +158,6 @@ export function runDevWarmupTests({ 'AbortError: This operation was aborted' ) - if (isTurbopack) { - // FIXME: - // In Turbopack, requests to the /revalidate route seem to occasionally crash - // due to some HMR or compilation issue. `revalidatePath` throws this error: - // - // Invariant: static generation store missing in revalidatePath - // - // This is unrelated to the logic being tested here, so for now, we skip the assertions - // that require us to revalidate. - console.log('WARNING: skipping revalidation assertions in turbopack') - return - } - // After a revalidation the subsequent render must discard the stale cache // entries. This should not affect the environment labels once the caches // are warm again. @@ -443,58 +417,52 @@ export function runDevWarmupTests({ }) }) - // FIXME: it seems like in Turbopack we sometimes get two instances of `workUnitAsyncStorage` -- - // `app-render` gets a second, newer instance, different from `io()`. - // Thus, `io()` gets an undefined `workUnitStore` and does nothing, so sync IO does not get tracked at all. - // This is likely caused by the same bug that breaks `/revalidate` (see other FIXME above), - // where a route crashes due to a missing `workStore`. - if (!isTurbopack) { - it('sync IO in the static phase', async () => { - const path = '/sync-io/static' + it('sync IO in the static phase', async () => { + const path = '/sync-io/static' - const assertLogs = async (browser: Playwright) => { - const logs = await browser.log() - - assertLog(logs, 'after first cache', 'Prerender') - // sync IO in the static stage errors and advances to Server. - assertLog(logs, 'after sync io', 'Server') - assertLog(logs, 'after cache read - page', 'Server') - } + const assertLogs = async (browser: Playwright) => { + const logs = await browser.log() - if (isInitialLoad) { - await testInitialLoad(path, assertLogs) - } else { - await testNavigation(path, assertLogs) - } - }) + assertLog(logs, 'after first cache', 'Prerender') + // sync IO in the static stage errors and advances to Server. + assertLog(logs, 'after sync io', 'Server') + assertLog(logs, 'after cache read - page', 'Server') + } - it('sync IO in the runtime phase', async () => { - const path = '/sync-io/runtime' + if (isInitialLoad) { + await testInitialLoad(path, assertLogs) + } else { + await testNavigation(path, assertLogs) + } + }) - const assertLogs = async (browser: Playwright) => { - const logs = await browser.log() + it('sync IO in the runtime phase', async () => { + const path = '/sync-io/runtime' - assertLog(logs, 'after first cache', 'Prerender') - assertLog(logs, 'after cookies', 'Prefetch') - if (hasRuntimePrefetch || partialPrefetching) { - // in partialPrefetching (via per-segment config or global flag), - // sync IO in the runtime stage errors and advances to Server. - assertLog(logs, 'after sync io', 'Server') - assertLog(logs, 'after cache read - page', 'Server') - } else { - // if runtime prefetching is not on, sync IO in the runtime stage does nothing. - assertLog(logs, 'after sync io', 'Prefetch') - assertLog(logs, 'after cache read - page', 'Prefetch') - } - } + const assertLogs = async (browser: Playwright) => { + const logs = await browser.log() - if (isInitialLoad) { - await testInitialLoad(path, assertLogs) + assertLog(logs, 'after first cache', 'Prerender') + assertLog(logs, 'after cookies', 'Prefetch') + if (hasRuntimePrefetch || partialPrefetching) { + // in partialPrefetching (via per-segment config or global flag), + // sync IO in the runtime stage errors and advances to Server. + assertLog(logs, 'after sync io', 'Server') + assertLog(logs, 'after cache read - page', 'Server') } else { - await testNavigation(path, assertLogs) + // if runtime prefetching is not on, sync IO in the runtime stage + // does nothing. + assertLog(logs, 'after sync io', 'Prefetch') + assertLog(logs, 'after cache read - page', 'Prefetch') } - }) - } + } + + if (isInitialLoad) { + await testInitialLoad(path, assertLogs) + } else { + await testNavigation(path, assertLogs) + } + }) }) }) } diff --git a/test/development/app-dir/cache-components-tasks/cache-components-tasks.test.ts b/test/development/app-dir/cache-components-tasks/cache-components-tasks.test.ts index 9c2415db41a8..25d3010d9b51 100644 --- a/test/development/app-dir/cache-components-tasks/cache-components-tasks.test.ts +++ b/test/development/app-dir/cache-components-tasks/cache-components-tasks.test.ts @@ -16,7 +16,7 @@ describe.each([ ])( 'cache-components-tasks - $description', ({ fixturePath, hasRuntimePrefetch }) => { - const { next, isTurbopack, isNextDev } = nextTestSetup({ + const { next } = nextTestSetup({ files: nodePath.join(__dirname, fixturePath), }) @@ -79,19 +79,6 @@ describe.each([ await retry(() => assertLogs(browser)) assertNoUnexpectedErrorsInCli() - if (isNextDev && isTurbopack) { - // FIXME: - // In Turbopack, requests to the /revalidate route seem to occasionally crash - // due to some HMR or compilation issue. `revalidatePath` throws this error: - // - // Invariant: static generation store missing in revalidatePath - // - // This is unrelated to the logic being tested here, so for now, we skip the assertions - // that require us to revalidate. - console.log('WARNING: skipping revalidation assertions in turbopack') - return - } - // After a revalidation the subsequent warmup render must discard stale // cache entries. // This should not affect the environment labels. @@ -119,19 +106,6 @@ describe.each([ await retry(() => assertLogs(browser)) assertNoUnexpectedErrorsInCli() - if (isNextDev && isTurbopack) { - // FIXME: - // In Turbopack, requests to the /revalidate route seem to occasionally crash - // due to some HMR or compilation issue. `revalidatePath` throws this error: - // - // Invariant: static generation store missing in revalidatePath - // - // This is unrelated to the logic being tested here, so for now, we skip the assertions - // that require us to revalidate. - console.log('WARNING: skipping revalidation assertions in turbopack') - return - } - // After a revalidation the subsequent warmup render must discard stale // cache entries. // This should not affect the environment labels. From d45672c0bc0684d2ab0c6cb3a484dd222cd169ff Mon Sep 17 00:00:00 2001 From: "next-js-bot[bot]" <279046576+next-js-bot[bot]@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:21:52 +0000 Subject: [PATCH 2/3] v16.3.1-canary.21 --- lerna.json | 2 +- packages/create-next-app/package.json | 2 +- packages/devlow-bench/package.json | 2 +- packages/eslint-config-next/package.json | 4 ++-- packages/eslint-plugin-internal/package.json | 2 +- packages/eslint-plugin-next/package.json | 2 +- packages/font/package.json | 2 +- packages/next-bundle-analyzer/package.json | 2 +- packages/next-codemod/package.json | 2 +- packages/next-env/package.json | 2 +- packages/next-mdx/package.json | 2 +- packages/next-playwright/package.json | 2 +- packages/next-plugin-storybook/package.json | 2 +- packages/next-polyfill-module/package.json | 2 +- packages/next-polyfill-nomodule/package.json | 2 +- packages/next-routing/package.json | 2 +- packages/next-rspack/package.json | 2 +- packages/next-swc/package.json | 2 +- packages/next/package.json | 14 +++++++------- packages/react-refresh-utils/package.json | 2 +- packages/third-parties/package.json | 4 ++-- pnpm-lock.yaml | 16 ++++++++-------- 22 files changed, 37 insertions(+), 37 deletions(-) diff --git a/lerna.json b/lerna.json index cf21c39263fc..df580516daaf 100644 --- a/lerna.json +++ b/lerna.json @@ -15,5 +15,5 @@ "registry": "https://registry.npmjs.org/" } }, - "version": "16.3.1-canary.20" + "version": "16.3.1-canary.21" } \ No newline at end of file diff --git a/packages/create-next-app/package.json b/packages/create-next-app/package.json index 3a43db49f65f..9a7b8c3bcf95 100644 --- a/packages/create-next-app/package.json +++ b/packages/create-next-app/package.json @@ -1,6 +1,6 @@ { "name": "create-next-app", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "keywords": [ "react", "next", diff --git a/packages/devlow-bench/package.json b/packages/devlow-bench/package.json index 49b768cf52ec..908624afab97 100644 --- a/packages/devlow-bench/package.json +++ b/packages/devlow-bench/package.json @@ -1,7 +1,7 @@ { "name": "@vercel/devlow-bench", "private": true, - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "Benchmarking tool for the developer workflow", "repository": { "type": "git", diff --git a/packages/eslint-config-next/package.json b/packages/eslint-config-next/package.json index a96f9ab1e129..a2dc24447148 100644 --- a/packages/eslint-config-next/package.json +++ b/packages/eslint-config-next/package.json @@ -1,6 +1,6 @@ { "name": "eslint-config-next", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "ESLint configuration used by Next.js.", "license": "MIT", "repository": { @@ -12,7 +12,7 @@ "dist" ], "dependencies": { - "@next/eslint-plugin-next": "16.3.1-canary.20", + "@next/eslint-plugin-next": "16.3.1-canary.21", "eslint-import-resolver-node": "^0.3.6", "eslint-import-resolver-typescript": "^3.5.2", "eslint-plugin-import": "^2.32.0", diff --git a/packages/eslint-plugin-internal/package.json b/packages/eslint-plugin-internal/package.json index 0c49b385f8c2..d95189976da3 100644 --- a/packages/eslint-plugin-internal/package.json +++ b/packages/eslint-plugin-internal/package.json @@ -1,7 +1,7 @@ { "name": "@next/eslint-plugin-internal", "private": true, - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "ESLint plugin for working on Next.js.", "exports": { ".": "./src/eslint-plugin-internal.js" diff --git a/packages/eslint-plugin-next/package.json b/packages/eslint-plugin-next/package.json index c4a31224c223..7ce45055e463 100644 --- a/packages/eslint-plugin-next/package.json +++ b/packages/eslint-plugin-next/package.json @@ -1,6 +1,6 @@ { "name": "@next/eslint-plugin-next", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "ESLint plugin for Next.js.", "main": "dist/index.js", "types": "dist/index.d.ts", diff --git a/packages/font/package.json b/packages/font/package.json index 958fcd25d630..9e36c766581c 100644 --- a/packages/font/package.json +++ b/packages/font/package.json @@ -1,7 +1,7 @@ { "name": "@next/font", "private": true, - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "repository": { "url": "vercel/next.js", "directory": "packages/font" diff --git a/packages/next-bundle-analyzer/package.json b/packages/next-bundle-analyzer/package.json index 2e2f74df5d19..647e861080c6 100644 --- a/packages/next-bundle-analyzer/package.json +++ b/packages/next-bundle-analyzer/package.json @@ -1,6 +1,6 @@ { "name": "@next/bundle-analyzer", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "main": "index.js", "types": "index.d.ts", "license": "MIT", diff --git a/packages/next-codemod/package.json b/packages/next-codemod/package.json index c94381b66b24..67fcaf119d66 100644 --- a/packages/next-codemod/package.json +++ b/packages/next-codemod/package.json @@ -1,6 +1,6 @@ { "name": "@next/codemod", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "license": "MIT", "repository": { "type": "git", diff --git a/packages/next-env/package.json b/packages/next-env/package.json index 92a8e63573cd..0256f24f7705 100644 --- a/packages/next-env/package.json +++ b/packages/next-env/package.json @@ -1,6 +1,6 @@ { "name": "@next/env", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "keywords": [ "react", "next", diff --git a/packages/next-mdx/package.json b/packages/next-mdx/package.json index b312d905edca..78d6ef8ffd4d 100644 --- a/packages/next-mdx/package.json +++ b/packages/next-mdx/package.json @@ -1,6 +1,6 @@ { "name": "@next/mdx", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "main": "index.js", "license": "MIT", "repository": { diff --git a/packages/next-playwright/package.json b/packages/next-playwright/package.json index 25efa7322b87..a2c21a496fd3 100644 --- a/packages/next-playwright/package.json +++ b/packages/next-playwright/package.json @@ -1,6 +1,6 @@ { "name": "@next/playwright", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "repository": { "url": "vercel/next.js", "directory": "packages/next-playwright" diff --git a/packages/next-plugin-storybook/package.json b/packages/next-plugin-storybook/package.json index 8c676c8da684..3341d527b8b2 100644 --- a/packages/next-plugin-storybook/package.json +++ b/packages/next-plugin-storybook/package.json @@ -1,6 +1,6 @@ { "name": "@next/plugin-storybook", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "repository": { "url": "vercel/next.js", "directory": "packages/next-plugin-storybook" diff --git a/packages/next-polyfill-module/package.json b/packages/next-polyfill-module/package.json index e8472b39a9d3..a955c79cf004 100644 --- a/packages/next-polyfill-module/package.json +++ b/packages/next-polyfill-module/package.json @@ -1,6 +1,6 @@ { "name": "@next/polyfill-module", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "A standard library polyfill for ES Modules supporting browsers (Edge 16+, Firefox 60+, Chrome 61+, Safari 10.1+)", "main": "dist/polyfill-module.js", "license": "MIT", diff --git a/packages/next-polyfill-nomodule/package.json b/packages/next-polyfill-nomodule/package.json index 88def75eccc7..9cfe87cd75d1 100644 --- a/packages/next-polyfill-nomodule/package.json +++ b/packages/next-polyfill-nomodule/package.json @@ -1,6 +1,6 @@ { "name": "@next/polyfill-nomodule", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "A polyfill for non-dead, nomodule browsers.", "main": "dist/polyfill-nomodule.js", "license": "MIT", diff --git a/packages/next-routing/package.json b/packages/next-routing/package.json index 0288a679a1b0..2aed89179ffa 100644 --- a/packages/next-routing/package.json +++ b/packages/next-routing/package.json @@ -1,6 +1,6 @@ { "name": "@next/routing", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "keywords": [ "react", "next", diff --git a/packages/next-rspack/package.json b/packages/next-rspack/package.json index c926d17650d0..b4867f29c123 100644 --- a/packages/next-rspack/package.json +++ b/packages/next-rspack/package.json @@ -1,6 +1,6 @@ { "name": "next-rspack", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "repository": { "url": "vercel/next.js", "directory": "packages/next-rspack" diff --git a/packages/next-swc/package.json b/packages/next-swc/package.json index 147318c06049..db2cd0a99a23 100644 --- a/packages/next-swc/package.json +++ b/packages/next-swc/package.json @@ -1,6 +1,6 @@ { "name": "@next/swc", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "private": true, "files": [ "native/" diff --git a/packages/next/package.json b/packages/next/package.json index 4d076a378dd6..82d3ff650bda 100644 --- a/packages/next/package.json +++ b/packages/next/package.json @@ -1,6 +1,6 @@ { "name": "next", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "The React Framework", "main": "./dist/server/next.js", "license": "MIT", @@ -100,7 +100,7 @@ ] }, "dependencies": { - "@next/env": "16.3.1-canary.20", + "@next/env": "16.3.1-canary.21", "@swc/helpers": "0.5.23", "baseline-browser-mapping": "^2.9.19", "caniuse-lite": "^1.0.30001579", @@ -164,11 +164,11 @@ "@modelcontextprotocol/sdk": "1.18.1", "@mswjs/interceptors": "0.42.0", "@napi-rs/triples": "1.2.0", - "@next/font": "16.3.1-canary.20", - "@next/polyfill-module": "16.3.1-canary.20", - "@next/polyfill-nomodule": "16.3.1-canary.20", - "@next/react-refresh-utils": "16.3.1-canary.20", - "@next/swc": "16.3.1-canary.20", + "@next/font": "16.3.1-canary.21", + "@next/polyfill-module": "16.3.1-canary.21", + "@next/polyfill-nomodule": "16.3.1-canary.21", + "@next/react-refresh-utils": "16.3.1-canary.21", + "@next/swc": "16.3.1-canary.21", "@opentelemetry/api": "1.6.0", "@playwright/test": "1.61.0", "@rspack/core": "1.6.7", diff --git a/packages/react-refresh-utils/package.json b/packages/react-refresh-utils/package.json index 1e2017c6c945..679dd4614698 100644 --- a/packages/react-refresh-utils/package.json +++ b/packages/react-refresh-utils/package.json @@ -1,6 +1,6 @@ { "name": "@next/react-refresh-utils", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "description": "An experimental package providing utilities for React Refresh.", "repository": { "url": "vercel/next.js", diff --git a/packages/third-parties/package.json b/packages/third-parties/package.json index 97efa240ca6f..eb07f494448b 100644 --- a/packages/third-parties/package.json +++ b/packages/third-parties/package.json @@ -1,6 +1,6 @@ { "name": "@next/third-parties", - "version": "16.3.1-canary.20", + "version": "16.3.1-canary.21", "repository": { "url": "vercel/next.js", "directory": "packages/third-parties" @@ -26,7 +26,7 @@ "third-party-capital": "1.0.20" }, "devDependencies": { - "next": "16.3.1-canary.20", + "next": "16.3.1-canary.21", "outdent": "0.8.0", "prettier": "2.5.1", "typescript": "6.0.2" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f6bb46ede3e4..80d598182025 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1024,7 +1024,7 @@ importers: packages/eslint-config-next: dependencies: '@next/eslint-plugin-next': - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../eslint-plugin-next eslint: specifier: '>=9.0.0' @@ -1107,7 +1107,7 @@ importers: packages/next: dependencies: '@next/env': - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../next-env '@swc/helpers': specifier: 0.5.23 @@ -1228,19 +1228,19 @@ importers: specifier: 1.2.0 version: 1.2.0 '@next/font': - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../font '@next/polyfill-module': - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../next-polyfill-module '@next/polyfill-nomodule': - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../next-polyfill-nomodule '@next/react-refresh-utils': - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../react-refresh-utils '@next/swc': - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../next-swc '@opentelemetry/api': specifier: 1.6.0 @@ -1983,7 +1983,7 @@ importers: version: 1.0.20 devDependencies: next: - specifier: 16.3.1-canary.20 + specifier: 16.3.1-canary.21 version: link:../next outdent: specifier: 0.8.0 From 863a0adaa2fd4857cea117b7aa597e36e80b97a0 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sun, 16 Aug 2026 17:28:16 -0700 Subject: [PATCH 3/3] turbo-persistence: add key-value tombstones for MultiValue families (#96929) Add a new `tombstone` format to the persistence layer so we can delete key-value pairs out of MultiValued tables This is in service of the upcoming GC support, but also fills a basic API gap in the db. To delete a key-value-pair you need to call `value_delete` and currently the values are limited to only those that are able to be stored inline in key blocks. This is a non-trivial limitation but it fits our current usecase, and makes the compaction/query algorithms a bit simpler (don't need to 'resolve' values) One non-trivial complexity was maintaining the 'FixedLayout' block optimization for a mix of tombstones and values, so now we support a slightly different mode where all keys have the same length but possibly different types. Finally, this branch solves a problem with deleting tombstones. Tombstones 'shadow' older values and allow us to drop them during compaction. With GC getting ready to start writing tombstones the risk becomes 'when can we delete a tombstone! This is solved probabilistically using the amqf filters, during compaction we drop tombstones if they could not possibly match anything in an older SST. Without this, tombstones in the TaskCache table would fill up over time. --- Cargo.lock | 1 + turbopack/crates/turbo-persistence/Cargo.toml | 1 + turbopack/crates/turbo-persistence/README.md | 56 +- .../turbo-persistence/src/bin/sst_inspect.rs | 172 ++++-- .../crates/turbo-persistence/src/collector.rs | 45 +- .../turbo-persistence/src/collector_entry.rs | 52 +- turbopack/crates/turbo-persistence/src/db.rs | 113 +++- turbopack/crates/turbo-persistence/src/lib.rs | 3 + .../turbo-persistence/src/lookup_entry.rs | 15 +- .../crates/turbo-persistence/src/meta_file.rs | 19 +- .../src/meta_file_builder.rs | 7 +- .../crates/turbo-persistence/src/rc_bytes.rs | 7 + .../src/static_sorted_file.rs | 144 +++-- .../src/static_sorted_file_builder.rs | 259 ++++++-- .../crates/turbo-persistence/src/tests.rs | 562 +++++++++++++++++- .../turbo-persistence/src/write_batch.rs | 63 +- 16 files changed, 1350 insertions(+), 169 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9e30ce91dedc..6e069ac13edb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10001,6 +10001,7 @@ name = "turbo-persistence" version = "0.1.0" dependencies = [ "anyhow", + "auto-hash-map", "bitfield", "byteorder", "codspeed-criterion-compat", diff --git a/turbopack/crates/turbo-persistence/Cargo.toml b/turbopack/crates/turbo-persistence/Cargo.toml index b70c73ccf1fb..ea5d8475a679 100644 --- a/turbopack/crates/turbo-persistence/Cargo.toml +++ b/turbopack/crates/turbo-persistence/Cargo.toml @@ -14,6 +14,7 @@ verbose_log = [] [dependencies] anyhow = { workspace = true } +auto-hash-map = { workspace = true } bitfield = { workspace = true } byteorder = { workspace = true } crc32fast = { workspace = true } diff --git a/turbopack/crates/turbo-persistence/README.md b/turbopack/crates/turbo-persistence/README.md index 7934eac48162..8b979bf2dbf4 100644 --- a/turbopack/crates/turbo-persistence/README.md +++ b/turbopack/crates/turbo-persistence/README.md @@ -29,7 +29,10 @@ Therefore there are these value types: - SMALL: Values 9–4096 bytes packed into shared value blocks within `*.sst` files. - MEDIUM: Values 4097 bytes – 64 MB stored in dedicated value blocks within `*.sst` files. - BLOB: Values > 64 MB stored in separate `*.blob` files. -- DELETED: Values that are deleted. (Tombstone) +- KEY DELETED: Every value for the key is deleted. (Key tombstone) +- KEY-VALUE DELETED: Only one named key → value pair is deleted, leaving other values for the same + key intact. (Key-value tombstone) Only meaningful for `MultiValue` families; see + [Key-value tombstones](#key-value-tombstones). - Future: - MERGE: An application specific update operation that is applied on the old value. @@ -138,7 +141,7 @@ Depending on the `type` field entry has a different format: - 8 bytes key hash (if block type 1) - key data - 4 bytes sequence number -- 2: deleted key / tombstone (no data) +- 2: deleted key / key tombstone (no data) - 8 bytes key hash (if block type 1) - key data - 3: normal key (medium sized value) @@ -150,25 +153,62 @@ Depending on the `type` field entry has a different format: - 2 byte block index - 3 bytes size - 4 bytes position in block -- 8..255: inlined value (currently only values ≤8 bytes are inlined, though the format supports up to 247) +- 8..=16: inlined value, size = type - 8 (the format supports up to 247, but `MAX_INLINE_VALUE_SIZE` + currently caps it at 8) - 8 bytes key hash (if block type 1) - key data - (type - 8) bytes value data (inline, no separate value block) +- 17..=25: key-value tombstone, deleted value size = type - 17 (mirrors the inline range and shifts + with `MAX_INLINE_VALUE_SIZE`) + - 8 bytes key hash (if block type 1) + - key data + - (type - 17) bytes of the deleted value, stored inline + +Both ranged kinds are open-ended, so a decoder must test the key-value tombstone range **before** +the inline range. The entries are sorted by key hash and key. +##### Key-value tombstones + +A key-value tombstone names the exact pair to remove, so it must carry a copy of the deleted +value's bytes. Since the value lives inline in the key block and its length is encoded in the type +byte, only inline-sized values can be deleted this way — hence `MAX_INLINE_VALUE_SIZE` bounds +`delete_value`. + +The size limit is a consequence of that encoding, not of the comparison logic: matching is a plain +byte comparison and does not care how a value is stored. Supporting larger deleted values is +therefore possible but unmotivated — the tombstone stores a second copy of the value, so the cost +of deleting approaches the cost of the value itself, and reclaiming space is the whole point. + +If it is ever needed, the natural encoding is a dedicated is-tombstone bit (e.g. the top bit) on the +entry type, making "deleted" orthogonal to storage class rather than a parallel type range. That +would also collapse the current duplication where the tombstone representation mirrors the inline +one at every layer. Note that blob-backed values need a separate design: comparing against a blob +means reading it, which would put unbounded I/O in the compaction path, and blob liveness +accounting would have to handle a tombstone holding a blob reference. + #### Key Block (fixed-size) -Used when all entries in a block have the same key size and value type. Eliminates the per-entry offset table, enabling direct arithmetic indexing during binary search. +Used when all entries in a block have the same key size, and either the same value type or at least +the same value size. Eliminates the per-entry offset table, enabling direct arithmetic indexing +during binary search. - 1 byte block type (3: fixed-size with hash, 4: fixed-size without hash) - 3 bytes entry count - 1 byte key size (uniform across all entries) -- 1 byte value type (shared by all entries, same encoding as variable-size type field) +- 1 byte value type (shared by all entries, same encoding as variable-size type field), or + `FIXED_KEY_BLOCK_MIXED_VALUE_TYPE` (4) when entries share a value size but not a value type +- 1 byte value size — only present when the value type is `FIXED_KEY_BLOCK_MIXED_VALUE_TYPE` - foreach entry (packed at stride = hash_len + key_size + val_size): - 8 bytes key hash (if block type 3) - key data (key_size bytes) - - value data (size determined by value type) + - 1 byte value type — only present when the block is mixed-type + - value data (size determined by the block's or the entry's value type) + +The mixed-type form exists so that same-sized inline values and key-value tombstones can share a +fixed-size block: they have equal value sizes but different type bytes. Tag 4 is available as the +mixed marker because it is not itself a valid entry type. Entry position for index `i` is computed as `header_size + i * stride` with no indirection. The writer automatically selects fixed-size format when all entries in a block qualify; otherwise falls back to the variable-size format above. @@ -321,3 +361,7 @@ Configuration options for compactions are: - fsync! - (this also deleted enqueued files) + +## Compatibility + +Currently, the database does not support cross version compatibility. Therefore all updates should be considered breaking changes and the only approach is to rewrite the databases. diff --git a/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs b/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs index f359cb743528..b7f4c5708bac 100644 --- a/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs +++ b/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs @@ -3,12 +3,11 @@ //! This tool inspects SST files to report entry type statistics per family, //! useful for verifying that inline value optimization is being used. //! -//! Entry types: -//! - 0: Small value (stored in value block) -//! - 1: Blob reference -//! - 2: Deleted/tombstone -//! - 3: Medium value -//! - 8-255: Inline value where (type - 8) = value byte count +//! Entry types are the `KEY_BLOCK_ENTRY_TYPE_*` constants in +//! [`turbo_persistence::static_sorted_file`]; the `--help` output lists them with their current +//! values. The two ranged kinds encode a size in the type byte: an inline value's byte count is +//! `type - KEY_BLOCK_ENTRY_TYPE_INLINE_MIN`, and a key-value tombstone's deleted byte count is +//! `type - KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN`. use std::{ collections::{BTreeMap, HashSet}, @@ -21,15 +20,17 @@ use fs_err::{self as fs, File}; use lzzzz::lz4::decompress; use memmap2::Mmap; use turbo_persistence::{ - BLOCK_HEADER_SIZE, checksum_block, + BLOCK_HEADER_SIZE, MAX_INLINE_VALUE_SIZE, checksum_block, meta_file::MetaFile, mmap_helper::advise_mmap_for_persistence, read_current_version, sst_filter::SstFilter, static_sorted_file::{ BLOCK_TYPE_FIXED_KEY_NO_HASH, BLOCK_TYPE_FIXED_KEY_WITH_HASH, BLOCK_TYPE_KEY_NO_HASH, - BLOCK_TYPE_KEY_WITH_HASH, KEY_BLOCK_ENTRY_TYPE_BLOB, KEY_BLOCK_ENTRY_TYPE_DELETED, - KEY_BLOCK_ENTRY_TYPE_INLINE_MIN, KEY_BLOCK_ENTRY_TYPE_MEDIUM, KEY_BLOCK_ENTRY_TYPE_SMALL, + BLOCK_TYPE_KEY_WITH_HASH, FIXED_KEY_BLOCK_MIXED_VALUE_TYPE, KEY_BLOCK_ENTRY_TYPE_BLOB, + KEY_BLOCK_ENTRY_TYPE_INLINE_MIN, KEY_BLOCK_ENTRY_TYPE_KEY_DELETED, + KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN, KEY_BLOCK_ENTRY_TYPE_MEDIUM, + KEY_BLOCK_ENTRY_TYPE_SMALL, }, }; @@ -96,10 +97,11 @@ struct SstStats { /// Value sizes by type (inline values track actual bytes) inline_value_bytes: u64, - small_value_refs: u64, // Count of references to value blocks - medium_value_refs: u64, // Count of references to medium values - blob_refs: u64, // Count of blob references - deleted_count: u64, // Count of deleted entries + small_value_refs: u64, // Count of references to value blocks + medium_value_refs: u64, // Count of references to medium values + blob_refs: u64, // Count of blob references + key_deleted_count: u64, // Count of key tombstones + key_value_deleted_count: u64, // Count of key-value tombstones /// File size in bytes file_size: u64, @@ -121,7 +123,8 @@ impl SstStats { self.small_value_refs += other.small_value_refs; self.medium_value_refs += other.medium_value_refs; self.blob_refs += other.blob_refs; - self.deleted_count += other.deleted_count; + self.key_deleted_count += other.key_deleted_count; + self.key_value_deleted_count += other.key_value_deleted_count; self.file_size += other.file_size; } } @@ -144,12 +147,16 @@ fn track_entry_type(stats: &mut SstStats, entry_type: u8) { KEY_BLOCK_ENTRY_TYPE_BLOB => { stats.blob_refs += 1; } - KEY_BLOCK_ENTRY_TYPE_DELETED => { - stats.deleted_count += 1; + KEY_BLOCK_ENTRY_TYPE_KEY_DELETED => { + stats.key_deleted_count += 1; } KEY_BLOCK_ENTRY_TYPE_MEDIUM => { stats.medium_value_refs += 1; } + // Must precede the inline arm: both are open-ended and the tombstone range sits above it. + ty if ty >= KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN => { + stats.key_value_deleted_count += 1; + } ty if ty >= KEY_BLOCK_ENTRY_TYPE_INLINE_MIN => { let inline_size = (ty - KEY_BLOCK_ENTRY_TYPE_INLINE_MIN) as u64; stats.inline_value_bytes += inline_size; @@ -162,8 +169,13 @@ fn entry_type_description(ty: u8) -> String { match ty { KEY_BLOCK_ENTRY_TYPE_SMALL => "small value (in value block)".to_string(), KEY_BLOCK_ENTRY_TYPE_BLOB => "blob reference".to_string(), - KEY_BLOCK_ENTRY_TYPE_DELETED => "deleted/tombstone".to_string(), + KEY_BLOCK_ENTRY_TYPE_KEY_DELETED => "key tombstone".to_string(), KEY_BLOCK_ENTRY_TYPE_MEDIUM => "medium value".to_string(), + // Must precede the inline arm: both are open-ended and the tombstone range sits above it. + ty if ty >= KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN => { + let size = ty - KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN; + format!("key-value tombstone ({size} byte value)") + } ty if ty >= KEY_BLOCK_ENTRY_TYPE_INLINE_MIN => { let inline_size = ty - KEY_BLOCK_ENTRY_TYPE_INLINE_MIN; format!("inline {} bytes", inline_size) @@ -368,9 +380,23 @@ fn parse_key_block_indices(index_block: &[u8]) -> HashSet { } /// Parsed header of a key block. +#[derive(Clone, Copy)] enum KeyBlockHeader { - Variable { entry_count: u32 }, - Fixed { entry_count: u32, value_type: u8 }, + Variable { + entry_count: u32, + }, + Fixed { + entry_count: u32, + value_type: u8, + }, + /// Fixed-size layout whose entries share a value size but not a value type, so each carries + /// its own type byte between its key and its value. + FixedMixedType { + entry_count: u32, + hash_len: usize, + key_size: usize, + stride: usize, + }, } /// Parses the header of a key block from the full decompressed block data. @@ -384,10 +410,28 @@ fn parse_key_block_header(block: &[u8]) -> Result { } BLOCK_TYPE_FIXED_KEY_WITH_HASH | BLOCK_TYPE_FIXED_KEY_NO_HASH => { assert!(block.len() >= 6, "Fixed key block header too small"); - Ok(KeyBlockHeader::Fixed { - entry_count, - value_type: block[5], - }) + if block[5] == FIXED_KEY_BLOCK_MIXED_VALUE_TYPE { + assert!(block.len() >= 7, "Mixed-type key block header too small"); + let hash_len = if block_type == BLOCK_TYPE_FIXED_KEY_WITH_HASH { + 8 + } else { + 0 + }; + let key_size = block[4] as usize; + let val_size = block[6] as usize; + Ok(KeyBlockHeader::FixedMixedType { + entry_count, + hash_len, + key_size, + // +1 for the per-entry type byte. + stride: hash_len + key_size + val_size + 1, + }) + } else { + Ok(KeyBlockHeader::Fixed { + entry_count, + value_type: block[5], + }) + } } _ => bail!("Invalid key block type: {block_type}"), } @@ -395,27 +439,32 @@ fn parse_key_block_header(block: &[u8]) -> Result { /// Iterates over entry type bytes in a key block. /// -/// For variable-size key blocks, reads byte 0 of each 4-byte offset table entry. -/// For fixed-size key blocks, yields the single `value_type` repeated `entry_count` times. +/// For variable-size key blocks, reads byte 0 of each 4-byte offset table entry. For fixed-size +/// key blocks, yields the single `value_type` repeated `entry_count` times, or reads the per-entry +/// type byte when the block has mixed types. fn iter_key_block_entry_types( header: KeyBlockHeader, block: &[u8], ) -> impl Iterator + '_ { - let (entry_count, fixed_type) = match header { - KeyBlockHeader::Variable { entry_count } => (entry_count, None), - KeyBlockHeader::Fixed { - entry_count, - value_type, - } => (entry_count, Some(value_type)), + let entry_count = match header { + KeyBlockHeader::Variable { entry_count } + | KeyBlockHeader::Fixed { entry_count, .. } + | KeyBlockHeader::FixedMixedType { entry_count, .. } => entry_count, }; - (0..entry_count).map(move |i| { - if let Some(vt) = fixed_type { - vt - } else { - // Variable block: offset table starts at byte 4 (after 1B type + 3B count), - // each entry is 4 bytes, first byte is the entry type. - let header_offset = KEY_BLOCK_HEADER_SIZE + i as usize * 4; - block[header_offset] + (0..entry_count).map(move |i| match header { + // Variable block: offset table starts at byte 4 (after 1B type + 3B count), + // each entry is 4 bytes, first byte is the entry type. + KeyBlockHeader::Variable { .. } => block[KEY_BLOCK_HEADER_SIZE + i as usize * 4], + KeyBlockHeader::Fixed { value_type, .. } => value_type, + KeyBlockHeader::FixedMixedType { + hash_len, + key_size, + stride, + .. + } => { + // Entry data starts after the 7-byte mixed-type header; the type byte sits between + // the entry's key and its value. + block[7 + i as usize * stride + hash_len + key_size] } }) } @@ -502,7 +551,7 @@ fn analyze_sst_file(db_path: &Path, info: &SstInfo) -> Result { raw.was_compressed, ); } - KeyBlockHeader::Fixed { .. } => { + KeyBlockHeader::Fixed { .. } | KeyBlockHeader::FixedMixedType { .. } => { stats.fixed_key_blocks.add( raw.compressed_size, raw.actual_size, @@ -645,11 +694,18 @@ fn print_value_storage(stats: &SstStats, prefix: &str) { format_number(stats.blob_refs) ); } - if stats.deleted_count > 0 { + if stats.key_deleted_count > 0 { + println!( + "{} Key tombstones: {} entries", + prefix, + format_number(stats.key_deleted_count) + ); + } + if stats.key_value_deleted_count > 0 { println!( - "{} Deleted: {} entries", + "{} Key-value tombstones: {} entries", prefix, - format_number(stats.deleted_count) + format_number(stats.key_value_deleted_count) ); } } @@ -830,14 +886,32 @@ fn main() -> Result<()> { eprintln!(" -v, --verbose Show per-SST file details (default: family totals only)"); eprintln!(); eprintln!("Entry types:"); - eprintln!(" 0: Small value (stored in separate value block)"); - eprintln!(" 1: Blob reference"); - eprintln!(" 2: Deleted/tombstone"); - eprintln!(" 3: Medium value"); - eprintln!(" 8+: Inline value (size = type - 8)"); + eprintln!( + " {KEY_BLOCK_ENTRY_TYPE_SMALL}: Small value (stored in separate value block)" + ); + eprintln!(" {KEY_BLOCK_ENTRY_TYPE_BLOB}: Blob reference"); + eprintln!( + " {KEY_BLOCK_ENTRY_TYPE_KEY_DELETED}: Key tombstone (deletes all values for the \ + key)" + ); + eprintln!(" {KEY_BLOCK_ENTRY_TYPE_MEDIUM}: Medium value"); + eprintln!( + " {KEY_BLOCK_ENTRY_TYPE_INLINE_MIN}-{}: Inline value (size = type - \ + {KEY_BLOCK_ENTRY_TYPE_INLINE_MIN})", + KEY_BLOCK_ENTRY_TYPE_INLINE_MIN + MAX_INLINE_VALUE_SIZE as u8 + ); + eprintln!( + " {KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN}-{}: Key-value tombstone (deleted \ + value size = type - {KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN})", + KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN + MAX_INLINE_VALUE_SIZE as u8 + ); eprintln!(); eprintln!("For TaskCache (family 3), values are 4-byte TaskIds."); - eprintln!("Expected entry type is 12 (8 + 4) for inline optimization."); + eprintln!( + "Expected entry type is {} ({KEY_BLOCK_ENTRY_TYPE_INLINE_MIN} + 4) for inline \ + optimization.", + KEY_BLOCK_ENTRY_TYPE_INLINE_MIN + 4 + ); std::process::exit(1); } }; diff --git a/turbopack/crates/turbo-persistence/src/collector.rs b/turbopack/crates/turbo-persistence/src/collector.rs index 80c144494bba..7ae553e298f7 100644 --- a/turbopack/crates/turbo-persistence/src/collector.rs +++ b/turbopack/crates/turbo-persistence/src/collector.rs @@ -4,7 +4,8 @@ use crate::{ FamilyKind, ValueBuffer, collector_entry::{CollectorEntry, CollectorEntryValue, EntryKey, TINY_VALUE_THRESHOLD}, constants::{ - DATA_THRESHOLD_PER_INITIAL_FILE, MAX_ENTRIES_PER_INITIAL_FILE, MAX_SMALL_VALUE_SIZE, + DATA_THRESHOLD_PER_INITIAL_FILE, MAX_ENTRIES_PER_INITIAL_FILE, MAX_INLINE_VALUE_SIZE, + MAX_SMALL_VALUE_SIZE, }, key::{StoreKey, hash_key}, value_block_count_tracker::ValueBlockCountTracker, @@ -86,7 +87,7 @@ impl Collector { }); } - /// Adds a tombstone pair to the collector. + /// Adds a tombstone pair to the collector. This deletes *all* values for `key`. pub fn delete(&mut self, key: K) { let key = EntryKey { hash: hash_key(&key), @@ -95,7 +96,39 @@ impl Collector { self.total_key_size += key.len(); self.entries.push(CollectorEntry { key, - value: CollectorEntryValue::Deleted, + value: CollectorEntryValue::KeyDeleted, + }); + } + + /// Adds a key-value tombstone to the collector: deletes only the single `key` -> `value` pair, + /// leaving any other values for `key` intact. + /// + /// Only meaningful for [`FamilyKind::MultiValue`] families, where a key can map to several + /// values and [`Collector::delete`] is too coarse: it would drop the unrelated values too. + /// The motivating case is the task cache, which is keyed by a hash and so holds more than one + /// value whenever two tasks collide. Removing one task must leave the colliding task's entry + /// readable, which requires naming the exact pair to delete. + /// + /// Deleting a pair written in the same batch is not supported; see + /// [`WriteBatch::delete_value`][crate::WriteBatch]. + /// + /// `value` must be at most [`MAX_INLINE_VALUE_SIZE`] bytes; callers must validate this. Larger + /// values could be supported in the future but there is currently no usecase. + pub fn delete_value(&mut self, key: K, value: &[u8]) { + debug_assert!(value.len() <= MAX_INLINE_VALUE_SIZE); + let key = EntryKey { + hash: hash_key(&key), + data: key, + }; + self.total_key_size += key.len(); + let mut data = [0u8; MAX_INLINE_VALUE_SIZE]; + data[..value.len()].copy_from_slice(value); + self.entries.push(CollectorEntry { + key, + value: CollectorEntryValue::KeyValueDeleted { + value: data, + len: value.len() as u8, + }, }); } @@ -110,19 +143,19 @@ impl Collector { self.entries.push(entry); } - /// Sorts entries by key. Tombstones are placed last within each key group. + /// Sorts entries by key. Within a key group, key-value tombstones are placed first and key + /// tombstones last (see [`CollectorEntryValue::sort_rank`]). /// This method does not deduplicate entries. /// /// In debug builds, asserts that SingleValue families have no duplicate keys. pub fn sorted(&mut self, family_kind: FamilyKind) -> (&[CollectorEntry], usize) { - // Sort by (hash, key) with tombstones placed last within each key group. // We can use unstable sort because the relative order of equal elements // doesn't matter — duplicates are either disallowed (SingleValue) or // allowed without deduplication (MultiValue). self.entries.sort_unstable_by(|a, b| { a.key .cmp(&b.key) - .then_with(|| a.value.is_deleted().cmp(&b.value.is_deleted())) + .then_with(|| a.value.sort_rank().cmp(&b.value.sort_rank())) }); #[cfg(debug_assertions)] diff --git a/turbopack/crates/turbo-persistence/src/collector_entry.rs b/turbopack/crates/turbo-persistence/src/collector_entry.rs index 13e7a2ee26e5..19de3e849bb3 100644 --- a/turbopack/crates/turbo-persistence/src/collector_entry.rs +++ b/turbopack/crates/turbo-persistence/src/collector_entry.rs @@ -32,17 +32,24 @@ pub enum CollectorEntryValue { Large { blob: u32, }, - Deleted, + KeyDeleted, + /// Key-value tombstone: deletes only this one value from the key's group. MultiValue only. + /// The deleted value is stored inline, so it is capped at [`MAX_INLINE_VALUE_SIZE`]. + KeyValueDeleted { + value: [u8; MAX_INLINE_VALUE_SIZE], + len: u8, + }, } impl CollectorEntryValue { pub fn len(&self) -> usize { match self { - CollectorEntryValue::Tiny { len, .. } => *len as usize, + CollectorEntryValue::KeyValueDeleted { len, .. } + | CollectorEntryValue::Tiny { len, .. } => *len as usize, CollectorEntryValue::Small { value } => value.len(), CollectorEntryValue::Medium { value } => value.len(), CollectorEntryValue::Large { blob: _ } => 0, - CollectorEntryValue::Deleted => 0, + CollectorEntryValue::KeyDeleted => 0, } } @@ -51,6 +58,22 @@ impl CollectorEntryValue { matches!(self, CollectorEntryValue::Medium { .. }) } + /// The value bytes, or `None` for variants that carry no value data of their own (blob + /// references and key tombstones). + #[cfg(feature = "verify_sst_content")] + pub fn as_bytes(&self) -> Option<&[u8]> { + match self { + // Separate arms: the inline buffers have different sizes, so they cannot be bound by + // a single or-pattern. + CollectorEntryValue::Tiny { value, len } => Some(&value[..*len as usize]), + CollectorEntryValue::KeyValueDeleted { value, len } => Some(&value[..*len as usize]), + CollectorEntryValue::Small { value } | CollectorEntryValue::Medium { value } => { + Some(value) + } + CollectorEntryValue::Large { .. } | CollectorEntryValue::KeyDeleted => None, + } + } + /// Returns the value size if it will be packed into a small value block, or 0 otherwise. pub fn small_value_size(&self) -> usize { match self { @@ -62,9 +85,21 @@ impl CollectorEntryValue { } } - /// Returns true if this value is a deletion tombstone. - pub fn is_deleted(&self) -> bool { - matches!(self, CollectorEntryValue::Deleted) + /// Sort rank within a key group. The two tombstone kinds sit at opposite ends: + /// + /// - Key-value tombstones (rank 0) go **first**, so a reader collects them before the values + /// they filter and can apply them in one forward pass. + /// - Values (rank 1) go in the middle. + /// - Key tombstones (rank 2) go **last**, because they shadow only entries older than + /// themselves — including entries in this same SST. A batch doing `put(A); delete; put(B)` + /// must keep A and B, so a reader that stops at the first key tombstone it sees still returns + /// the same-batch values it already collected. + pub fn sort_rank(&self) -> u8 { + match self { + CollectorEntryValue::KeyValueDeleted { .. } => 0, + CollectorEntryValue::KeyDeleted => 2, + _ => 1, + } } } @@ -133,7 +168,10 @@ impl Entry for CollectorEntry { } CollectorEntryValue::Medium { value } => EntryValue::Medium { value }, CollectorEntryValue::Large { blob } => EntryValue::Large { blob: *blob }, - CollectorEntryValue::Deleted => EntryValue::Deleted, + CollectorEntryValue::KeyDeleted => EntryValue::KeyDeleted, + CollectorEntryValue::KeyValueDeleted { value, len } => EntryValue::KeyValueDeleted { + value: &value[..*len as usize], + }, } } } diff --git a/turbopack/crates/turbo-persistence/src/db.rs b/turbopack/crates/turbo-persistence/src/db.rs index afd60c8c8a54..20e55ebbe944 100644 --- a/turbopack/crates/turbo-persistence/src/db.rs +++ b/turbopack/crates/turbo-persistence/src/db.rs @@ -2,6 +2,7 @@ use std::{ borrow::Cow, collections::HashSet, fmt::Display, + hash::BuildHasherDefault, io::{BufWriter, ErrorKind, Write}, mem::take, ops::RangeInclusive, @@ -13,6 +14,7 @@ use std::{ }; use anyhow::{Context, Result, bail}; +use auto_hash_map::AutoSet; use byteorder::{BE, ReadBytesExt, WriteBytesExt}; use dashmap::DashSet; use fs_err::{self as fs, File, OpenOptions, ReadDir}; @@ -20,6 +22,7 @@ use jiff::Timestamp; use memmap2::Mmap; use nohash_hasher::BuildNoHashHasher; use parking_lot::{Mutex, RwLock}; +use rustc_hash::FxHasher; use serde::{Deserialize, Serialize}; use smallvec::SmallVec; use tracing::span::EnteredSpan; @@ -1524,6 +1527,7 @@ impl TurboPersistence .parallel_scheduler .parallel_map_collect_owned::<_, _, Result>>(merge_jobs, |indices| { let _span = span.clone().entered(); + if indices.len() == 1 { // If we only have one file, we can just move it let index = indices[0]; @@ -1547,6 +1551,39 @@ impl TurboPersistence }); } + // A tombstone is dead if no older SST contains a matching key. + // Returns `true`` if the tombstone is definitely dead (no false + // positives), if `false` is returned then the tomstone is only likely + // to be alive since the amqf may have false positive match for the + let tombstone_is_dead = { + // Filters of every SST older than this job. + // + // A tombstone only suppresses values older than itself, and within + // the job `MergeIter` yields + // newest-first so the loop below already drops + // those. What remains is everything older than the job's oldest + // member. + let oldest_index_in_job = indices + .iter() + .copied() + .min() + .expect("merge jobs are not empty"); + let older_filters = ssts_with_ranges[..oldest_index_in_job] + .iter() + .map(|sst| { + let entry = + meta_files[sst.meta_index].entry(sst.index_in_meta); + (entry.min_hash(), entry.max_hash(), entry.amqf()) + }) + .collect::>(); + move |hash: u64| { + !older_filters.iter().any(|(min, max, amqf)| { + hash >= *min + && hash <= *max + && amqf.contains_fingerprint(hash) + }) + } + }; // Open SST files independently for compaction. // Uses MADV_SEQUENTIAL for better OS page management // and avoids caching mmaps on MetaEntry's OnceLock. @@ -1675,6 +1712,13 @@ impl TurboPersistence // - MultiValue: skip all older entries after encountering a tombstone // (which signals deletion of all prior values for this key) let mut skip_remaining_for_this_key = false; + // Values deleted by key-value tombstones in the current key group. + // Reset at each key boundary. + let mut deleted_values_for_this_key: AutoSet< + RcBytes, + BuildHasherDefault, + 1, + > = AutoSet::default(); let family_config = &self.config.family_configs[family as usize]; for entry in iter { @@ -1682,8 +1726,27 @@ impl TurboPersistence if current_key.as_ref() != Some(&entry.key) { // we changed keys so undo this flag skip_remaining_for_this_key = false; + deleted_values_for_this_key.clear(); current_key = Some(entry.key.clone()); } + // Key-value tombstones sort first within a group, so each is + // recorded before the values it might delete. + // See: `crate::collector_entry::sort_rank` + if let IterValue::KeyValueDeleted { value } = &entry.value { + deleted_values_for_this_key.insert(value.clone()); + // Applied to this job's values above; keep it only if an SST + // outside the job could still hold a matching key. + if tombstone_is_dead(entry.hash) { + continue; + } + } else if !deleted_values_for_this_key.is_empty() + // Deleted values cannot match blobs, just normal payloads. + && let IterValue::Slice { value } = &entry.value + && deleted_values_for_this_key.contains(value) + { + // Deleted by a key-value tombstone seen earlier in this group. + continue; + } if !skip_remaining_for_this_key { let is_used = used_key_hashes .as_ref() @@ -1696,8 +1759,9 @@ impl TurboPersistence match family_config.kind { FamilyKind::MultiValue => { // For MultiValue families we only skip remaining if we - // see a tombstone - if matches!(entry.value, IterValue::Deleted) { + // see a key tombstone. Key-value tombstones are + // handled above and never reach here. + if matches!(entry.value, IterValue::KeyDeleted) { skip_remaining_for_this_key = true; } } @@ -1707,6 +1771,12 @@ impl TurboPersistence skip_remaining_for_this_key = true; } } + // If this is a tombstone, see if we need to retain it or not. + if matches!(entry.value, IterValue::KeyDeleted) + && tombstone_is_dead(entry.hash) + { + continue; + } collector.add_entry( entry, path, @@ -1944,6 +2014,12 @@ impl TurboPersistence #[cfg(feature = "stats")] let mut found_in_sst = false; + // Values deleted by key-value tombstones seen so far. Because we walk meta files newest + // first, and tombstones sort first within a key group, every tombstone that could apply to + // a value has already been seen by the time we reach that value. + let mut deleted_values: AutoSet, 1> = + AutoSet::default(); + let mut size = 0; for meta in inner.meta_files.iter().rev() { @@ -1973,20 +2049,18 @@ impl TurboPersistence found_in_sst = true; } inner.accessed_key_hashes[family].insert(hash); - // Process values. Tombstones sort last within a key group, - // so when we see a tombstone, we can return immediately. for value in values { match value { - LookupValue::Deleted => { + LookupValue::KeyDeleted => { #[cfg(feature = "stats")] self.stats.hits_deleted.fetch_add(1, Ordering::Relaxed); if !FIND_ALL { span.record("result_size", "deleted"); return Ok(SmallVec::new()); } - // Tombstone is last in key group. Return accumulated - // values (from this SST and newer layers). Stop - // searching older SSTs. + // A key tombstone deletes every older value for this + // key. Return what we accumulated from this SST and newer + // layers and stop searching older SSTs. if output.is_empty() { span.record("result_size", "deleted"); } else { @@ -1994,9 +2068,19 @@ impl TurboPersistence } return Ok(output); } + LookupValue::KeyValueDeleted { value } => { + #[cfg(feature = "stats")] + self.stats.hits_deleted.fetch_add(1, Ordering::Relaxed); + // Cannot terminate the search: older layers may hold other + // values for the same key. + deleted_values.insert(value); + } LookupValue::Slice { value } => { #[cfg(feature = "stats")] self.stats.hits_small.fetch_add(1, Ordering::Relaxed); + if deleted_values.contains(&value) { + continue; + } if !FIND_ALL { span.record("result_size", value.len()); return Ok(SmallVec::from_buf([value])); @@ -2008,6 +2092,9 @@ impl TurboPersistence #[cfg(feature = "stats")] self.stats.hits_blob.fetch_add(1, Ordering::Relaxed); let blob = self.read_blob(sequence_number)?; + if deleted_values.iter().any(|d| **d == *blob) { + continue; + } if !FIND_ALL { span.record("result_size", blob.len()); return Ok(SmallVec::from_buf([blob])); @@ -2120,12 +2207,20 @@ impl TurboPersistence if let Some(result) = result { inner.accessed_key_hashes[family].insert(hash); let result = match result { - LookupValue::Deleted => { + LookupValue::KeyDeleted => { #[cfg(feature = "stats")] self.stats.hits_deleted.fetch_add(1, Ordering::Relaxed); deleted += 1; None } + LookupValue::KeyValueDeleted { .. } => { + // Key-value tombstones are only written to MultiValue families, and + // `batch_get` rejects those above. + bail!( + "unexpected key-value tombstone in SingleValue family {}", + self.config.family_configs[family].name + ) + } LookupValue::Slice { value } => { #[cfg(feature = "stats")] self.stats.hits_small.fetch_add(1, Ordering::Relaxed); diff --git a/turbopack/crates/turbo-persistence/src/lib.rs b/turbopack/crates/turbo-persistence/src/lib.rs index ebc5e52f1daa..fa0e9bd484e7 100644 --- a/turbopack/crates/turbo-persistence/src/lib.rs +++ b/turbopack/crates/turbo-persistence/src/lib.rs @@ -75,6 +75,9 @@ impl Default for DbConfig { } } } +/// The largest value that [`WriteBatch::delete_value`] can delete, since the tombstone stores +/// a copy of the value inline. +pub use constants::MAX_INLINE_VALUE_SIZE; pub use key::{KeyBase, QueryKey, StoreKey, hash_key}; pub use meta_file::MetaEntryFlags; pub use parallel_scheduler::{ParallelScheduler, SerialScheduler}; diff --git a/turbopack/crates/turbo-persistence/src/lookup_entry.rs b/turbopack/crates/turbo-persistence/src/lookup_entry.rs index 108625ed872c..a7fa17544ac3 100644 --- a/turbopack/crates/turbo-persistence/src/lookup_entry.rs +++ b/turbopack/crates/turbo-persistence/src/lookup_entry.rs @@ -11,7 +11,10 @@ use crate::{ #[derive(PartialEq)] pub enum LookupValue { /// The value was deleted. - Deleted, + KeyDeleted, + /// A single value was deleted from this key's group (MultiValue families only). Other values + /// for the same key are unaffected. The bytes are the deleted value. + KeyValueDeleted { value: B }, /// The value is stored in the SST file. /// /// The bytes will be pointing either at a keyblock or a value block in the SST @@ -24,7 +27,9 @@ pub enum LookupValue { /// non-atomic refcounting). pub enum IterValue { /// The value was deleted. - Deleted, + KeyDeleted, + /// A single value was deleted from this key's group (MultiValue families only). + KeyValueDeleted { value: RcBytes }, /// The value is stored in the SST file. Slice { value: RcBytes }, /// The value is stored in a blob file. @@ -39,7 +44,8 @@ pub enum IterValue { impl From> for IterValue { fn from(v: LookupValue) -> Self { match v { - LookupValue::Deleted => IterValue::Deleted, + LookupValue::KeyDeleted => IterValue::KeyDeleted, + LookupValue::KeyValueDeleted { value } => IterValue::KeyValueDeleted { value }, LookupValue::Slice { value } => IterValue::Slice { value }, LookupValue::Blob { sequence_number } => IterValue::Blob { sequence_number }, } @@ -70,7 +76,8 @@ impl Entry for LookupEntry { fn value(&self) -> EntryValue<'_> { match &self.value { - IterValue::Deleted => EntryValue::Deleted, + IterValue::KeyDeleted => EntryValue::KeyDeleted, + IterValue::KeyValueDeleted { value } => EntryValue::KeyValueDeleted { value }, IterValue::Slice { value } => { if value.len() <= MAX_INLINE_VALUE_SIZE { EntryValue::Inline { value } diff --git a/turbopack/crates/turbo-persistence/src/meta_file.rs b/turbopack/crates/turbo-persistence/src/meta_file.rs index dbde5538cf3b..5a8ba6ff0cf2 100644 --- a/turbopack/crates/turbo-persistence/src/meta_file.rs +++ b/turbopack/crates/turbo-persistence/src/meta_file.rs @@ -49,6 +49,9 @@ impl Display for MetaEntryFlags { } } +/// Magic number identifying a `.meta` file. +pub(crate) const META_FILE_MAGIC: u32 = 0xFE4ADA4A; + /// On-disk layout of a single entry header in the `.meta` file. /// /// Fields are big-endian to match the existing wire format written by [`MetaFileBuilder`]. @@ -139,6 +142,10 @@ impl MetaEntry { self.amqf_data_offset.end - self.amqf_data_offset.start } + pub fn amqf(&self) -> &qfilter::FilterRef<'static> { + &self.amqf + } + /// Returns the raw serialized AMQF bytes from the mmap. pub fn raw_amqf<'l>(&self, amqf_data: &'l [u8]) -> &'l [u8] { &amqf_data[self.amqf_data_offset.start as usize..self.amqf_data_offset.end as usize] @@ -276,7 +283,7 @@ impl MetaFile { // Parse the header from the mmap via ReadBytesExt on &[u8]. let mut reader: &[u8] = &mmap; let magic = reader.read_u32::()?; - if magic != 0xFE4ADA4A { + if magic != META_FILE_MAGIC { bail!("Invalid magic number"); } let family = reader.read_u32::()?; @@ -480,10 +487,12 @@ impl MetaFile { // Return immediately with the first result return Ok(MetaLookupResult::SstLookup(SstLookupResult::Found(values))); } - // Check for tombstone — stops search across older SSTs within this meta file. - // Since tombstones sort last within a key group, if the last value is Deleted, - // we have a tombstone. - let has_tombstone = values.last().is_some_and(|v| *v == LookupValue::Deleted); + // A key tombstone stops the search across older SSTs within this meta file. + // It sorts last within a key group, so it is the last value if present. + // Key-value tombstones do not stop the search: they delete a single value, + // and older SSTs may hold others for this key. + let has_tombstone = + values.last().is_some_and(|v| *v == LookupValue::KeyDeleted); all_results.extend(values); if has_tombstone { return Ok(MetaLookupResult::SstLookup(SstLookupResult::Found( diff --git a/turbopack/crates/turbo-persistence/src/meta_file_builder.rs b/turbopack/crates/turbo-persistence/src/meta_file_builder.rs index 7d7d8c244c56..af2baaae2b45 100644 --- a/turbopack/crates/turbo-persistence/src/meta_file_builder.rs +++ b/turbopack/crates/turbo-persistence/src/meta_file_builder.rs @@ -9,7 +9,10 @@ use fs_err::File; use qfilter::Filter; use zerocopy::IntoBytes; -use crate::{meta_file::EntryHeader, static_sorted_file_builder::StaticSortedFileBuilderMeta}; +use crate::{ + meta_file::{EntryHeader, META_FILE_MAGIC}, + static_sorted_file_builder::StaticSortedFileBuilderMeta, +}; pub struct MetaFileBuilder<'a> { family: u32, @@ -54,7 +57,7 @@ impl<'a> MetaFileBuilder<'a> { // Wrap the writer to count the bytes written, so callers can accumulate written-byte totals // without stat'ing the file afterwards. let mut file = CountingWriter::new(BufWriter::new(File::create(file)?)); - file.write_u32::(0xFE4ADA4A)?; // Magic number + file.write_u32::(META_FILE_MAGIC)?; // Magic number file.write_u32::(self.family)?; self.obsolete_sst_files.sort(); diff --git a/turbopack/crates/turbo-persistence/src/rc_bytes.rs b/turbopack/crates/turbo-persistence/src/rc_bytes.rs index f9f2097cf652..234ddb828f14 100644 --- a/turbopack/crates/turbo-persistence/src/rc_bytes.rs +++ b/turbopack/crates/turbo-persistence/src/rc_bytes.rs @@ -1,6 +1,7 @@ use std::{ borrow::Borrow, fmt::{self, Debug, Formatter}, + hash::{Hash, Hasher}, ops::{Deref, Range}, rc::Rc, }; @@ -75,6 +76,12 @@ impl Debug for RcBytes { impl Eq for RcBytes {} +impl Hash for RcBytes { + fn hash(&self, state: &mut H) { + Hash::hash(self.deref(), state); + } +} + impl SharedBytes for RcBytes { type MmapHandle = Rc; diff --git a/turbopack/crates/turbo-persistence/src/static_sorted_file.rs b/turbopack/crates/turbo-persistence/src/static_sorted_file.rs index 4bd1a233a3c8..93c3985fc55b 100644 --- a/turbopack/crates/turbo-persistence/src/static_sorted_file.rs +++ b/turbopack/crates/turbo-persistence/src/static_sorted_file.rs @@ -40,16 +40,30 @@ pub const BLOCK_TYPE_FIXED_KEY_WITH_HASH: u8 = 3; /// The block header for a fixed-size key block without hash. pub const BLOCK_TYPE_FIXED_KEY_NO_HASH: u8 = 4; +/// Written in a fixed-size key block header's value type field when entries share a value size but +/// not a value type. Each entry then carries its own type byte ahead of its value. +pub const FIXED_KEY_BLOCK_MIXED_VALUE_TYPE: u8 = 4; + /// The tag for a small-sized value. pub const KEY_BLOCK_ENTRY_TYPE_SMALL: u8 = 0; /// The tag for the blob value. pub const KEY_BLOCK_ENTRY_TYPE_BLOB: u8 = 1; -/// The tag for the deleted value. -pub const KEY_BLOCK_ENTRY_TYPE_DELETED: u8 = 2; +/// The tag for the deleted value. This is a *key* tombstone: it deletes every value for the key. +pub const KEY_BLOCK_ENTRY_TYPE_KEY_DELETED: u8 = 2; /// The tag for a medium-sized value. pub const KEY_BLOCK_ENTRY_TYPE_MEDIUM: u8 = 3; /// The minimum tag for inline values. The actual size is (tag - INLINE_MIN). pub const KEY_BLOCK_ENTRY_TYPE_INLINE_MIN: u8 = 8; +/// The minimum tag for a key-value tombstone, which deletes only the one value it carries and +/// leaves other values for the same key intact. Only meaningful for +/// [`FamilyKind::MultiValue`][crate::FamilyKind::MultiValue] families. +/// +/// This mirrors the inline value range: the deleted value is stored inline in the key block and +/// its size is (tag - KEY_VALUE_DELETED_MIN). Only inline-sized values can be deleted this way — +/// a tombstone for a larger value would have to store a second copy of it, costing more than the +/// value it reclaims. +pub const KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN: u8 = + KEY_BLOCK_ENTRY_TYPE_INLINE_MIN + MAX_INLINE_VALUE_SIZE as u8 + 1; /// Encoded size of a small value reference: 2B block index + 2B size + 4B offset. pub(crate) const SMALL_VALUE_REF_SIZE: usize = 8; @@ -58,12 +72,13 @@ pub(crate) const MEDIUM_VALUE_REF_SIZE: usize = 2; /// Encoded size of a blob value reference: 4B blob id. pub(crate) const BLOB_VALUE_REF_SIZE: usize = 4; /// Encoded size of a deleted (tombstone) value reference. -pub(crate) const DELETED_VALUE_REF_SIZE: usize = 0; +pub(crate) const KEY_DELETED_REF_SIZE: usize = 0; -// Static assertion: MAX_INLINE_VALUE_SIZE must fit in the key type encoding. -// Key types 8-255 encode inline values of size 0-247, so max is 255 - 8 = 247. +// Static assertion: both the inline range and the key-value tombstone range that follows it must +// fit in the key type byte. The tombstone range starts after the inline range and is the same +// width, so the tombstone range's top is the binding constraint. const _: () = assert!( - MAX_INLINE_VALUE_SIZE <= (u8::MAX - KEY_BLOCK_ENTRY_TYPE_INLINE_MIN) as usize, + MAX_INLINE_VALUE_SIZE <= (u8::MAX - KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN) as usize, "MAX_INLINE_VALUE_SIZE exceeds what can be encoded in key type byte" ); @@ -369,19 +384,21 @@ impl StaticSortedFile { ensure!(block.len() >= 6, "fixed key block too short"); let entry_count = be::read_u24(&block[1..]) as usize; let key_size = be::read_u8(&block[4..]) as usize; - let value_type = be::read_u8(&block[5..]); - let val_size = entry_val_size(value_type)?; + let header_type = be::read_u8(&block[5..]); + let FixedValueLayout { + value_type, + val_size, + header_size, + } = fixed_value_layout(&block, header_type)?; let stride = hash_len as usize + key_size + val_size; - let entries = &block[6..]; + let entries = &block[header_size..]; ensure!( entries.len() == entry_count * stride, "fixed key block for {entry_count} entries must is the wrong size" ); self.lookup_block_inner::(&block, entry_count, key_hash, key, reader, |i| { - Ok(get_fixed_key_entry( - entries, i, hash_len, key_size, value_type, stride, - )) + get_fixed_key_entry(entries, i, hash_len, key_size, value_type, stride) }) } @@ -422,10 +439,9 @@ impl StaticSortedFile { return Ok(SstLookupResult::Found(SmallVec::from_buf([result]))); } // FIND_ALL (MultiValue) mode: collect all values for this key. - // Tombstones (Deleted) sort last within each key group, so we - // scan backward to find the start of the key group, then forward - // to collect all entries. The tombstone, if present, will be the - // last entry in the results. + // Within a key group, key-value tombstones sort first and key tombstones + // last. We scan backward to find the start of the key group, then forward to + // collect all entries. let mut results = SmallVec::new(); for i in (l..m).rev() { let GetKeyEntryResult { @@ -439,12 +455,10 @@ impl StaticSortedFile { } results.push(self.handle_key_match(ty, val, block, reader)?); } - // Technically we could `.reverse()` the items collected by the backwards - // scan, but the only ordering constraint we need to maintain for single - // sst multivalue reads is that a deleted token, if it exists comes last. - // Because all the backwards scan items are strictly before the found item - // we know they don't contain the _last_ item. So we don't care about - // their order. + // Restore on-disk order: callers depend on both ends of the key group, with + // key-value tombstones preceding the values they filter and a key tombstone + // landing last. + results.reverse(); // Add the entry at `m` results.push(self.handle_key_match(ty, val, block, reader)?); @@ -712,7 +726,14 @@ fn handle_key_match_generic( let sequence_number = be::read_u32(val); LookupValue::Blob { sequence_number } } - KEY_BLOCK_ENTRY_TYPE_DELETED => LookupValue::Deleted, + KEY_BLOCK_ENTRY_TYPE_KEY_DELETED => LookupValue::KeyDeleted, + // Must precede the inline arm: both are open-ended and the tombstone range sits above it. + ty if ty >= KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN => { + // The deleted value is stored inline, so `val` is already the correct slice. + // SAFETY: val points into key_block's data + let value = unsafe { key_block.slice_from_subslice(val) }; + LookupValue::KeyValueDeleted { value } + } _ => { // Inline value — val is already the correct slice // SAFETY: val points into key_block's data @@ -747,11 +768,12 @@ pub struct StaticSortedFileIter { enum CurrentKeyBlockKind { /// Variable-size entries with an offset table for random access. Variable { offsets: RcBytes, hash_len: u8 }, - /// Fixed-size entries with uniform key size and value type (no offset table). + /// Fixed-size entries with uniform key size and value size (no offset table). Fixed { hash_len: u8, key_size: usize, - value_type: u8, + /// The type shared by every entry, or `None` if each entry carries its own type byte. + value_type: Option, stride: usize, }, } @@ -865,11 +887,13 @@ impl StaticSortedFileIter { 0 }; let key_size = data[4] as usize; - let value_type = data[5]; - let val_size = entry_val_size(value_type)?; + let FixedValueLayout { + value_type, + val_size, + header_size, + } = fixed_value_layout(data, data[5])?; let stride = hash_len as usize + key_size + val_size; - // Header is 6 bytes for fixed-size blocks - let entries_range = 6..block.len(); + let entries_range = header_size..block.len(); let entries = block.slice(entries_range); Ok(CurrentKeyBlock { kind: CurrentKeyBlockKind::Fixed { @@ -912,7 +936,7 @@ impl StaticSortedFileIter { *key_size, *value_type, *stride, - ), + )?, }; let full_hash = if hash.is_empty() { crate::key::hash_key(&key) @@ -1017,7 +1041,11 @@ fn entry_val_size(ty: u8) -> Result { KEY_BLOCK_ENTRY_TYPE_SMALL => Ok(SMALL_VALUE_REF_SIZE), KEY_BLOCK_ENTRY_TYPE_MEDIUM => Ok(MEDIUM_VALUE_REF_SIZE), KEY_BLOCK_ENTRY_TYPE_BLOB => Ok(BLOB_VALUE_REF_SIZE), - KEY_BLOCK_ENTRY_TYPE_DELETED => Ok(DELETED_VALUE_REF_SIZE), + KEY_BLOCK_ENTRY_TYPE_KEY_DELETED => Ok(KEY_DELETED_REF_SIZE), + // Must precede the inline arm: both are open-ended and the tombstone range sits above it. + ty if ty >= KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN => { + Ok((ty - KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN) as usize) + } ty if ty >= KEY_BLOCK_ENTRY_TYPE_INLINE_MIN => { Ok((ty - KEY_BLOCK_ENTRY_TYPE_INLINE_MIN) as usize) } @@ -1067,20 +1095,58 @@ fn get_key_entry<'l>( /// /// All entries have the same key size and value type, so positions are computed /// arithmetically with no offset table indirection. +/// How a fixed-size key block encodes its entry values, decoded from the block header. +struct FixedValueLayout { + /// The type shared by every entry, or `None` if each entry carries its own type byte. + value_type: Option, + /// Value bytes per entry, including any per-entry type byte. + val_size: usize, + /// Total header size, which the entry data follows. + header_size: usize, +} + +/// Decodes the value layout from a fixed-size key block header. +fn fixed_value_layout(block: &[u8], header_type: u8) -> Result { + if header_type == FIXED_KEY_BLOCK_MIXED_VALUE_TYPE { + // Mixed-type block: the value size follows the header's type byte, and each entry + // carries its own type. + ensure!(block.len() >= 7, "mixed-type fixed key block too short"); + Ok(FixedValueLayout { + value_type: None, + // +1 for the per-entry type byte, which is part of the stride. + val_size: be::read_u8(&block[6..]) as usize + 1, + header_size: 7, + }) + } else { + Ok(FixedValueLayout { + value_type: Some(header_type), + val_size: entry_val_size(header_type)?, + header_size: 6, + }) + } +} + fn get_fixed_key_entry<'l>( entries: &'l [u8], index: usize, hash_len: u8, key_size: usize, - value_type: u8, + value_type: Option, stride: usize, -) -> GetKeyEntryResult<'l> { +) -> Result> { let hash_len_usize = hash_len as usize; let start = index * stride; - GetKeyEntryResult { - hash: &entries[start..start + hash_len_usize], - key: &entries[start + hash_len_usize..start + hash_len_usize + key_size], - ty: value_type, - val: &entries[start + hash_len_usize + key_size..(index + 1) * stride], - } + let key_start = start + hash_len_usize; + let key_end = key_start + key_size; + // In a mixed-type block the entry's type byte sits between its key and its value. + let (ty, val_start) = match value_type { + Some(ty) => (ty, key_end), + None => (be::read_u8(&entries[key_end..]), key_end + 1), + }; + Ok(GetKeyEntryResult { + hash: &entries[start..key_start], + key: &entries[key_start..key_end], + ty, + val: &entries[val_start..(index + 1) * stride], + }) } diff --git a/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs b/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs index 66be2750686f..f4e25667622f 100644 --- a/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs +++ b/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs @@ -15,9 +15,11 @@ use crate::{ meta_file::MetaEntryFlags, static_sorted_file::{ BLOB_VALUE_REF_SIZE, BLOCK_TYPE_FIXED_KEY_NO_HASH, BLOCK_TYPE_FIXED_KEY_WITH_HASH, - BLOCK_TYPE_INDEX, BLOCK_TYPE_KEY_NO_HASH, BLOCK_TYPE_KEY_WITH_HASH, DELETED_VALUE_REF_SIZE, - KEY_BLOCK_ENTRY_TYPE_BLOB, KEY_BLOCK_ENTRY_TYPE_DELETED, KEY_BLOCK_ENTRY_TYPE_INLINE_MIN, - KEY_BLOCK_ENTRY_TYPE_MEDIUM, KEY_BLOCK_ENTRY_TYPE_SMALL, MEDIUM_VALUE_REF_SIZE, + BLOCK_TYPE_INDEX, BLOCK_TYPE_KEY_NO_HASH, BLOCK_TYPE_KEY_WITH_HASH, + FIXED_KEY_BLOCK_MIXED_VALUE_TYPE, KEY_BLOCK_ENTRY_TYPE_BLOB, + KEY_BLOCK_ENTRY_TYPE_INLINE_MIN, KEY_BLOCK_ENTRY_TYPE_KEY_DELETED, + KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN, KEY_BLOCK_ENTRY_TYPE_MEDIUM, + KEY_BLOCK_ENTRY_TYPE_SMALL, KEY_DELETED_REF_SIZE, MEDIUM_VALUE_REF_SIZE, SMALL_VALUE_REF_SIZE, }, }; @@ -65,6 +67,12 @@ const MIN_KEY_SIZE_FOR_COMPRESSION: usize = 16; /// fall back to variable-size layout. const MAX_FIXED_KEY_LEN: usize = u8::MAX as usize; +/// Maximum value size that can use fixed-size key block layout. +/// +/// Mixed-type fixed blocks store the value size in a single header byte, since it can no longer be +/// derived from a single shared entry type. +const MAX_FIXED_VAL_SIZE: usize = u8::MAX as usize; + /// Newtype for the key block entry type byte. /// /// This encodes what kind of value reference an entry has (small, medium, blob, deleted, or @@ -74,33 +82,46 @@ struct EntryType(u8); /// Tracks whether a key block's entries are uniform enough for fixed-size layout. /// +/// Fixed layout needs a uniform *stride*, which requires a uniform key length and a uniform value +/// size. A uniform value *type* is a stronger condition that additionally lets the type be hoisted +/// into the block header; when types differ but sizes agree, the type is stored per entry instead +/// (1 byte, still cheaper than the 4-byte offset table entry a variable block would need). +/// /// State transitions: -/// - `Unknown` → first entry → `Fixed { key_len, value_type }` -/// - `Fixed` + matching entry → stays `Fixed` -/// - `Fixed` + mismatched key_len or value_type → `Variable` +/// - `Unknown` → first entry → `Fixed` +/// - `Fixed` + matching key_len and value type → stays `Fixed` +/// - `Fixed` + matching key_len and value *size* → `Fixed` with `value_type: None` +/// - `Fixed` + mismatched key_len or value size → `Variable` /// - `Variable` → stays `Variable` #[derive(Clone, Copy)] enum KeyBlockFormat { /// No entries yet — format undetermined. Unknown, - /// All entries so far have uniform key length and value type. - Fixed { key_len: u8, value_type: EntryType }, - /// Entries have mixed key lengths or value types; must use offset table. + /// All entries so far have uniform key length and value size. + Fixed { + key_len: u8, + val_size: u8, + /// The shared entry type, or `None` if entries have differing types of the same size. + value_type: Option, + }, + /// Entries have mixed key lengths or value sizes; must use offset table. Variable, } impl KeyBlockFormat { /// Updates the format after seeing an entry with the given key length and value type. /// - /// A `Fixed` state is only reachable when all entries have matching key length and value type, + /// A `Fixed` state is only reachable when all entries have matching key length and value size, /// and the key length fits in a u8 (required by the on-disk header). fn update(&mut self, key_len: usize, value_type: EntryType) { + let val_size = value_type_val_size(value_type); *self = match *self { KeyBlockFormat::Unknown => { - if key_len <= MAX_FIXED_KEY_LEN { + if key_len <= MAX_FIXED_KEY_LEN && val_size <= MAX_FIXED_VAL_SIZE { KeyBlockFormat::Fixed { key_len: key_len as u8, - value_type, + val_size: val_size as u8, + value_type: Some(value_type), } } else { KeyBlockFormat::Variable @@ -108,10 +129,13 @@ impl KeyBlockFormat { } KeyBlockFormat::Fixed { key_len: k, + val_size: s, value_type: v, - } if k as usize == key_len && v == value_type => KeyBlockFormat::Fixed { + } if k as usize == key_len && s as usize == val_size => KeyBlockFormat::Fixed { key_len: k, - value_type: v, + val_size: s, + // Collapse to `None` as soon as two entries disagree on type. + value_type: v.filter(|v| *v == value_type), }, KeyBlockFormat::Fixed { .. } | KeyBlockFormat::Variable => KeyBlockFormat::Variable, }; @@ -250,7 +274,11 @@ pub enum EntryValue<'l> { /// Large-sized value. They are stored in a blob file. Large { blob: u32 }, /// Tombstone. The value was removed. - Deleted, + KeyDeleted, + /// Key-value tombstone. Only the one carried value was removed; other values for the same key + /// survive. MultiValue families only. The value must be at most [`MAX_INLINE_VALUE_SIZE`] + /// bytes. + KeyValueDeleted { value: &'l [u8] }, } #[derive(Debug, Clone)] @@ -392,7 +420,13 @@ enum ValueRef { /// Large blob stored externally. Blob { blob_id: u32 }, /// Tombstone. - Deleted, + KeyDeleted, + /// Key-value tombstone: deletes only the carried value from the key's group. The value is + /// stored inline, exactly like [`ValueRef::Inline`]. + KeyValueDeleted { + data: [u8; MAX_INLINE_VALUE_SIZE], + len: u8, + }, } impl ValueRef { @@ -403,7 +437,10 @@ impl ValueRef { ValueRef::Medium { .. } => KEY_BLOCK_ENTRY_TYPE_MEDIUM, ValueRef::Inline { len, .. } => KEY_BLOCK_ENTRY_TYPE_INLINE_MIN + *len, ValueRef::Blob { .. } => KEY_BLOCK_ENTRY_TYPE_BLOB, - ValueRef::Deleted => KEY_BLOCK_ENTRY_TYPE_DELETED, + ValueRef::KeyDeleted => KEY_BLOCK_ENTRY_TYPE_KEY_DELETED, + ValueRef::KeyValueDeleted { len, .. } => { + KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN + *len + } }) } @@ -437,7 +474,10 @@ impl ValueRef { BE::write_u32(&mut scratch, *blob_id); buffer.extend(scratch); } - ValueRef::Deleted => { /* no value bytes */ } + ValueRef::KeyDeleted => { /* no value bytes */ } + ValueRef::KeyValueDeleted { data, len } => { + buffer.extend(&data[..*len as usize]); + } ValueRef::PendingSmall { .. } => { unreachable!("PendingSmall should have been resolved"); } @@ -703,7 +743,17 @@ impl StreamingSstWriter { } } EntryValue::Large { blob } => ValueRef::Blob { blob_id: blob }, - EntryValue::Deleted => ValueRef::Deleted, + EntryValue::KeyDeleted => ValueRef::KeyDeleted, + EntryValue::KeyValueDeleted { value } => { + // Enforced by `WriteBatch::delete_value`, which rejects oversized values. + debug_assert!(value.len() <= MAX_INLINE_VALUE_SIZE); + let mut data = [0u8; MAX_INLINE_VALUE_SIZE]; + data[..value.len()].copy_from_slice(value); + ValueRef::KeyValueDeleted { + data, + len: value.len() as u8, + } + } }; self.push_pending_key_entry(entry, value_ref); @@ -843,6 +893,7 @@ impl StreamingSstWriter { if let KeyBlockFormat::Fixed { key_len: key_size, + val_size, value_type, } = info.format { @@ -851,6 +902,7 @@ impl StreamingSstWriter { entry_count as u32, has_hash, key_size, + val_size, value_type, ); for i in start..end { @@ -1123,13 +1175,18 @@ impl<'l> KeyBlockBuilder<'l> { // --------------------------------------------------------------------------- /// The size of the fixed-size key block header (block type + entry count + key size + value type). +/// Mixed-type blocks append one more byte for the value size. const FIXED_KEY_BLOCK_HEADER_SIZE: usize = 6; -/// Builder for a fixed-size key block where all entries share the same key size and value type. +/// Builder for a fixed-size key block where all entries share the same key size and value size. /// -/// No offset table is written — entry positions are computed arithmetically from the stride. +/// No offset table is written — entry positions are computed arithmetically from the stride. When +/// entries share a value size but not a value type, the header records +/// [`FIXED_KEY_BLOCK_MIXED_VALUE_TYPE`] and each entry carries its own type byte before its value. struct FixedKeyBlockBuilder<'l> { buffer: &'l mut Vec, + /// Whether each entry writes its own type byte (set for mixed-type blocks). + per_entry_type: bool, } impl<'l> FixedKeyBlockBuilder<'l> { @@ -1138,11 +1195,12 @@ impl<'l> FixedKeyBlockBuilder<'l> { entry_count: u32, has_hash: bool, key_size: u8, - value_type: EntryType, + val_size: u8, + value_type: Option, ) -> Self { let hash_len: usize = if has_hash { 8 } else { 0 }; - let val_size = value_type_val_size(value_type); - let stride = hash_len + key_size as usize + val_size; + let per_entry_type = value_type.is_none(); + let stride = hash_len + key_size as usize + val_size as usize + usize::from(per_entry_type); buffer.reserve(FIXED_KEY_BLOCK_HEADER_SIZE + entry_count as usize * stride); let block_type = if has_hash { @@ -1156,19 +1214,30 @@ impl<'l> FixedKeyBlockBuilder<'l> { (entry_count >> 8) as u8, entry_count as u8, key_size, - value_type.0, + value_type.map_or(FIXED_KEY_BLOCK_MIXED_VALUE_TYPE, |ty| ty.0), ]); + // Mixed-type blocks cannot derive the value size from the header's type byte, so it is + // written explicitly. + if per_entry_type { + buffer.push(val_size); + } - Self { buffer } + Self { + buffer, + per_entry_type, + } } - /// Writes a single entry (hash + key + value data) to the block. + /// Writes a single entry (hash + key + optional type byte + value data) to the block. fn put(&mut self, entry: &E, value_ref: &ValueRef, has_hash: bool) { if has_hash { self.buffer .extend_from_slice(&entry.key_hash().to_be_bytes()); } entry.write_key_to(self.buffer); + if self.per_entry_type { + self.buffer.push(value_ref.entry_type().0); + } value_ref.write_value_to(self.buffer); } @@ -1186,7 +1255,11 @@ fn value_type_val_size(ty: EntryType) -> usize { KEY_BLOCK_ENTRY_TYPE_SMALL => SMALL_VALUE_REF_SIZE, KEY_BLOCK_ENTRY_TYPE_MEDIUM => MEDIUM_VALUE_REF_SIZE, KEY_BLOCK_ENTRY_TYPE_BLOB => BLOB_VALUE_REF_SIZE, - KEY_BLOCK_ENTRY_TYPE_DELETED => DELETED_VALUE_REF_SIZE, + KEY_BLOCK_ENTRY_TYPE_KEY_DELETED => KEY_DELETED_REF_SIZE, + // Must precede the inline arm: both are open-ended and the tombstone range sits above it. + ty if ty >= KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN => { + (ty - KEY_BLOCK_ENTRY_TYPE_KEY_VALUE_DELETED_MIN) as usize + } ty if ty >= KEY_BLOCK_ENTRY_TYPE_INLINE_MIN => { (ty - KEY_BLOCK_ENTRY_TYPE_INLINE_MIN) as usize } @@ -1260,7 +1333,8 @@ mod tests { /// Already-formatted block with `uncompressed_size = 0` (stored as-is). MediumRaw(Vec), Blob(u32), - Deleted, + KeyDeleted, + KeyValueDeleted(Vec), } impl TestEntry { @@ -1292,7 +1366,7 @@ mod tests { } fn deleted(key: &[u8]) -> Self { - Self::new(key, TestValueKind::Deleted) + Self::new(key, TestValueKind::KeyDeleted) } fn medium_raw(key: &[u8], value: &[u8]) -> Self { @@ -1335,7 +1409,8 @@ mod tests { block: v, }, TestValueKind::Blob(id) => EntryValue::Large { blob: *id }, - TestValueKind::Deleted => EntryValue::Deleted, + TestValueKind::KeyDeleted => EntryValue::KeyDeleted, + TestValueKind::KeyValueDeleted(v) => EntryValue::KeyValueDeleted { value: v }, } } } @@ -1409,8 +1484,22 @@ mod tests { }; assert_eq!(*sequence_number, *expected_id); } - (TestValueKind::Deleted, SstLookupResult::Found(values)) - if values.len() == 1 && matches!(values[0], LookupValue::Deleted) => {} + (TestValueKind::KeyDeleted, SstLookupResult::Found(values)) + if values.len() == 1 && matches!(values[0], LookupValue::KeyDeleted) => {} + (TestValueKind::KeyValueDeleted(expected), SstLookupResult::Found(values)) + if values.len() == 1 + && matches!(values[0], LookupValue::KeyValueDeleted { .. }) => + { + let LookupValue::KeyValueDeleted { value } = &values[0] else { + unreachable!() + }; + assert_eq!( + value.as_ref(), + expected.as_slice(), + "tombstone value mismatch for key {:?}", + std::str::from_utf8(&entry.key) + ); + } _ => { panic!( "Unexpected lookup result for key {:?}", @@ -1710,7 +1799,7 @@ mod tests { std::str::from_utf8(&entry.key) ); } - (LookupValue::Deleted, LookupValue::Deleted) => {} + (LookupValue::KeyDeleted, LookupValue::KeyDeleted) => {} ( LookupValue::Blob { sequence_number: s1, @@ -1775,6 +1864,108 @@ mod tests { Ok(()) } + /// Reads the first key block of an SST, returning its raw (uncompressed) bytes. + /// + /// Block 0 is always a key block; the block offset table sits at the end of the file. + fn read_first_block(dir: &Path, seq: u32, block_count: u16) -> Result> { + let data = fs_err::read(dir.join(format!("{seq:08}.sst")))?; + let offsets_start = data.len() - block_count as usize * size_of::(); + let end = BE::read_u32(&data[offsets_start..]) as usize; + let raw = &data[..end]; + // Each block is prefixed by BLOCK_HEADER_SIZE bytes: 4B uncompressed size + 4B checksum. + // An uncompressed size of 0 means the block is stored as-is. + let uncompressed_size = BE::read_u32(raw) as usize; + let body = &raw[BLOCK_HEADER_SIZE..]; + Ok(if uncompressed_size == 0 { + body.to_vec() + } else { + let mut out = vec![0u8; uncompressed_size]; + lzzzz::lz4::decompress(body, &mut out)?; + out + }) + } + + /// A tombstone and a value of the same size keep the block in fixed layout. + /// + /// This is what makes tombstones cheap for uniform-key families like the task cache: without + /// the mixed-type layout, one tombstone would demote its whole block to the variable format + /// and add a 4-byte offset table entry for every entry in it. + #[test] + fn fixed_layout_survives_mixed_value_types_of_equal_size() -> Result<()> { + let dir = tempfile::tempdir()?; + + // Uniform 8-byte keys, uniform 4-byte values, but two different entry types. + let mut entries: Vec = (0..64u64) + .map(|i| { + let key = format!("k-{i:06}"); + if i % 4 == 0 { + TestEntry::new( + key.as_bytes(), + TestValueKind::KeyValueDeleted(vec![0xAAu8; 4]), + ) + } else { + TestEntry::inline(key.as_bytes(), &[0xBBu8; 4]) + } + }) + .collect(); + sort_entries(&mut entries); + + let meta = write_sst(dir.path(), 1, &entries, MetaEntryFlags::default())?; + let block = read_first_block(dir.path(), 1, meta.block_count)?; + + assert!( + block[0] == BLOCK_TYPE_FIXED_KEY_WITH_HASH || block[0] == BLOCK_TYPE_FIXED_KEY_NO_HASH, + "mixed value types of equal size should stay in fixed layout, got block type {}", + block[0] + ); + assert_eq!( + block[5], FIXED_KEY_BLOCK_MIXED_VALUE_TYPE, + "block should be marked mixed-type" + ); + assert_eq!(block[6], 4, "value size should be recorded in the header"); + + // The layout is only useful if it still reads back correctly. + let sst = open_sst(dir.path(), 1, &meta)?; + let kc = make_cache(); + let vc = make_cache(); + for entry in &entries { + assert_lookup(&sst, entry, &kc, &vc)?; + } + Ok(()) + } + + /// Differing value *sizes* cannot share a stride, so the block must fall back to variable + /// layout rather than silently misreading entries. + #[test] + fn mixed_value_sizes_fall_back_to_variable_layout() -> Result<()> { + let dir = tempfile::tempdir()?; + + let mut entries: Vec = (0..64u64) + .map(|i| { + let key = format!("k-{i:06}"); + let len = if i % 4 == 0 { 2 } else { 4 }; + TestEntry::inline(key.as_bytes(), &vec![0xCCu8; len]) + }) + .collect(); + sort_entries(&mut entries); + + let meta = write_sst(dir.path(), 1, &entries, MetaEntryFlags::default())?; + let block = read_first_block(dir.path(), 1, meta.block_count)?; + assert!( + block[0] == BLOCK_TYPE_KEY_WITH_HASH || block[0] == BLOCK_TYPE_KEY_NO_HASH, + "differing value sizes should use variable layout, got block type {}", + block[0] + ); + + let sst = open_sst(dir.path(), 1, &meta)?; + let kc = make_cache(); + let vc = make_cache(); + for entry in &entries { + assert_lookup(&sst, entry, &kc, &vc)?; + } + Ok(()) + } + #[test] fn single_medium_raw_entry() -> Result<()> { let dir = tempfile::tempdir()?; diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index da007139c160..6bb30c27bed0 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -5,9 +5,11 @@ use rayon::iter::{IntoParallelIterator, ParallelIterator}; use crate::{ DbConfig, FamilyConfig, FamilyKind, - constants::{MAX_MEDIUM_VALUE_SIZE, MAX_SMALL_VALUE_SIZE}, + constants::{MAX_INLINE_VALUE_SIZE, MAX_MEDIUM_VALUE_SIZE, MAX_SMALL_VALUE_SIZE}, db::{CompactConfig, TurboPersistence, read_current_version}, + lookup_entry::IterValue, parallel_scheduler::ParallelScheduler, + static_sorted_file::{StaticSortedFileIter, StaticSortedFileMetaData}, write_batch::WriteBatch, }; @@ -2261,6 +2263,564 @@ fn current_file_is_json_with_commit_time() -> Result<()> { "commit_time {} outside [{before}, {after}]", version.commit_time ); + Ok(()) +} + +/// A key-value tombstone deletes only the pair it names, leaving other values for the same key. +#[test] +fn valued_tombstone_deletes_only_its_pair() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + let key = vec![1u8]; + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + // Three values under one key, each in its own SST. + for v in [10u32, 20, 30] { + let batch = db.write_batch()?; + batch.put(0, key.clone(), v.to_be_bytes().to_vec().into())?; + db.commit_write_batch(batch)?; + } + + // Delete just the middle one. + let batch = db.write_batch()?; + batch.delete_value(0, key.clone(), 20u32.to_be_bytes().to_vec().into())?; + db.commit_write_batch(batch)?; + + let mut results = db + .get_multiple(0, &key.as_slice())? + .iter() + .map(|v| u32::from_be_bytes((**v).try_into().unwrap())) + .collect::>(); + results.sort(); + assert_eq!(results, vec![10, 30], "only the named pair should be gone"); + + db.shutdown()?; + Ok(()) +} + +/// A partial compaction must NOT drop a key-value tombstone: an unmerged older SST may still hold a +/// matching value, and dropping the tombstone would resurrect it. +#[test] +fn valued_tombstone_survives_partial_compaction() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + // Enough distinct keys that each SST spans a real slice of the hash space. The compaction + // selector estimates duplication by scaling sizes against that spread, so a couple of keys + // sharing one hash makes the estimate degenerate and no merge is ever chosen — which would + // leave this test passing without compacting anything. + const KEYS: u32 = 2000; + + // Oldest layer holds the values that the tombstones must keep suppressing. + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.put( + 0, + k.to_be_bytes().to_vec(), + 42u32.to_be_bytes().to_vec().into(), + )?; + } + db.commit_write_batch(batch)?; + + // Newer layers so compaction has overlapping candidates to merge partially. + for v in [1u32, 2, 3] { + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.put(0, k.to_be_bytes().to_vec(), v.to_be_bytes().to_vec().into())?; + } + db.commit_write_batch(batch)?; + } + + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.delete_value( + 0, + k.to_be_bytes().to_vec(), + 42u32.to_be_bytes().to_vec().into(), + )?; + } + db.commit_write_batch(batch)?; + + // Compact repeatedly; whatever coverage the selector chooses, 42 must stay deleted. + for round in 0..3 { + db.compact(&CompactConfig { + min_merge_count: 2, + optimal_merge_count: 2, + min_merge_duplication_bytes: 1, + optimal_merge_duplication_bytes: 1, + ..Default::default() + })?; + for k in [0u32, KEYS / 2, KEYS - 1] { + let results = db + .get_multiple(0, &k.to_be_bytes().to_vec().as_slice())? + .iter() + .map(|v| u32::from_be_bytes((**v).try_into().unwrap())) + .collect::>(); + assert!( + !results.contains(&42), + "42 was resurrected for key {k} in round {round}: {results:?}" + ); + } + } + + db.shutdown()?; + Ok(()) +} + +/// Deletes must survive a reopen: the tombstone is persisted, not just held in memory. +#[test] +fn valued_tombstone_persists_across_reopen() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + let key = vec![3u8]; + + { + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + let batch = db.write_batch()?; + batch.put(0, key.clone(), 100u32.to_be_bytes().to_vec().into())?; + batch.put(0, key.clone(), 200u32.to_be_bytes().to_vec().into())?; + db.commit_write_batch(batch)?; + + let batch = db.write_batch()?; + batch.delete_value(0, key.clone(), 100u32.to_be_bytes().to_vec().into())?; + db.commit_write_batch(batch)?; + db.shutdown()?; + } + + { + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + let results = db + .get_multiple(0, &key.as_slice())? + .iter() + .map(|v| u32::from_be_bytes((**v).try_into().unwrap())) + .collect::>(); + assert_eq!(results, vec![200], "tombstone lost across reopen"); + db.shutdown()?; + } + + Ok(()) +} + +/// A key tombstone still deletes everything, including values a key-value tombstone left alone. +#[test] +fn whole_key_tombstone_still_deletes_all_values() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + let key = vec![5u8]; + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + let batch = db.write_batch()?; + batch.put(0, key.clone(), 1u32.to_be_bytes().to_vec().into())?; + batch.put(0, key.clone(), 2u32.to_be_bytes().to_vec().into())?; + db.commit_write_batch(batch)?; + + let batch = db.write_batch()?; + batch.delete_value(0, key.clone(), 1u32.to_be_bytes().to_vec().into())?; + db.commit_write_batch(batch)?; + + let batch = db.write_batch()?; + batch.delete(0, key.clone())?; + db.commit_write_batch(batch)?; + + assert!( + db.get_multiple(0, &key.as_slice())?.is_empty(), + "key tombstone should remove everything" + ); + + db.shutdown()?; + Ok(()) +} + +/// Counts tombstone entries (both kinds) across every live SST, by reading the files directly. +/// Tombstone counts are not tracked in the meta file, so there is nothing cheaper to read. +fn count_tombstones( + path: &Path, + db: &TurboPersistence, +) -> Result { + let mut count = 0; + for meta in db.meta_info()? { + for entry in &meta.entries { + let sst = StaticSortedFileMetaData { + sequence_number: entry.sequence_number, + block_count: entry.block_count, + }; + for item in StaticSortedFileIter::open(path, sst)? { + if matches!( + item?.value, + IterValue::KeyDeleted | IterValue::KeyValueDeleted { .. } + ) { + count += 1; + } + } + } + } + Ok(count) +} + +/// Compaction reclaims tombstones once no *older* SST outside the job can still hold the key. +/// Without this, tombstones accumulate forever. +#[test] +fn compaction_reclaims_tombstones_when_no_older_sst_has_the_key() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + // Enough distinct keys that the SSTs span a real hash range: the compaction selector + // estimates duplication by scaling sizes against the key-space spread, and a handful of + // keys sharing one hash makes that estimate degenerate. + const KEYS: u32 = 2000; + + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.put( + 0, + k.to_be_bytes().to_vec(), + 1u32.to_be_bytes().to_vec().into(), + )?; + batch.put( + 0, + k.to_be_bytes().to_vec(), + 2u32.to_be_bytes().to_vec().into(), + )?; + } + db.commit_write_batch(batch)?; + + // Delete one of the two values for every key. + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.delete_value( + 0, + k.to_be_bytes().to_vec(), + 1u32.to_be_bytes().to_vec().into(), + )?; + } + db.commit_write_batch(batch)?; + + assert_eq!( + count_tombstones(path, &db)?, + KEYS as usize, + "tombstones should be on disk before compaction" + ); + db.compact(&CompactConfig { + min_merge_count: 2, + optimal_merge_count: 2, + min_merge_duplication_bytes: 1, + optimal_merge_duplication_bytes: 1, + ..Default::default() + })?; + + // No older SST holds these keys, so every tombstone is dead weight and must be gone. Assert + // on the tombstone entries themselves: total file size would shrink from dropping the deleted + // values alone, so it cannot distinguish a working probe from one that never fires. + assert_eq!( + count_tombstones(path, &db)?, + 0, + "compaction should have reclaimed every tombstone" + ); + + // ...and the deletes must still hold after reclamation. + for k in [0u32, KEYS / 2, KEYS - 1] { + let results = db + .get_multiple(0, &k.to_be_bytes().to_vec().as_slice())? + .iter() + .map(|v| u32::from_be_bytes((**v).try_into().unwrap())) + .collect::>(); + assert_eq!(results, vec![2], "value 1 must stay deleted for key {k}"); + } + + db.shutdown()?; + Ok(()) +} + +/// When an older SST *outside* the compaction job still holds the key, the tombstone must be +/// kept. Dropping it would resurrect the value. +#[test] +fn compaction_keeps_tombstone_when_older_sst_has_the_key() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + const KEYS: u32 = 2000; + + // Oldest layer: the values that must stay suppressed. + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.put( + 0, + k.to_be_bytes().to_vec(), + 1u32.to_be_bytes().to_vec().into(), + )?; + } + db.commit_write_batch(batch)?; + + // Newer layers: an unrelated value per key, then a tombstone for the old one. + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.put( + 0, + k.to_be_bytes().to_vec(), + 2u32.to_be_bytes().to_vec().into(), + )?; + } + db.commit_write_batch(batch)?; + + let batch = db.write_batch()?; + for k in 0..KEYS { + batch.delete_value( + 0, + k.to_be_bytes().to_vec(), + 1u32.to_be_bytes().to_vec().into(), + )?; + } + db.commit_write_batch(batch)?; + + // Compact repeatedly. Whatever subset each job picks, value 1 must never come back: while an + // older SST still holds it, the probe has to keep the tombstone alive. + for round in 0..4 { + db.compact(&CompactConfig { + min_merge_count: 2, + optimal_merge_count: 2, + min_merge_duplication_bytes: 1, + optimal_merge_duplication_bytes: 1, + ..Default::default() + })?; + + for k in [0u32, KEYS / 2, KEYS - 1] { + let mut results = db + .get_multiple(0, &k.to_be_bytes().to_vec().as_slice())? + .iter() + .map(|v| u32::from_be_bytes((**v).try_into().unwrap())) + .collect::>(); + results.sort(); + assert_eq!( + results, + vec![2], + "value 1 resurrected for key {k} in round {round}" + ); + } + } + + db.shutdown()?; + Ok(()) +} + +/// A compaction job's SSTs need not be contiguous: the selector picks members by hash-range +/// overlap and skips already-claimed candidates, so an SST can sit "between" two job members +/// without belonging to the job. A tombstone must still be kept for it. +/// +/// This is the case a sequence-number threshold gets wrong — it would treat the skipped SST as +/// part of the job and drop a tombstone that is still load-bearing. +#[test] +fn compaction_keeps_tombstone_when_skipped_sst_has_the_key() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + const KEYS: u32 = 2000; + + // Every SST spans a wide, overlapping slice of the hash space. That is what lets the selector + // form jobs that skip over an intervening file: with narrow or identical ranges it only ever + // picks contiguous runs, and the bug this guards against stays hidden. + let key_for = |round: u32, i: u32| { + let mut k = vec![0u8; 8]; + k[..4].copy_from_slice(&i.to_be_bytes()); + k[4..].copy_from_slice(&round.to_be_bytes()); + k + }; + + // Oldest layer: the values that must stay suppressed once deleted. + let batch = db.write_batch()?; + for i in 0..KEYS { + batch.put(0, key_for(0, i), 1u32.to_be_bytes().to_vec().into())?; + } + db.commit_write_batch(batch)?; + + // Several more layers touching the same keys, so the selector has many overlapping candidates. + for round in 1..7u32 { + let batch = db.write_batch()?; + for i in 0..KEYS { + batch.put(0, key_for(0, i), (round + 1).to_be_bytes().to_vec().into())?; + batch.put(0, key_for(round, i), 9u32.to_be_bytes().to_vec().into())?; + } + db.commit_write_batch(batch)?; + } + + // Delete the oldest value for every key in the first layer. + let batch = db.write_batch()?; + for i in 0..KEYS { + batch.delete_value(0, key_for(0, i), 1u32.to_be_bytes().to_vec().into())?; + } + db.commit_write_batch(batch)?; + + // More layers *after* the tombstone, so it sits in the middle of the stack rather than at the + // newest end. A job containing the tombstone's SST can then skip over older SSTs that still + // hold value 1 — those are only caught by probing every older SST the job does not merge, not + // just the ones below the job's oldest member. + for round in 7..12u32 { + let batch = db.write_batch()?; + for i in 0..KEYS { + batch.put(0, key_for(round, i), 9u32.to_be_bytes().to_vec().into())?; + } + db.commit_write_batch(batch)?; + } + + for round in 0..6 { + db.compact(&CompactConfig { + min_merge_count: 2, + max_merge_count: 3, + optimal_merge_count: 2, + min_merge_duplication_bytes: 1, + optimal_merge_duplication_bytes: 1, + ..Default::default() + })?; + + for i in [0u32, KEYS / 2, KEYS - 1] { + let results = db + .get_multiple(0, &key_for(0, i).as_slice())? + .iter() + .map(|v| u32::from_be_bytes((**v).try_into().unwrap())) + .collect::>(); + assert!( + !results.contains(&1), + "deleted value resurrected for key {i} in round {round}: {results:?}" + ); + } + } + + db.shutdown()?; + Ok(()) +} + +/// Tombstones carry the deleted value inline, so every size up to the inline limit round-trips. +/// The boundary sizes matter: the tag range is packed directly above the inline value range, so an +/// off-by-one in either bound would decode a tombstone as a value or vice versa. +#[test] +fn valued_tombstone_supports_all_inline_value_sizes() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + // One key per value size, each holding a deleted value of that size plus a survivor. + for len in 0..=MAX_INLINE_VALUE_SIZE { + let key = vec![len as u8]; + let doomed = vec![0xAAu8; len]; + + let batch = db.write_batch()?; + batch.put(0, key.clone(), doomed.clone().into())?; + batch.put(0, key.clone(), vec![0xBBu8; 3].into())?; + db.commit_write_batch(batch)?; + + let batch = db.write_batch()?; + batch.delete_value(0, key.clone(), doomed.clone().into())?; + db.commit_write_batch(batch)?; + + let results = db.get_multiple(0, &key.as_slice())?; + assert_eq!( + results.iter().map(|v| v.to_vec()).collect::>(), + vec![vec![0xBBu8; 3]], + "{len}-byte value should have been deleted, and only it" + ); + } + + db.shutdown()?; + Ok(()) +} + +/// Values too large to store inline are rejected rather than silently truncated: the tombstone +/// carries a copy of the value, so deleting a large value would cost more than it reclaims. +#[test] +fn valued_tombstone_rejects_values_larger_than_inline() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + multi_value_config(), + RayonParallelScheduler, + )?; + + let batch = db.write_batch()?; + let too_big = vec![0u8; MAX_INLINE_VALUE_SIZE + 1]; + let err = batch + .delete_value(0, vec![1u8], too_big.into()) + .expect_err("oversized value should be rejected"); + assert!( + err.to_string().contains("at most"), + "unexpected error: {err}" + ); + + db.shutdown()?; + Ok(()) +} + +/// Key-value tombstones are meaningless in a SingleValue family, where `delete` already removes +/// the single value exactly. Rejecting at the API keeps the tombstone off disk, where it would +/// otherwise only surface as an error at read time. +#[test] +fn valued_tombstone_rejects_single_value_families() -> Result<()> { + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let db = TurboPersistence::<_, 1>::open_with_config_and_parallel_scheduler( + path.to_path_buf(), + DbConfig::<1>::default(), + RayonParallelScheduler, + )?; + + let batch = db.write_batch()?; + let err = batch + .delete_value(0, vec![1u8], 1u32.to_be_bytes().to_vec().into()) + .expect_err("SingleValue family should be rejected"); + assert!( + err.to_string().contains("MultiValue"), + "unexpected error: {err}" + ); + + db.shutdown()?; Ok(()) } diff --git a/turbopack/crates/turbo-persistence/src/write_batch.rs b/turbopack/crates/turbo-persistence/src/write_batch.rs index 42e65aed7aef..1c837e3e18f4 100644 --- a/turbopack/crates/turbo-persistence/src/write_batch.rs +++ b/turbopack/crates/turbo-persistence/src/write_batch.rs @@ -6,7 +6,7 @@ use std::{ sync::atomic::{AtomicU32, AtomicU64, Ordering}, }; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail}; use byteorder::{BE, WriteBytesExt}; use either::Either; use fs_err::File; @@ -15,11 +15,11 @@ use smallvec::SmallVec; use thread_local::ThreadLocal; use crate::{ - FamilyConfig, ValueBuffer, + FamilyConfig, FamilyKind, ValueBuffer, collector::Collector, collector_entry::CollectorEntry, compression::{checksum_block, compress_into_buffer}, - constants::{MAX_MEDIUM_VALUE_SIZE, THREAD_LOCAL_SIZE_SHIFT}, + constants::{MAX_INLINE_VALUE_SIZE, MAX_MEDIUM_VALUE_SIZE, THREAD_LOCAL_SIZE_SHIFT}, db::WriteOperationGuard, key::StoreKey, meta_file::MetaEntryFlags, @@ -245,7 +245,11 @@ impl<'db, K: StoreKey + Send + Sync, S: ParallelScheduler, const FAMILIES: usize Ok(()) } - /// Puts a delete operation into the write batch. + /// Puts a delete operation into the write batch. This deletes *all* values for `key`. + /// + /// Combining this with a [`WriteBatch::put`] of the same key in the same batch is **not + /// supported**: which one wins is undefined, and callers are expected to resolve the intent + /// themselves before writing. pub fn delete(&self, family: u32, key: K) -> Result<()> { let state = self.thread_local_state(); let collector = self.thread_local_collector_mut(state, family)?; @@ -253,6 +257,39 @@ impl<'db, K: StoreKey + Send + Sync, S: ParallelScheduler, const FAMILIES: usize Ok(()) } + /// Deletes a single key-value pair, leaving any other values for `key` intact. + /// + /// Only valid for [`FamilyKind::MultiValue`] families: in a `SingleValue` family a key has one + /// value and [`WriteBatch::delete`] already removes it exactly. + /// + /// Deleting a pair that is written in the same batch — by this or any other operation on the + /// key — is **not supported**, for the reason given on [`WriteBatch::delete`]: which one wins + /// is undefined, and it is the caller's job to resolve that before writing. + /// + /// Only values of at most [`MAX_INLINE_VALUE_SIZE`] bytes can be deleted this way. This is a + /// simplifying limitation that could be relaxed if needed. Of course in general the storage + /// overhead of deleting large values by value makes it apriori inefficient. + pub fn delete_value(&self, family: u32, key: K, value: ValueBuffer<'_>) -> Result<()> { + let family_config = &self.family_configs[usize_from_u32(family)]; + if family_config.kind != FamilyKind::MultiValue { + bail!( + "delete_value is only valid for MultiValue families, but family {} is SingleValue", + family_config.name + ); + } + if value.len() > MAX_INLINE_VALUE_SIZE { + bail!( + "delete_value only supports values of at most {MAX_INLINE_VALUE_SIZE} bytes, got \ + {} bytes", + value.len() + ); + } + let state = self.thread_local_state(); + let collector = self.thread_local_collector_mut(state, family)?; + collector.delete_value(key, &value); + Ok(()) + } + /// Flushes a family of the write batch, reducing the amount of buffered memory used. /// Does not commit any data persistently. /// @@ -547,10 +584,22 @@ impl<'db, K: StoreKey + Send + Sync, S: ParallelScheduler, const FAMILIES: usize "we wrote a blob but did not read it" ); } - CollectorEntryValue::Deleted => assert!( - values.first() == Some(&LookupValue::Deleted), - "we wrote a deleted tombstone but it was not first in results" + // Key tombstones sort last within a key group, so a same-batch + // `put(K, v); delete(K)` reads back as [v, KeyDeleted]. + CollectorEntryValue::KeyDeleted => assert!( + values.last() == Some(&LookupValue::KeyDeleted), + "we wrote a key tombstone but it was not last in results" ), + CollectorEntryValue::KeyValueDeleted { value, len } => { + let expected = &value[..*len as usize]; + assert!( + values.iter().any(|lv| matches!( + lv, + LookupValue::KeyValueDeleted { value } if &**value == expected + )), + "we wrote a key-value tombstone but did not read it back" + ) + } v => { assert!( values.into_iter().any(|lv| {