From d60dc88372c3b75829d0f75732824a408846a062 Mon Sep 17 00:00:00 2001 From: Phil Mataras Date: Wed, 19 Aug 2026 04:11:55 +0000 Subject: [PATCH 1/2] fix(chunks): stop the absolute-offset symlink from vanishing on rewrite The absolute-offset index was written by unlinking the existing symlink and then creating it again. That opens a window in which the entry does not exist. getByAbsoluteOffset reads ENOENT during that window, treats it as a cache miss, and refetches a chunk that is already on disk -- silently, since ENOENT is deliberately not logged there. The refetch then rewrites the same offset, reopening the window. The unlink also made concurrent writers collide. Writers racing on one offset resolve to the same target, so the loser's symlink call failed EEXIST and was logged at error with a stack trace. On a production gateway this was the single hottest error in the log at roughly 36 per second (~128k/hour), which both cost real work to serialise and buried genuine failures. Create the link directly instead. An EEXIST whose target already matches is a no-op rather than an error, which is the overwhelmingly common case and leaves the entry continuously present. A genuinely different target is still replaced -- the "allows updating" case the unlink existed for. Applied to both the chunk data and chunk metadata stores, which carried identical copies of the pattern. Caching behaviour is otherwise unchanged: the index remains best-effort and failures still never prevent a chunk from being cached. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WndVj5cprBtfa5d2qqnacp --- src/store/fs-chunk-data-store.test.ts | 62 +++++++++++++++++++++++++++ src/store/fs-chunk-data-store.ts | 25 ++++++++--- src/store/fs-chunk-metadata-store.ts | 28 ++++++++---- 3 files changed, 102 insertions(+), 13 deletions(-) diff --git a/src/store/fs-chunk-data-store.test.ts b/src/store/fs-chunk-data-store.test.ts index 10a04b6c6..48146ff63 100644 --- a/src/store/fs-chunk-data-store.test.ts +++ b/src/store/fs-chunk-data-store.test.ts @@ -35,6 +35,68 @@ describe('FsChunkDataStore', () => { rmSync(tempDir, { recursive: true, force: true }); }); + describe('absolute offset symlink', () => { + const dataRoot = 'wRq6f05oRupfTW_M5dcYBtwK5P8rSNYu20vC6D_o-M4'; + const relativeOffset = 0; + const absoluteOffset = 388149830525175; + const chunkData: ChunkData = { + chunk: Buffer.from('test chunk data'), + hash: crypto.createHash('sha256').update('test chunk data').digest(), + }; + + const symlinkPath = () => + join( + tempDir, + 'data', + 'by-absolute-offset', + '388', + '149', + absoluteOffset.toString(), + ); + + it('should not unlink when the existing link already points at the target', async () => { + const fsp = (await import('node:fs')).promises; + + // First write establishes the link. + await store.set(dataRoot, relativeOffset, chunkData, absoluteOffset); + const before = await fsp.readlink(symlinkPath()); + + // Unlinking before re-linking would leave a window where the index entry + // does not exist; a concurrent read would see ENOENT and refetch data + // that is already on disk. A rewrite of the same offset must therefore + // leave the link untouched. + let unlinked = false; + const realUnlink = fsp.unlink.bind(fsp); + (fsp as any).unlink = async (p: any) => { + if (String(p).includes('by-absolute-offset')) unlinked = true; + return realUnlink(p); + }; + try { + await store.set(dataRoot, relativeOffset, chunkData, absoluteOffset); + } finally { + (fsp as any).unlink = realUnlink; + } + + assert.equal(unlinked, false, 'must not unlink an already-correct link'); + assert.equal(await fsp.readlink(symlinkPath()), before); + }); + + it('should replace the link when the target genuinely differs', async () => { + const fsp = (await import('node:fs')).promises; + const otherRoot = 'aBq6f05oRupfTW_M5dcYBtwK5P8rSNYu20vC6D_o-M4'; + + await store.set(dataRoot, relativeOffset, chunkData, absoluteOffset); + const first = await fsp.readlink(symlinkPath()); + + // Same absolute offset, different data root: the index must follow it. + await store.set(otherRoot, relativeOffset, chunkData, absoluteOffset); + const second = await fsp.readlink(symlinkPath()); + + assert.notEqual(second, first); + assert.ok(second.includes(otherRoot)); + }); + }); + describe('set', () => { it('should save chunk data to the correct path', async () => { const dataRoot = 'wRq6f05oRupfTW_M5dcYBtwK5P8rSNYu20vC6D_o-M4'; diff --git a/src/store/fs-chunk-data-store.ts b/src/store/fs-chunk-data-store.ts index b90642220..bb0664c2d 100644 --- a/src/store/fs-chunk-data-store.ts +++ b/src/store/fs-chunk-data-store.ts @@ -214,14 +214,29 @@ export class FsChunkDataStore implements ChunkDataStore { this.chunkDataRootPath(dataRoot, relativeOffset), ); - // Remove existing symlink if present (allows updating) + // Link directly rather than unlinking first. Unlinking opens a window in + // which the index entry does not exist: a concurrent read of this offset + // sees ENOENT, treats it as a cache miss, and refetches data that is + // already on disk. Concurrent writers for the same offset resolve to the + // same target, so an EEXIST whose target already matches is a no-op, not + // an error -- which is what made this the hottest error in the log. + // A genuinely different target still gets replaced: that is the + // "allows updating" case the unlink was there for. try { + await fs.promises.symlink(targetPath, symlinkPath); + } catch (error: any) { + if (error.code !== 'EEXIST') { + throw error; + } + const existing = await fs.promises + .readlink(symlinkPath) + .catch(() => undefined); + if (existing === targetPath) { + return; + } await fs.promises.unlink(symlinkPath); - } catch { - // Ignore if doesn't exist + await fs.promises.symlink(targetPath, symlinkPath); } - - await fs.promises.symlink(targetPath, symlinkPath); } catch (error: any) { this.log.error('Failed to create absolute offset symlink', { dataRoot, diff --git a/src/store/fs-chunk-metadata-store.ts b/src/store/fs-chunk-metadata-store.ts index 21fcd3741..33f0fdfd3 100644 --- a/src/store/fs-chunk-metadata-store.ts +++ b/src/store/fs-chunk-metadata-store.ts @@ -196,17 +196,29 @@ export class FsChunkMetadataStore implements ChunkMetadataStore { this.chunkMetadataPath(dataRoot, relativeOffset), ); - // Remove existing symlink if present (allows updating). - // Note: Race condition possible between unlink and symlink if another - // process creates a symlink at the same path. We catch all errors below - // to ensure cache write succeeds - symlink index is best-effort. + // Link directly rather than unlinking first. Unlinking opens a window in + // which the index entry does not exist: a concurrent read of this offset + // sees ENOENT, treats it as a cache miss, and refetches data that is + // already on disk. Concurrent writers for the same offset resolve to the + // same target, so an EEXIST whose target already matches is a no-op, not + // an error -- which is what made this the hottest error in the log. + // A genuinely different target still gets replaced: that is the + // "allows updating" case the unlink was there for. try { + await fs.promises.symlink(targetPath, symlinkPath); + } catch (error: any) { + if (error.code !== 'EEXIST') { + throw error; + } + const existing = await fs.promises + .readlink(symlinkPath) + .catch(() => undefined); + if (existing === targetPath) { + return; + } await fs.promises.unlink(symlinkPath); - } catch { - // Ignore if doesn't exist + await fs.promises.symlink(targetPath, symlinkPath); } - - await fs.promises.symlink(targetPath, symlinkPath); } catch (error: any) { this.log.error('Failed to create absolute offset symlink', { dataRoot, From 0bb9fb9c5fe71767571af74ec000efab28f4690a Mon Sep 17 00:00:00 2001 From: Phil Mataras Date: Wed, 19 Aug 2026 04:18:56 +0000 Subject: [PATCH 2/2] fix(chunks): replace a retargeted offset symlink atomically Addresses CodeRabbit review feedback. The previous commit closed the missing-entry window for the common case but left it open on the retarget path, which still unlinked the live symlink before recreating it. That is the same defect in miniature: a concurrent read landing between the two calls sees ENOENT, treats it as a cache miss, and refetches data that is on disk. Retargeting now writes a uniquely-named temporary symlink beside the index entry and renames it into place. rename() over an existing path is atomic within a filesystem, so a reader always resolves either the previous target or the new one, never nothing. The temporary link is removed if the rename fails, so a failure cannot leave debris in the index directory. The mismatched-target test now asserts that the index path is never unlinked during a retarget, and that no temporary entries survive. Both properties fail against the previous implementation. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WndVj5cprBtfa5d2qqnacp --- src/store/fs-chunk-data-store.test.ts | 28 +++++++++++++++++++++++++-- src/store/fs-chunk-data-store.ts | 27 ++++++++++++++++++++++++-- src/store/fs-chunk-metadata-store.ts | 28 +++++++++++++++++++++++++-- 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/src/store/fs-chunk-data-store.test.ts b/src/store/fs-chunk-data-store.test.ts index 48146ff63..7837e4c79 100644 --- a/src/store/fs-chunk-data-store.test.ts +++ b/src/store/fs-chunk-data-store.test.ts @@ -88,12 +88,36 @@ describe('FsChunkDataStore', () => { await store.set(dataRoot, relativeOffset, chunkData, absoluteOffset); const first = await fsp.readlink(symlinkPath()); - // Same absolute offset, different data root: the index must follow it. - await store.set(otherRoot, relativeOffset, chunkData, absoluteOffset); + // Replacement must be atomic too: unlinking the live path would + // reintroduce the window this change closes, so the retarget has to go + // through rename() and must never unlink the index path itself. + let unlinkedIndexPath = false; + const realUnlink = fsp.unlink.bind(fsp); + (fsp as any).unlink = async (p: any) => { + if (String(p) === symlinkPath()) unlinkedIndexPath = true; + return realUnlink(p); + }; + try { + // Same absolute offset, different data root: the index must follow it. + await store.set(otherRoot, relativeOffset, chunkData, absoluteOffset); + } finally { + (fsp as any).unlink = realUnlink; + } const second = await fsp.readlink(symlinkPath()); + assert.equal( + unlinkedIndexPath, + false, + 'retarget must replace atomically, not unlink the live path', + ); assert.notEqual(second, first); assert.ok(second.includes(otherRoot)); + + // No temporary links left behind. + const dir = await fsp.readdir( + join(tempDir, 'data', 'by-absolute-offset', '388', '149'), + ); + assert.deepEqual(dir, [absoluteOffset.toString()]); }); }); diff --git a/src/store/fs-chunk-data-store.ts b/src/store/fs-chunk-data-store.ts index bb0664c2d..6416c1d03 100644 --- a/src/store/fs-chunk-data-store.ts +++ b/src/store/fs-chunk-data-store.ts @@ -199,6 +199,16 @@ export class FsChunkDataStore implements ChunkDataStore { } } + /** + * Point the absolute-offset index at a chunk, creating or updating the link. + * + * The entry is never momentarily absent: the common case (an existing link + * already pointing at this target) is a no-op, and a genuine retarget is + * applied with an atomic rename. A concurrent reader therefore always + * resolves either the previous target or the new one. The index is + * best-effort -- failures are logged and swallowed so they cannot prevent + * the chunk itself from being cached. + */ private async createAbsoluteOffsetSymlink( dataRoot: string, relativeOffset: number, @@ -234,8 +244,21 @@ export class FsChunkDataStore implements ChunkDataStore { if (existing === targetPath) { return; } - await fs.promises.unlink(symlinkPath); - await fs.promises.symlink(targetPath, symlinkPath); + // Replace atomically. Unlinking first would reintroduce the very + // window this change exists to close: rename() over an existing path + // is atomic within a filesystem, so a concurrent read always sees + // either the old link or the new one, never nothing. The temporary + // name is unique so concurrent replacements cannot collide on it. + const tmpPath = `${symlinkPath}.tmp-${crypto + .randomBytes(8) + .toString('hex')}`; + await fs.promises.symlink(targetPath, tmpPath); + try { + await fs.promises.rename(tmpPath, symlinkPath); + } catch (renameError: any) { + await fs.promises.unlink(tmpPath).catch(() => undefined); + throw renameError; + } } } catch (error: any) { this.log.error('Failed to create absolute offset symlink', { diff --git a/src/store/fs-chunk-metadata-store.ts b/src/store/fs-chunk-metadata-store.ts index 33f0fdfd3..d2857e66e 100644 --- a/src/store/fs-chunk-metadata-store.ts +++ b/src/store/fs-chunk-metadata-store.ts @@ -4,6 +4,7 @@ * * SPDX-License-Identifier: AGPL-3.0-or-later */ +import crypto from 'node:crypto'; import fs from 'node:fs'; import path from 'node:path'; import winston from 'winston'; @@ -181,6 +182,16 @@ export class FsChunkMetadataStore implements ChunkMetadataStore { } } + /** + * Point the absolute-offset index at a chunk, creating or updating the link. + * + * The entry is never momentarily absent: the common case (an existing link + * already pointing at this target) is a no-op, and a genuine retarget is + * applied with an atomic rename. A concurrent reader therefore always + * resolves either the previous target or the new one. The index is + * best-effort -- failures are logged and swallowed so they cannot prevent + * the chunk itself from being cached. + */ private async createAbsoluteOffsetSymlink( dataRoot: string, relativeOffset: number, @@ -216,8 +227,21 @@ export class FsChunkMetadataStore implements ChunkMetadataStore { if (existing === targetPath) { return; } - await fs.promises.unlink(symlinkPath); - await fs.promises.symlink(targetPath, symlinkPath); + // Replace atomically. Unlinking first would reintroduce the very + // window this change exists to close: rename() over an existing path + // is atomic within a filesystem, so a concurrent read always sees + // either the old link or the new one, never nothing. The temporary + // name is unique so concurrent replacements cannot collide on it. + const tmpPath = `${symlinkPath}.tmp-${crypto + .randomBytes(8) + .toString('hex')}`; + await fs.promises.symlink(targetPath, tmpPath); + try { + await fs.promises.rename(tmpPath, symlinkPath); + } catch (renameError: any) { + await fs.promises.unlink(tmpPath).catch(() => undefined); + throw renameError; + } } } catch (error: any) { this.log.error('Failed to create absolute offset symlink', {