From a992ec0b4bf4c4a46caf9ef38b49f0a07010eb6b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 12 Sep 2026 06:45:37 -0400 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=A7=B9=20fix:=20Evict=20Stale=20File-?= =?UTF-8?q?Object=20Index=20Entries=20(#182)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: evict stale file-object index entries Forget cached locators after successful deletion and missing-object downloads so replacement keys resolve immediately. Reuse exact resolver matching in the delete route to avoid prefix collisions. Fixes #181 * fix: keep file-object deletion storage-authoritative * fix: retire superseded upload objects * fix: canonicalize replacement object keys * fix: collapse legacy object-key siblings * fix: recover reads from stale locators * fix: namespace canonical object identities --- service/src/file-object-resolver.test.ts | 143 ++++++++++++++++++++++- service/src/file-object-resolver.ts | 124 +++++++++++++++++--- service/src/file-server.ts | 74 +++++++++--- 3 files changed, 306 insertions(+), 35 deletions(-) diff --git a/service/src/file-object-resolver.test.ts b/service/src/file-object-resolver.test.ts index deeb3265..2342d87a 100644 --- a/service/src/file-object-resolver.test.ts +++ b/service/src/file-object-resolver.test.ts @@ -1,8 +1,27 @@ import { describe, expect, test } from 'bun:test'; -import { FileObjectResolver, mapObjectDetails } from './file-object-resolver'; +import { canonicalObjectId, FileObjectResolver, mapObjectDetails, storageKeyForUpload } from './file-object-resolver'; import type { BucketItemStat } from 'minio'; describe('storage object resolution', () => { + test('replacement uploads converge on one stable object key', () => { + expect(storageKeyForUpload('s', 'id', '.csv', true)).toBe('s/.codeapi-objects/aWQ'); + expect(storageKeyForUpload('s', 'id', '.pdf', true)).toBe('s/.codeapi-objects/aWQ'); + expect(canonicalObjectId(storageKeyForUpload('s', 'report.csv', '', true))).toBe('report.csv'); + expect(storageKeyForUpload('s', 'generated', '.csv', false)).toBe('s/generated.csv'); + }); + + test('canonical dotted identities cannot match another legacy identity', async () => { + const dottedKey = storageKeyForUpload('s', 'report.csv', '', true); + const resolver = new FileObjectResolver({ + bucket: 'files', + list: async function* () { yield { name: dottedKey }; }, + stat: async () => ({ metaData: {} } as BucketItemStat), + }); + + expect(await resolver.listFresh('s', 'report.csv')).toEqual([dottedKey]); + expect(await resolver.listFresh('s', 'report')).toEqual([]); + }); + test('indexes exact identities while reading fresh version metadata on every request', async () => { const index = new Map(); let lists = 0; @@ -33,6 +52,128 @@ describe('storage object resolution', () => { present = true; expect(await resolver.resolve('s', 'id')).toBe('s/id.txt'); }); + + test('falls back to storage when the advisory index is unavailable', async () => { + const failures: string[] = []; + const resolver = new FileObjectResolver({ + bucket: 'files', + list: async function* () { yield { name: 's/id.txt' }; }, + stat: async () => ({ metaData: {} } as BucketItemStat), + onIndexError: operation => failures.push(operation), + index: { + get: async () => { throw new Error('redis unavailable'); }, + set: async () => { throw new Error('redis unavailable'); }, + forget: async () => { throw new Error('redis unavailable'); }, + }, + }); + + expect(await resolver.resolve('s', 'id')).toBe('s/id.txt'); + expect(await resolver.listFresh('s', 'id')).toEqual(['s/id.txt']); + expect(failures).toEqual(['get', 'set']); + }); + + test('fresh listing ignores a stale locator and returns every exact sibling', async () => { + const index = new Map([['locator', 's/id.txt']]); + let lists = 0; + const resolver = new FileObjectResolver({ + bucket: 'files', + list: async function* () { + lists++; + yield { name: 's/identifier.txt' }; + yield { name: 's/id.csv' }; + yield { name: 's/id.pdf' }; + }, + stat: async () => ({ metaData: {} } as BucketItemStat), + index: { + get: async () => index.get('locator') ?? null, + set: async (_key, value) => index.set('locator', value), + forget: async (_key, value) => { if (index.get('locator') === value) index.delete('locator'); }, + }, + }); + + expect(await resolver.listFresh('s', 'id')).toEqual(['s/id.csv', 's/id.pdf']); + expect(index.get('locator')).toBe('s/id.txt'); + expect(lists).toBe(2); + }); + + test('metadata re-resolves storage after a cached locator is missing', async () => { + const index = new Map([['locator', 's/id.txt']]); + let heads = 0; + const resolver = new FileObjectResolver({ + bucket: 'files', + list: async function* () { yield { name: 's/.codeapi-objects/aWQ' }; }, + stat: async key => { + heads++; + if (key === 's/id.txt') throw Object.assign(new Error('missing'), { code: 'NoSuchKey' }); + return { + size: 1, + etag: 'current', + lastModified: new Date(), + metaData: { 'codeapi-version': 'current' }, + } as BucketItemStat; + }, + index: { + get: async () => index.get('locator') ?? null, + set: async (_key, value) => index.set('locator', value), + forget: async (_key, value) => { if (index.get('locator') === value) index.delete('locator'); }, + }, + }); + + const metadata = await resolver.metadata('s', 'id'); + expect(metadata?.key).toBe('s/.codeapi-objects/aWQ'); + expect(metadata?.stat.metaData['codeapi-version']).toBe('current'); + expect(index.get('locator')).toBe('s/.codeapi-objects/aWQ'); + expect(heads).toBe(2); + }); + + test('forgets a deleted locator so a replacement key for the same identity resolves', async () => { + const index = new Map(); + const stored = new Set(['s/id.txt']); + const resolver = new FileObjectResolver({ + bucket: 'files', + list: async function* (prefix) { for (const name of stored) if (name.startsWith(prefix)) yield { name }; }, + stat: async () => ({ metaData: {} } as BucketItemStat), + index: { + get: async k => index.get(k) ?? null, + set: async (k, v, replace) => { if (replace || !index.has(k)) index.set(k, v); }, + forget: async (k, v) => { if (index.get(k) === v) index.delete(k); }, + }, + }); + + expect(await resolver.resolve('s', 'id')).toBe('s/id.txt'); + stored.delete('s/id.txt'); + await resolver.forget('s', 'id', 's/id.txt'); + expect(index.size).toBe(0); + + stored.add('s/id.csv'); + expect(await resolver.resolve('s', 'id')).toBe('s/id.csv'); + }); + + test('eviction is scoped to the identity and never drops a newer cached key', async () => { + const index = new Map(); + let lists = 0; + const resolver = new FileObjectResolver({ + bucket: 'files', + list: async function* () { lists++; yield { name: 's/id.txt' }; }, + stat: async () => ({ metaData: {} } as BucketItemStat), + index: { + get: async k => index.get(k) ?? null, + set: async (k, v, replace) => { if (replace || !index.has(k)) index.set(k, v); }, + forget: async (k, v) => { if (index.get(k) === v) index.delete(k); }, + }, + }); + + expect(await resolver.resolve('s', 'id')).toBe('s/id.txt'); + // A concurrent upload republished the identity before the delete evicted it. + await resolver.remember('s', 'id', 's/id.csv'); + await resolver.forget('s', 'id', 's/id.txt'); + // Keys outside the identity can never reach its entry. + await resolver.forget('s', 'id', 's2/id.txt'); + await resolver.forget('s', 'id', 's/other.txt'); + + expect(await resolver.resolve('s', 'id')).toBe('s/id.csv'); + expect(lists).toBe(1); + }); }); test('metadata listing stays bounded and ordered across 240 objects', async () => { diff --git a/service/src/file-object-resolver.ts b/service/src/file-object-resolver.ts index a41e0dfc..bc4b4ce5 100644 --- a/service/src/file-object-resolver.ts +++ b/service/src/file-object-resolver.ts @@ -2,10 +2,28 @@ import { createHash } from 'node:crypto'; import path from 'node:path'; import type { BucketItemStat } from 'minio'; +const CANONICAL_OBJECT_DIRECTORY = '.codeapi-objects'; + +export function canonicalObjectKey(session: string, id: string): string { + return `${session}/${CANONICAL_OBJECT_DIRECTORY}/${Buffer.from(id, 'utf8').toString('base64url')}`; +} + +export function canonicalObjectId(key: string): string | undefined { + const parts = key.split('/'); + if (parts.length !== 3 || parts[1] !== CANONICAL_OBJECT_DIRECTORY || parts[2] === '') return undefined; + try { + const id = Buffer.from(parts[2], 'base64url').toString('utf8'); + return canonicalObjectKey(parts[0], id) === key ? id : undefined; + } catch { + return undefined; + } +} + export interface ObjectResolverDependencies { bucket: string; list(prefix: string): AsyncIterable<{ name?: string }>; stat(key: string): Promise; + onIndexError?(operation: 'get' | 'set' | 'forget', error: unknown): void; index?: { get(key: string): Promise; set(key: string, value: string, replace: boolean): Promise; @@ -13,48 +31,122 @@ export interface ObjectResolverDependencies { }; } +/** Caller-supplied identities keep one stable storage key across replacement + * filenames. This gives concurrent PUTs one last-writer-wins S3 object without + * requiring a distributed lock or leaving extension-keyed siblings behind. */ +export function storageKeyForUpload( + session: string, + id: string, + extension: string, + replacing: boolean, +): string { + return replacing ? canonicalObjectKey(session, id) : `${session}/${id}${extension}`; +} + /** Storage-key index is a hint, never metadata or authorization. A fresh HEAD * proves existence and supplies the current version even on index/cache hits. */ export class FileObjectResolver { constructor(private readonly deps: ObjectResolverDependencies) {} + private reportIndexError(operation: 'get' | 'set' | 'forget', error: unknown): void { + this.deps.onIndexError?.(operation, error); + } + private indexKey(session: string, id: string): string { return `codeapi:file-key:${createHash('sha256').update(JSON.stringify([this.deps.bucket, session, id])).digest('hex')}`; } private matches(key: string, session: string, id: string): boolean { + if (key === canonicalObjectKey(session, id)) return true; return path.posix.dirname(key) === session && (path.posix.basename(key) === id || path.posix.basename(key, path.posix.extname(key)) === id); } async remember(session: string, id: string, key: string, replace = true): Promise { if (!this.matches(key, session, id)) throw new Error('Object key does not match storage identity'); - await this.deps.index?.set(this.indexKey(session, id), key, replace); + try { + await this.deps.index?.set(this.indexKey(session, id), key, replace); + } catch (error) { + this.reportIndexError('set', error); + } } - async resolve(session: string, id: string): Promise { - const cached = await this.deps.index?.get(this.indexKey(session, id)); - if (cached && this.matches(cached, session, id)) return cached; - for await (const object of this.deps.list(`${session}/${id}`)) { - if (object.name && this.matches(object.name, session, id)) { - await this.remember(session, id, object.name, false); - return object.name; + /** Evict a cached locator once its object is known to be gone. Scoped to the + * requested identity, and conditional on the stored value so a replacement + * key published concurrently for the same identity is never dropped. */ + async forget(session: string, id: string, key: string): Promise { + if (!this.matches(key, session, id)) return; + try { + await this.deps.index?.forget(this.indexKey(session, id), key); + } catch (error) { + this.reportIndexError('forget', error); + } + } + + private async cached(session: string, id: string): Promise { + try { + const key = await this.deps.index?.get(this.indexKey(session, id)); + return key && this.matches(key, session, id) ? key : undefined; + } catch (error) { + this.reportIndexError('get', error); + return undefined; + } + } + + private async findInStorage(session: string, id: string, replaceIndex: boolean): Promise { + for (const prefix of [canonicalObjectKey(session, id), `${session}/${id}`]) { + for await (const object of this.deps.list(prefix)) { + if (object.name && this.matches(object.name, session, id)) { + await this.remember(session, id, object.name, replaceIndex); + return object.name; + } } } return undefined; } + /** List every exact storage key for an identity without consulting its + * locator. Used to collapse legacy siblings and delete the whole identity. */ + async listFresh(session: string, id: string): Promise { + const keys = new Set(); + for (const prefix of [canonicalObjectKey(session, id), `${session}/${id}`]) { + for await (const object of this.deps.list(prefix)) { + if (object.name && this.matches(object.name, session, id)) keys.add(object.name); + } + } + return [...keys]; + } + + async resolve(session: string, id: string): Promise { + return await this.cached(session, id) ?? await this.findInStorage(session, id, false); + } + + /** Recover once a cached key is proven missing. Eviction and publication are + * advisory; the authoritative storage listing determines the replacement. */ + async recover(session: string, id: string, missingKey: string): Promise { + await this.forget(session, id, missingKey); + const [current] = await this.listFresh(session, id); + if (current) await this.remember(session, id, current); + return current; + } + async metadata(session: string, id: string): Promise<{ key: string; stat: BucketItemStat } | undefined> { - const key = await this.resolve(session, id); + let key = await this.resolve(session, id); if (!key) return undefined; - try { - return { key, stat: await this.deps.stat(key) }; - } catch (error) { - if (!['NoSuchKey', 'NotFound', 'NoSuchObject'].includes((error as { code?: string }).code ?? '')) throw error; - await this.deps.index?.forget(this.indexKey(session, id), key); - // Do not cache absence: a later upload can publish this identity again. - return undefined; + for (let attempt = 0; attempt < 2; attempt++) { + try { + return { key, stat: await this.deps.stat(key) }; + } catch (error) { + if (!['NoSuchKey', 'NotFound', 'NoSuchObject'].includes((error as { code?: string }).code ?? '')) throw error; + if (attempt === 1) { + await this.forget(session, id, key); + return undefined; + } + key = await this.recover(session, id, key); + if (!key) return undefined; + } } + return undefined; } } diff --git a/service/src/file-server.ts b/service/src/file-server.ts index 9b326ca2..3dfa0a10 100644 --- a/service/src/file-server.ts +++ b/service/src/file-server.ts @@ -1,8 +1,7 @@ import b from 'busboy'; import { randomUUID } from 'node:crypto'; -import { mapObjectDetails } from './file-object-resolver'; +import { canonicalObjectId, FileObjectResolver, mapObjectDetails, storageKeyForUpload } from './file-object-resolver'; import { sendFileDownload } from './file-download'; -import { FileObjectResolver } from './file-object-resolver'; import path from 'path'; import IORedis from 'ioredis'; import express from 'express'; @@ -151,6 +150,7 @@ const objectResolver = new FileObjectResolver({ bucket: bucketName, list: prefix => minioClient.listObjects(bucketName, prefix, true), stat: key => minioClient.statObject(bucketName, key), + onIndexError: (operation, error) => logger.warn('File-object index operation failed', { operation, error }), ...(env.FILE_OBJECT_INDEX_ENABLED ? { index: { get: (key: string) => redisClient.get(key), set: (key: string, value: string, replace: boolean) => replace @@ -162,6 +162,16 @@ const objectResolver = new FileObjectResolver({ } } : {}), }); +/** Index eviction is best effort: the index is only a hint, so a Redis failure + * must never turn a completed delete or a missing-object 404 into a 500. */ +async function forgetObjectKey(session_id: string, objectId: string, objectName: string): Promise { + try { + await objectResolver.forget(session_id, objectId, objectName); + } catch (error) { + logger.warn('Failed to evict file-object index entry', { error, session_id, objectId, objectName }); + } +} + const minioRegion = process.env.MINIO_REGION ?? process.env.AWS_REGION ?? 'us-east-1'; async function ensureBucketExists(retries = 10, delay = 1000): Promise { @@ -257,7 +267,14 @@ async function uploadFile( ): Promise { const fileId = existingFileId ?? nanoid(); const fileExtension = path.extname(filename); - const objectName = `${session_id}/${fileId}${fileExtension}`; + // Caller-supplied identities use one canonical key, so concurrent writers + // converge on S3's last-writer semantics regardless of filename extension. + const objectName = storageKeyForUpload( + session_id, + fileId, + fileExtension, + existingFileId != null, + ); const encodedFilename = Buffer.from(filename).toString('base64'); @@ -287,6 +304,15 @@ async function uploadFile( } else { await minioClient.putObject(bucketName, objectName, peeked.body, undefined, metaData); } + if (existingFileId != null) { + // Retire every extension-keyed sibling left by older replacement behavior. + // Concurrent replacement writers share objectName and never delete it. + for (const sibling of await objectResolver.listFresh(session_id, fileId)) { + if (sibling === objectName) continue; + await minioClient.removeObject(bucketName, sibling); + await objectResolver.forget(session_id, fileId, sibling); + } + } await objectResolver.remember(session_id, fileId, objectName); logger.info(`[${INSTANCE_ID}] File ID: ${fileId} | Filename: ${filename} | Session key: ${sessionKey}`); await redisClient.set(`upload:${sessionKey}${session_id}${fileId}`, 'true', 'EX', env.SESSION_CACHE_TTL); @@ -490,11 +516,21 @@ app.get('/sessions/:session_id/objects/:objectId/metadata', async (req, res) => app.get('/sessions/:session_id/objects/:objectId', async (req, res) => { const { session_id, objectId } = req.params; + let objectName: string | undefined; try { - const objectName = await objectResolver.resolve(session_id, objectId); + objectName = await objectResolver.resolve(session_id, objectId); if (!objectName) return res.status(404).json({ error: 'File not found' }); - const dataStream = await minioClient.getObject(bucketName, objectName); + let dataStream: Readable; + try { + dataStream = await minioClient.getObject(bucketName, objectName); + } catch (error) { + const missing = ['NoSuchKey', 'NotFound', 'NoSuchObject'].includes((error as { code?: string }).code ?? ''); + if (!missing) throw error; + objectName = await objectResolver.recover(session_id, objectId, objectName); + if (!objectName) return res.status(404).json({ error: 'File not found' }); + dataStream = await minioClient.getObject(bucketName, objectName); + } try { const headers = (dataStream as Readable & { headers?: Record }).headers ?? {}; if (!headers['x-amz-meta-codeapi-version'] || !headers['x-amz-meta-original-filename']) { @@ -515,8 +551,11 @@ app.get('/sessions/:session_id/objects/:objectId', async (req, res) => { } } catch (err) { logger.error('Error downloading file', { error: err, session_id, objectId }); + const missing = ['NoSuchKey', 'NotFound', 'NoSuchObject'].includes((err as { code?: string }).code ?? ''); + // A locator that no longer names bytes must not shadow a replacement object + // published for the same identity until the index TTL expires. + if (missing && objectName) await forgetObjectKey(session_id, objectId, objectName); if (!res.headersSent && !res.destroyed) { - const missing = ['NoSuchKey', 'NotFound', 'NoSuchObject'].includes((err as { code?: string }).code ?? ''); return res.status(missing ? 404 : 500).json({ error: 'Error downloading file' }); } } @@ -527,6 +566,10 @@ app.get('/sessions/:session_id/objects/:objectId', async (req, res) => { */ function parseObjectName(objectName: string | undefined): { session_id: string; file_id: string } | null { if (objectName == null || objectName === '') return null; + const canonicalId = canonicalObjectId(objectName); + if (canonicalId != null) { + return { session_id: objectName.split('/', 1)[0], file_id: canonicalId }; + } const parts = objectName.split('/'); if (parts.length < 2) return null; const session_id = parts[0]; @@ -605,17 +648,9 @@ app.delete('/sessions/:session_id/objects/:fileId', async (req, res) => { const { session_id, fileId } = req.params; try { - const stream = minioClient.listObjects(bucketName, `${session_id}/${fileId}`, true); - let objectName = ''; + const objectNames = await objectResolver.listFresh(session_id, fileId); - for await (const obj of stream) { - if (obj.name.startsWith(`${session_id}/${fileId}`) === true) { - objectName = obj.name; - break; - } - } - - if (!objectName) { + if (objectNames.length === 0) { logger.warn('File not found for deletion', { session_id, fileId, bucketName }); return res.status(404).json({ error: 'File not found', @@ -626,8 +661,11 @@ app.delete('/sessions/:session_id/objects/:fileId', async (req, res) => { }); } - await minioClient.removeObject(bucketName, objectName); - logger.info(`[${INSTANCE_ID}] File deleted successfully: ${objectName}`); + for (const objectName of objectNames) { + await minioClient.removeObject(bucketName, objectName); + await forgetObjectKey(session_id, fileId, objectName); + } + logger.info(`[${INSTANCE_ID}] File identity deleted successfully`, { session_id, fileId, objectNames }); return res.status(200).json({ message: 'File deleted successfully', session_id, From c3fd558195c8c4513977b83963c6aad495a4874d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 12 Sep 2026 06:45:48 -0400 Subject: [PATCH 2/4] perf: enable authorized input reuse by default (#183) --- api/src/config-defaults.test.ts | 21 +++++++++++++++++++++ api/src/config.ts | 3 ++- docs/INPUT_REUSE.md | 8 ++++---- helm/codeapi/values.yaml | 2 +- 4 files changed, 28 insertions(+), 6 deletions(-) create mode 100644 api/src/config-defaults.test.ts diff --git a/api/src/config-defaults.test.ts b/api/src/config-defaults.test.ts new file mode 100644 index 00000000..281f5679 --- /dev/null +++ b/api/src/config-defaults.test.ts @@ -0,0 +1,21 @@ +import { expect, test } from 'bun:test'; +import path from 'node:path'; + +function readHttpInputCacheDefault(value?: string): boolean { + const env = { ...process.env }; + if (value === undefined) delete env.SANDBOX_HTTP_INPUT_CACHE_ENABLED; + else env.SANDBOX_HTTP_INPUT_CACHE_ENABLED = value; + const script = [ + `const { config } = await import(${JSON.stringify(path.resolve(process.cwd(), 'src/config.ts'))})`, + 'process.stdout.write(JSON.stringify(config.http_input_cache_enabled))', + ].join(';'); + const result = Bun.spawnSync({ cmd: [process.execPath, '--eval', script], env }); + expect(result.exitCode).toBe(0); + return JSON.parse(result.stdout.toString()) as boolean; +} + +test('authorized HTTP input reuse defaults on and retains an explicit rollback switch', () => { + expect(readHttpInputCacheDefault()).toBe(true); + expect(readHttpInputCacheDefault('false')).toBe(false); + expect(readHttpInputCacheDefault('true')).toBe(true); +}); diff --git a/api/src/config.ts b/api/src/config.ts index f3b935ff..a5d0c993 100644 --- a/api/src/config.ts +++ b/api/src/config.ts @@ -122,7 +122,8 @@ export const config = { /* Ceiling for the pushed input cache (session-inputs.ts). Eviction is * always safe — a miss simply re-pushes on the next probe — so this is a * disk guard, not a correctness knob. */ - http_input_cache_enabled: process.env.SANDBOX_HTTP_INPUT_CACHE_ENABLED === 'true', + http_input_cache_enabled: + (process.env.SANDBOX_HTTP_INPUT_CACHE_ENABLED ?? 'true') === 'true', http_input_cache_max_objects: safeInt(process.env.SANDBOX_HTTP_INPUT_CACHE_MAX_OBJECTS, 4096), http_input_cache_max_inflight: safeInt(process.env.SANDBOX_HTTP_INPUT_CACHE_MAX_INFLIGHT, 16), input_cache_max_bytes: safeInt( diff --git a/docs/INPUT_REUSE.md b/docs/INPUT_REUSE.md index 40e277c4..f87afa89 100644 --- a/docs/INPUT_REUSE.md +++ b/docs/INPUT_REUSE.md @@ -48,7 +48,7 @@ sequenceDiagram | `egressGrant.inputManifestTimeoutMs` | `CODEAPI_INPUT_MANIFEST_TIMEOUT_MS` | `10000` | | `fileServer.objectIndexEnabled` | `CODEAPI_FILE_OBJECT_INDEX_ENABLED` | `false` | | `fileServer.metadataConcurrency` | `CODEAPI_FILE_METADATA_CONCURRENCY` | `1` | -| `workerSandbox.sandbox.httpInputCacheEnabled` | `SANDBOX_HTTP_INPUT_CACHE_ENABLED` | `false` | +| `workerSandbox.sandbox.httpInputCacheEnabled` | `SANDBOX_HTTP_INPUT_CACHE_ENABLED` | `true` | | `workerSandbox.sandbox.httpInputCacheMaxInflight` | `SANDBOX_HTTP_INPUT_CACHE_MAX_INFLIGHT` | `16` | | `workerSandbox.sandbox.httpInputCacheMaxObjects` | `SANDBOX_HTTP_INPUT_CACHE_MAX_OBJECTS` | `4096` | | `workerSandbox.sandbox.inputCacheMaxBytes` | `SANDBOX_INPUT_CACHE_MAX_BYTES` | `536870912` | @@ -61,14 +61,14 @@ Metadata listing concurrency preserves order and is capped at 64. A canary can u ## Rollout and rollback -1. Deploy the new binaries with feature flags off. Atomic accounting supports existing JSON ledgers, and legacy downloads retain metadata compatibility. The new file server stamps future uploads with versions. +1. Deploy the new binaries with HTTP input reuse enabled by default. Mixed-version requests remain compatible: older gateways and relays fall back to normal downloads, while older unversioned objects return `cacheable: false`. The new file server stamps future uploads with versions. Set `workerSandbox.sandbox.httpInputCacheEnabled=false` only when a staged rollout requires the immediate rollback path. 2. Update **all** egress-gateway replicas before enabling compact ledgers. New binaries read both formats regardless of the creation flag. Older binaries cannot read compact hashes. To roll back to an older binary, disable compact creation, drain active grants, and wait their maximum TTL plus grace; never delete active ledgers to force a rollback. 3. Update all file-server writers before enabling the object-key index. Otherwise an older writer can change a locator without updating the index. Keep file-server replicas consistent during an indexed rollout. -4. Update the gateway, relay, runner, and launcher before enabling HTTP reuse on a small runner canary. Older gateway/relay metadata routes return 404/405 and fall back safely. Older files return `cacheable: false`. Keep the feature disabled for storage adapters that cannot return user metadata on GET. +4. Canary the default-on HTTP reuse path after updating the gateway, relay, runner, and launcher. Keep the feature explicitly disabled for storage adapters that cannot return user metadata on GET. 5. Observe `codeapi_sandbox_http_input_cache_events_total` (bounded event labels, no identities), cold and warm preparation latency, storage/Redis operations, admission fairness, request budgets, and memory/disk pressure before widening the rollout. A successful manifest consumes one read request for the batch, matching the existing list-request accounting unit. Per-file compatibility preflights each consume a read request; each cold miss consumes an additional download request. Do not disable budget enforcement to accommodate a workload. 6. Disable HTTP reuse to return to normal downloads immediately. Cached files can age out normally; no workspace deletion or migration is needed. -The creation flags default off. No deployment or object retention policy is changed by this code. Command grouping and persistent sessions remain independent options, not prerequisites for content reuse. Nothing deletes user inputs or infers shell dependencies. +HTTP input reuse defaults on. Compact-ledger creation and the object-key index remain off until their mixed-version rollout requirements are satisfied. No object retention policy is changed by this code. Command grouping and persistent sessions remain independent options, not prerequisites for content reuse. Nothing deletes user inputs or infers shell dependencies. ## Validation diff --git a/helm/codeapi/values.yaml b/helm/codeapi/values.yaml index 3ecfbef8..bafed305 100644 --- a/helm/codeapi/values.yaml +++ b/helm/codeapi/values.yaml @@ -273,7 +273,7 @@ workerSandbox: # Defaults to maxConcurrentJobs when unset. jobUidCount: null workspaceReaperMaxAgeSeconds: 3600 - httpInputCacheEnabled: false + httpInputCacheEnabled: true httpInputCacheMaxInflight: 16 httpInputCacheMaxObjects: 4096 inputCacheMaxBytes: 536870912 From 76129e15e5f5a62d193074524459c80708c0d741 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 12 Sep 2026 06:58:40 -0400 Subject: [PATCH 3/4] fix: honor requested input destinations (#184) --- api/openapi.yaml | 1 + api/src/download.test.ts | 111 +++++++++-------------- api/src/job-helpers.test.ts | 169 +++--------------------------------- api/src/job.ts | 86 ++++-------------- 4 files changed, 67 insertions(+), 300 deletions(-) diff --git a/api/openapi.yaml b/api/openapi.yaml index e4c04c14..23e1224c 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -67,6 +67,7 @@ components: properties: name: type: string + description: Relative path where the file is mounted in the sandbox. id: type: string content: diff --git a/api/src/download.test.ts b/api/src/download.test.ts index da877dad..d0b6ed6e 100644 --- a/api/src/download.test.ts +++ b/api/src/download.test.ts @@ -18,12 +18,8 @@ import { /** * Integration tests for `Job.downloadAndWriteFile` against a real HTTP - * listener. These exercise the cross-repo round-trip: the file-server - * (codeapi/service) emits `Content-Disposition: attachment; - * filename*=UTF-8''` for nested artifacts, and the - * sandbox-side parser must recover the path so the file lands at the same - * nested location on the next prime(). Hitting a real listener (not a - * mocked Response) catches anything fetch-level that a unit test would miss. + * listener. Hitting a real listener verifies that response metadata cannot + * redirect a caller-validated sandbox destination. */ interface DownloadInternals { @@ -176,21 +172,16 @@ afterEach(async () => { await fsp.rm(tmpDir, { recursive: true, force: true }); }); -describe('downloadAndWriteFile / RFC 5987 round-trip', () => { - it('writes a nested-path artifact at the encoded location', async () => { - /* Simulates the matplotlib-bug shape: codeapi previously returned a - * flat `file.name` and the original path was carried only by the - * server's `filename*=` header. The fix is: parser recovers the path, - * `mkdir { recursive: true }` creates the parent dir, file ends up - * where the user expects to `cat` it on the next turn. */ +describe('downloadAndWriteFile destinations', () => { + it('writes a nested-path artifact at the requested location', async () => { const file: TFile = { id: 'nested-id', storage_session_id: 'prev-session', - name: 'flat-fallback.txt', + name: 'proj/notes.txt', }; routes.set(`/sessions/${encodeURIComponent(file.storage_session_id!)}/objects/${encodeURIComponent(file.id!)}`, { status: 200, - contentDisposition: "attachment; filename*=UTF-8''proj%2Fnotes.txt", + contentDisposition: "attachment; filename*=UTF-8''stored-original.txt", body: 'hello from a nested artifact\n', }); @@ -212,7 +203,7 @@ describe('downloadAndWriteFile / RFC 5987 round-trip', () => { const file: TFile = { id: 'opaque-object-handle', storage_session_id: 'opaque-session-handle', - name: 'gateway-fallback.txt', + name: 'gateway.txt', }; let sawGrantHeader = false; let sawRelayToken = false; @@ -275,14 +266,11 @@ describe('downloadAndWriteFile / RFC 5987 round-trip', () => { expect((await fsp.stat(path.join(tmpDir, 'readonly.txt'))).mode & 0o777).toBe(SANDBOX_READONLY_FILE_MODE); }); - it('falls back to the legacy filename= form when filename*= is absent', async () => { - /* Backwards-compat: older file-servers (or proxies that strip RFC - * 5987 extended-form headers) still send the legacy quoted form. The - * parser must still find a name and write the file. */ + it('downloads when a legacy filename header matches the requested name', async () => { const file: TFile = { id: 'legacy-id', storage_session_id: 'prev-session', - name: 'ignored.txt', + name: 'legacy.txt', }; routes.set(`/sessions/${encodeURIComponent(file.storage_session_id!)}/objects/${encodeURIComponent(file.id!)}`, { status: 200, @@ -323,7 +311,7 @@ describe('downloadAndWriteFile / RFC 5987 round-trip', () => { expect(await fsp.stat(path.join(tmpDir, 'opaque-storage-id.xlsx')).catch(() => null)).toBeNull(); }); - it('resolves concurrent header destinations without provisional-name false conflicts', async () => { + it('keeps concurrent inputs at their requested destinations', async () => { const renamed: TFile = { id: 'renamed-id', storage_session_id: 'prev-session', @@ -338,8 +326,6 @@ describe('downloadAndWriteFile / RFC 5987 round-trip', () => { status: 200, contentDisposition: 'attachment; filename="actual.txt"', body: 'renamed bytes', - /* Make the other ref resolve `vacated.txt` while this ref's requested - * name would still be provisional under the old reservation scheme. */ delayMs: 75, }); routes.set(`/sessions/${encodeURIComponent(replacement.storage_session_id!)}/objects/${encodeURIComponent(replacement.id!)}`, { @@ -356,75 +342,62 @@ describe('downloadAndWriteFile / RFC 5987 round-trip', () => { await job.prime(); const submissionDir = asInternals(job).submissionDir; expect(await fsp.readFile(path.join(submissionDir, 'actual.txt'), 'utf8')) - .toBe('renamed bytes'); - expect(await fsp.readFile(path.join(submissionDir, 'vacated.txt'), 'utf8')) .toBe('replacement bytes'); + expect(await fsp.readFile(path.join(submissionDir, 'vacated.txt'), 'utf8')) + .toBe('renamed bytes'); } finally { config.prime_concurrency = originalPrimeConcurrency; await job.cleanup(); } }); - it('rejects concurrent refs that resolve to the same destination before either can overwrite', async () => { - const slower: TFile = { - id: 'same-slower-id', + it('keeps distinct requested names when stored objects share an original filename', async () => { + const original: TFile = { + id: 'original-id', storage_session_id: 'prev-session', - name: 'slower-fallback.txt', + name: 'data.xlsx', }; - const faster: TFile = { - id: 'same-faster-id', + const aliased: TFile = { + id: 'aliased-id', storage_session_id: 'prev-session', - name: 'faster-fallback.txt', + name: 'data-3f9a2c.xlsx', }; - routes.set(`/sessions/${encodeURIComponent(slower.storage_session_id!)}/objects/${encodeURIComponent(slower.id!)}`, { + routes.set(`/sessions/${encodeURIComponent(original.storage_session_id!)}/objects/${encodeURIComponent(original.id!)}`, { status: 200, - contentDisposition: 'attachment; filename="same.txt"', - body: 'slower bytes', + contentDisposition: 'attachment; filename="data.xlsx"', + body: 'original bytes', delayMs: 75, }); - routes.set(`/sessions/${encodeURIComponent(faster.storage_session_id!)}/objects/${encodeURIComponent(faster.id!)}`, { + routes.set(`/sessions/${encodeURIComponent(aliased.storage_session_id!)}/objects/${encodeURIComponent(aliased.id!)}`, { status: 200, - contentDisposition: 'attachment; filename="same.txt"', - body: 'faster bytes', + contentDisposition: 'attachment; filename="data.xlsx"', + body: 'aliased bytes', }); - let dirty = false; const job = makeJob( - [slower, faster], - sessionWorkspaceAt(tmpDir, 'rt_concurrent_same_destination', () => { dirty = true; }), + [original, aliased], + sessionWorkspaceAt(tmpDir, 'rt_shared_original_filename'), ); const originalPrimeConcurrency = config.prime_concurrency; config.prime_concurrency = 2; - let deadlockTimer: ReturnType | undefined; try { - const outcome = await Promise.race([ - job.prime().then( - () => ({ status: 'fulfilled' as const }), - error => ({ status: 'rejected' as const, error }), - ), - new Promise<{ status: 'timeout' }>(resolve => { - deadlockTimer = setTimeout(() => resolve({ status: 'timeout' }), 2_000); - }), - ]); - if (deadlockTimer) clearTimeout(deadlockTimer); - expect(outcome.status).toBe('rejected'); - if (outcome.status === 'rejected') { - expect(outcome.error).toBeInstanceOf(SessionWorkspaceDirtyError); - } - expect(dirty).toBe(true); - expect(await fsp.readFile(path.join(tmpDir, 'same.txt'), 'utf8')).toBe('faster bytes'); + await job.prime(); + const submissionDir = asInternals(job).submissionDir; + expect(await fsp.readFile(path.join(submissionDir, 'data.xlsx'), 'utf8')) + .toBe('original bytes'); + expect(await fsp.readFile(path.join(submissionDir, 'data-3f9a2c.xlsx'), 'utf8')) + .toBe('aliased bytes'); } finally { - if (deadlockTimer) clearTimeout(deadlockTimer); config.prime_concurrency = originalPrimeConcurrency; await job.cleanup(); } }); - it('decodes UTF-8 percent-encoded names with non-ASCII characters', async () => { + it('keeps a Unicode requested name when the header is percent encoded', async () => { const file: TFile = { id: 'utf8-id', storage_session_id: 'prev-session', - name: 'ignored.txt', + name: '你好.txt', }; routes.set(`/sessions/${encodeURIComponent(file.storage_session_id!)}/objects/${encodeURIComponent(file.id!)}`, { status: 200, @@ -645,11 +618,7 @@ describe('downloadAndWriteFile / RFC 5987 round-trip', () => { await expect(fsp.access(path.join(tmpDir, 'should-not-exist.txt'))).rejects.toThrow(); }); - it('rejects a server-supplied filename that escapes the submission dir', async () => { - /* Companion guarantee for the path-preserving sanitizer on the - * LibreChat side: if a malicious / misconfigured server tries to - * smuggle a `..` traversal via Content-Disposition, the codeapi-side - * `validateFilePath` aborts before any write happens. */ + it('ignores a server-supplied filename that escapes the submission dir', async () => { const file: TFile = { id: 'evil-id', storage_session_id: 'prev-session', @@ -658,16 +627,14 @@ describe('downloadAndWriteFile / RFC 5987 round-trip', () => { routes.set(`/sessions/${encodeURIComponent(file.storage_session_id!)}/objects/${encodeURIComponent(file.id!)}`, { status: 200, contentDisposition: "attachment; filename*=UTF-8''..%2F..%2Fescape.txt", - body: 'should never be written', + body: 'safe bytes', }); const job = makeJob([file]); asInternals(job).submissionDir = tmpDir; - /* downloadAndWriteFile rethrows ValidationError fast (no retries) so - * `expect(...).rejects` is the right assertion. */ - await expect(job.downloadAndWriteFile(file)).rejects.toThrow(); - /* Defensive: nothing escaped to a parent dir. */ + await expect(job.downloadAndWriteFile(file)).resolves.toBe('innocent.txt'); + expect(await fsp.readFile(path.join(tmpDir, 'innocent.txt'), 'utf8')).toBe('safe bytes'); const parent = path.dirname(tmpDir); await expect(fsp.access(path.join(parent, 'escape.txt'))).rejects.toThrow(); }); diff --git a/api/src/job-helpers.test.ts b/api/src/job-helpers.test.ts index 0c31f9d3..460902da 100644 --- a/api/src/job-helpers.test.ts +++ b/api/src/job-helpers.test.ts @@ -3,7 +3,7 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import { - resolveOriginalName, + resolveInputDestination, isNormalizedObjectForSession, markerConflictsWithExplicitFile, aggregateBashExtras, @@ -41,168 +41,19 @@ function makeRuntime(overrides: Partial & { language: string; pkgdir: s }; } -describe('resolveOriginalName', () => { - function responseWithHeader(value?: string): Response { - const headers = new Headers(); - if (value !== undefined) headers.set('content-disposition', value); - return new Response(null, { headers }); - } - - it('returns file.name when no Content-Disposition is present', () => { - expect( - resolveOriginalName(responseWithHeader(), { name: 'script.py', id: 'abc' }), - ).toBe('script.py'); - }); - - it('extracts quoted filename from Content-Disposition', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment; filename="server-name.py"'), - { name: 'client-name.py', id: 'abc' }, - ), - ).toBe('server-name.py'); - }); - - it('extracts unquoted filename from Content-Disposition', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment; filename=plain.txt'), - { name: 'ignored.txt', id: 'abc' }, - ), - ).toBe('plain.txt'); - }); - - it('falls back to file.id when file.name is empty and no header exists', () => { - expect( - resolveOriginalName(responseWithHeader(), { name: '', id: 'file-id-123' }), - ).toBe('file-id-123'); - }); - - it('falls back to file.name when header is malformed (no filename token)', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment'), - { name: 'fallback.py', id: 'abc' }, - ), - ).toBe('fallback.py'); - }); - - it('stops at the closing quote when the quoted filename is followed by more params', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment; filename="foo.txt"; size=123'), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('foo.txt'); - }); - - it('stops at a semicolon when the unquoted filename is followed by more params', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment; filename=foo.txt; size=123'), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('foo.txt'); - }); - - it('stops at whitespace when the unquoted filename is followed by whitespace-separated params', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment; filename=foo.txt extra'), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('foo.txt'); - }); - - it('returns empty string when both name and id are absent', () => { - expect(resolveOriginalName(responseWithHeader(), { name: '' })).toBe(''); +describe('resolveInputDestination', () => { + it('uses the caller-requested sandbox path', () => { + expect(resolveInputDestination({ name: 'nested/script.py', id: 'abc' })) + .toBe('nested/script.py'); }); - it('returns empty string when name is empty, id is absent, and header is malformed', () => { - expect( - resolveOriginalName(responseWithHeader('attachment'), { name: '' }), - ).toBe(''); - }); - - it('decodes RFC 5987 filename*= preserving slashes for nested artifact paths', () => { - expect( - resolveOriginalName( - responseWithHeader("attachment; filename*=UTF-8''test_folder%2Ftest_file.txt"), - { name: 'test_file.txt', id: 'abc' }, - ), - ).toBe('test_folder/test_file.txt'); - }); - - it('decodes RFC 5987 filename*= with a UTF-8 charset that includes a language tag', () => { - expect( - resolveOriginalName( - responseWithHeader("attachment; filename*=UTF-8'en'foo%20bar.txt"), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('foo bar.txt'); - }); - - it('decodes RFC 5987 filename*= with non-ASCII characters', () => { - expect( - resolveOriginalName( - responseWithHeader("attachment; filename*=UTF-8''%E4%BD%A0%E5%A5%BD.txt"), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('你好.txt'); - }); - - it('tolerates a filename*= form missing the UTF-8 prefix', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment; filename*=plain.txt'), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('plain.txt'); + it('falls back to the object id when no name exists', () => { + expect(resolveInputDestination({ name: '', id: 'file-id-123' })) + .toBe('file-id-123'); }); - it('falls through to legacy filename= when filename*= is malformed', () => { - expect( - resolveOriginalName( - responseWithHeader("attachment; filename*=UTF-8''bad%ZZ; filename=\"legacy.txt\""), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('legacy.txt'); - }); - - it('prefers filename*= over a legacy filename= present in the same header', () => { - expect( - resolveOriginalName( - responseWithHeader("attachment; filename=\"legacy.txt\"; filename*=UTF-8''nested%2Ffile.txt"), - { name: 'ignored', id: 'abc' }, - ), - ).toBe('nested/file.txt'); - }); - - it('keeps the requested name when an old file server advertises the opaque object basename', () => { - expect( - resolveOriginalName( - responseWithHeader("attachment; filename*=UTF-8''storage-id.xlsx"), - { name: 'Sample_-_Superstore.xlsx', id: 'storage-id' }, - ), - ).toBe('Sample_-_Superstore.xlsx'); - }); - - it('keeps the requested name for a legacy opaque filename header', () => { - expect( - resolveOriginalName( - responseWithHeader('attachment; filename="storage-id.csv"'), - { name: 'original.csv', id: 'storage-id' }, - ), - ).toBe('original.csv'); - }); - - it('keeps an authoritative nested filename even when its basename matches the object id', () => { - expect( - resolveOriginalName( - responseWithHeader("attachment; filename*=UTF-8''exports%2Fstorage-id.csv"), - { name: 'original.csv', id: 'storage-id' }, - ), - ).toBe('exports/storage-id.csv'); + it('returns an empty path when both name and id are absent', () => { + expect(resolveInputDestination({ name: '' })).toBe(''); }); }); diff --git a/api/src/job.ts b/api/src/job.ts index d2504de9..b7b82148 100644 --- a/api/src/job.ts +++ b/api/src/job.ts @@ -173,52 +173,12 @@ export function ensureNodeModulesSymlink( } /** - * Extracts the on-disk filename from a Content-Disposition response header, - * falling back to the request-supplied `file.name` (or `file.id` if no name - * was provided). Pure; exported for unit testing. - * - * Matches RFC 5987 / 8187 `filename*=UTF-8''` first because - * the file server emits that form for UTF-8-safe transport of arbitrary - * names — including paths with `/` separators that the legacy `filename=` - * form would mangle. Falls back to the legacy quoted (`filename="..."`) or - * unquoted (`filename=...`) forms, each stopping at the closing quote or - * the first whitespace/semicolon so trailing params like - * `attachment; filename="foo.txt"; size=123` correctly yield `foo.txt`. + * Resolves the on-disk destination for a by-reference input. The request owns + * the sandbox path; object response metadata must not redirect the write. + * Pure; exported for unit testing. */ -export function resolveOriginalName(response: Response, file: TFile): string { - const fallback = file.name || (file.id ?? ''); - const header = response.headers.get('content-disposition'); - if (!header) return fallback; - - const preferRequestedName = (candidate: string): string => { - /* Older file servers advertised path.basename(objectName) when an - * S3-compatible backend omitted original-filename user metadata. That - * basename is ``, so it is a storage identifier rather - * than an authoritative destination. Preserve the caller's requested name - * during rolling upgrades instead of exposing the opaque id in /mnt/data. */ - const opaqueStem = path.basename(candidate, path.extname(candidate)); - const isFlatObjectBasename = candidate === path.basename(candidate); - return file.name && file.id && isFlatObjectBasename && opaqueStem === file.id - ? file.name - : candidate; - }; - - const star = header.match(/filename\*=(?:UTF-8'[^']*')?([^;]+)/i); - if (star) { - const raw = star[1].trim(); - try { - return preferRequestedName(decodeURIComponent(raw)); - } catch { - /* Malformed percent-encoding (e.g. `%ZZ`) — fall through to the legacy - * forms. The same header may emit both `filename*=` and a legacy - * `filename=` per RFC 5987 §4.3, so a corrupt extended form should - * not poison a valid fallback. */ - } - } - - const match = header.match(/filename="([^"]+)"/i) - ?? header.match(/filename=([^\s;]+)/i); - return match ? preferRequestedName(match[1]) : fallback; +export function resolveInputDestination(file: TFile): string { + return file.name || (file.id ?? ''); } /** @@ -959,12 +919,9 @@ export class Job { ); } requestedDestinations.set(file.name, file); - /* Inline destinations are final, so keep them reserved while reference - * downloads resolve their authoritative Content-Disposition names. - * A ref's requested name is only a fallback, not a real destination yet: - * reserving every ref here makes concurrent swaps/order-dependent - * renames falsely conflict before the owning response has resolved. */ - if (!file.id) this.inputDestinations.set(file.name, file); + /* The request owns every sandbox destination. Reserve it before parallel + * priming begins so object metadata cannot redirect a later write. */ + this.inputDestinations.set(file.name, file); } if (this.session) { @@ -1083,10 +1040,8 @@ export class Job { ): Promise { throwIfAborted(context.signal); if (this.session && file.id && (await this.reusePrimedInput(file, context))) { - /* Reuse has no response header to pass through downloadAndWriteFile, so - * its requested name becomes authoritative only after the on-disk copy - * has been verified. Reserve it before another concurrent ref can claim - * and overwrite that path. */ + /* Inherited markers are registered after prime's initial reservation + * pass, so reserve the verified requested path here as well. */ this.reserveInputDestination(file, file.name); return; } @@ -1345,10 +1300,10 @@ export class Job { throw new Error(`HTTP error: ${response.status}`); } - const originalName = resolveOriginalName(response, file); - validateFilePath(originalName, operation.submissionDir); - this.reserveInputDestination(file, originalName); - const finalPath = path.join(operation.submissionDir, originalName); + const destination = resolveInputDestination(file); + validateFilePath(destination, operation.submissionDir); + this.reserveInputDestination(file, destination); + const finalPath = path.join(operation.submissionDir, destination); const finalParent = path.dirname(finalPath); /* Persistent-session workspaces can hold a prior turn's symlink, so build * ancestors no-follow; a fresh per-job workspace can use plain mkdir -p. */ @@ -1376,7 +1331,7 @@ export class Job { operation.signal, ); const readOnly = response.headers.get('x-read-only')?.toLowerCase() === 'true'; - this.inputFileHashes.set(originalName, { + this.inputFileHashes.set(destination, { originalId: file.id, originalSessionId: file.storage_session_id!, hash, @@ -1389,15 +1344,8 @@ export class Job { await applyReadOnlyInputPermissions(finalPath); } - /* Keep the in-memory TFile in sync with the on-disk name so that - * inputByName lookups in handleSessionFiles match walkDir's - * path.relative() output. Otherwise a Content-Disposition override - * would leave file.name pointing at the client-submitted name while - * the file lives under originalName on disk. */ - if (originalName !== file.name) file.name = originalName; - - this.log.info({ file: originalName, hash: hash.substring(0, 8) }, 'Downloaded file'); - return originalName; + this.log.info({ file: destination, hash: hash.substring(0, 8) }, 'Downloaded file'); + return destination; } catch (error: unknown) { if (response?.body && !response.bodyUsed) { await response.body.cancel().catch(() => {}); From 9d3936fa73e1b447f2e9a3a3961cdef8b411da18 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 12 Sep 2026 08:39:20 -0400 Subject: [PATCH 4/4] fix: disambiguate legacy dotted object identities (#186) --- service/src/file-object-resolver.test.ts | 37 +++++++++++++++++++++++- service/src/file-object-resolver.ts | 15 ++++++++-- service/src/file-server.ts | 13 ++++++--- 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/service/src/file-object-resolver.test.ts b/service/src/file-object-resolver.test.ts index 2342d87a..69ad3c2f 100644 --- a/service/src/file-object-resolver.test.ts +++ b/service/src/file-object-resolver.test.ts @@ -1,5 +1,12 @@ import { describe, expect, test } from 'bun:test'; -import { canonicalObjectId, FileObjectResolver, mapObjectDetails, storageKeyForUpload } from './file-object-resolver'; +import { + canonicalObjectId, + canonicalObjectKey, + FileObjectResolver, + legacyObjectId, + mapObjectDetails, + storageKeyForUpload, +} from './file-object-resolver'; import type { BucketItemStat } from 'minio'; describe('storage object resolution', () => { @@ -22,6 +29,34 @@ describe('storage object resolution', () => { expect(await resolver.listFresh('s', 'report')).toEqual([]); }); + test('legacy extension keys map to exactly one dotted or undotted identity', async () => { + const canonicalDotted = canonicalObjectKey('s', 'report.csv'); + const objects = new Set([ + 's/report.csv', + 's/report.csv.txt', + canonicalDotted, + ]); + const resolver = new FileObjectResolver({ + bucket: 'files', + list: async function* (prefix) { + for (const name of objects) if (name.startsWith(prefix)) yield { name }; + }, + stat: async () => ({ metaData: {} } as BucketItemStat), + }); + + expect(legacyObjectId('s/report.csv', 's')).toBe('report'); + expect(legacyObjectId('s/report.csv.txt', 's')).toBe('report.csv'); + expect(legacyObjectId('s/report', 's')).toBe('report'); + expect(legacyObjectId('other/report.csv', 's')).toBeUndefined(); + await expect(resolver.remember('s', 'report.csv', 's/report.csv')) + .rejects.toThrow('Object key does not match storage identity'); + expect(await resolver.listFresh('s', 'report')).toEqual(['s/report.csv']); + expect(await resolver.listFresh('s', 'report.csv')).toEqual([ + canonicalDotted, + 's/report.csv.txt', + ]); + }); + test('indexes exact identities while reading fresh version metadata on every request', async () => { const index = new Map(); let lists = 0; diff --git a/service/src/file-object-resolver.ts b/service/src/file-object-resolver.ts index bc4b4ce5..1665941a 100644 --- a/service/src/file-object-resolver.ts +++ b/service/src/file-object-resolver.ts @@ -19,6 +19,18 @@ export function canonicalObjectId(key: string): string | undefined { } } +/** Legacy objects were stored as `/`. + * Derive exactly one identity by removing that final extension when present. + * In particular, `session/report.csv` belongs to `report`, while a legacy + * `report.csv` identity would be stored as e.g. `session/report.csv.txt`. + * Dotted identities without a filename extension use canonical storage. */ +export function legacyObjectId(key: string, session: string): string | undefined { + if (path.posix.dirname(key) !== session) return undefined; + const basename = path.posix.basename(key); + const extension = path.posix.extname(basename); + return extension === '' ? basename : basename.slice(0, -extension.length); +} + export interface ObjectResolverDependencies { bucket: string; list(prefix: string): AsyncIterable<{ name?: string }>; @@ -58,8 +70,7 @@ export class FileObjectResolver { private matches(key: string, session: string, id: string): boolean { if (key === canonicalObjectKey(session, id)) return true; - return path.posix.dirname(key) === session && - (path.posix.basename(key) === id || path.posix.basename(key, path.posix.extname(key)) === id); + return legacyObjectId(key, session) === id; } async remember(session: string, id: string, key: string, replace = true): Promise { diff --git a/service/src/file-server.ts b/service/src/file-server.ts index 3dfa0a10..22302042 100644 --- a/service/src/file-server.ts +++ b/service/src/file-server.ts @@ -1,6 +1,12 @@ import b from 'busboy'; import { randomUUID } from 'node:crypto'; -import { canonicalObjectId, FileObjectResolver, mapObjectDetails, storageKeyForUpload } from './file-object-resolver'; +import { + canonicalObjectId, + FileObjectResolver, + legacyObjectId, + mapObjectDetails, + storageKeyForUpload, +} from './file-object-resolver'; import { sendFileDownload } from './file-download'; import path from 'path'; import IORedis from 'ioredis'; @@ -573,9 +579,8 @@ function parseObjectName(objectName: string | undefined): { session_id: string; const parts = objectName.split('/'); if (parts.length < 2) return null; const session_id = parts[0]; - const fileNameWithExt = parts[1]; - // Remove extension to get file_id - const file_id = fileNameWithExt.replace(/\.[^.]+$/, ''); + const file_id = legacyObjectId(objectName, session_id); + if (file_id == null) return null; return { session_id, file_id }; }