diff --git a/.changeset/storage-list-cursor-enumeration.md b/.changeset/storage-list-cursor-enumeration.md new file mode 100644 index 0000000000..f38d2ea6d1 --- /dev/null +++ b/.changeset/storage-list-cursor-enumeration.md @@ -0,0 +1,63 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-storage": minor +--- + +feat(spec,service-storage): restore prefix enumeration cursor-shaped — `IStorageService.list(prefix, { cursor, limit })` (#6781) + +`list?(prefix): Promise` was retired in #5540 / #5541 on the +measurement "nothing in the repo calls either". True for this repo, false one repo +over: `cloud` has two production callers — tenant attachment reclamation on +environment delete (cloud#935 is the incident where that sweep silently did nothing) +and marketplace snapshot GC. Both retirement notes reserved exactly one route back, +word for word, and this is it (maintainer ruling on cloud#1203, option B). + +**The new member is the reserved shape, not the old one restored.** + +```ts +list?(prefix: string, options?: StorageListOptions): Promise; + +interface StorageListOptions { cursor?: string; limit?: number } +interface StorageListPage { items: StorageFileInfo[]; nextCursor?: string } +``` + +The two defects #5266 measured in the old signature are now unrepresentable: + +| #5266 defect | Why it cannot recur | +| --- | --- | +| S3 truncated at 1000 objects, no signal | A page carries `nextCursor` **iff** more remains. The 1000 is now the default `limit`, and a capped page says so instead of looking complete. | +| local listed one level, S3 recursed | One prescribed semantics — raw key-string prefix, matched recursively — asserted against **both** backends from one table in `storage-adapter-list.conformance.test.ts`. | + +**Semantics every adapter must implement** (`IStorageService.list` carries the full +text): raw key prefix, so `list('a')` returns `a/b/c` *and* `ab.txt` and a trailing +slash is what scopes to a folder; files only, with filesystem directories and S3 +zero-byte directory markers both skipped; ascending key order; pages full except the +last; `nextCursor` iff more remains; no duplicates and no gaps across a run. + +**`limit` and `cursor` are refused, never coerced** — `VALIDATION_ERROR` / 400 +(ADR-0112). The validator and the cursor codec live on the *contract* +(`resolveStorageListLimit`, `encodeStorageListCursor`, `decodeStorageListCursor`), not +in each adapter, so two backends cannot answer the same bad argument two ways. A +consequence worth knowing: a cursor means one thing everywhere — "resume after this +key" — so both shipped adapters issue byte-identical cursors and a +`SwappableStorageService` adapter swap mid-sweep resumes instead of restarting. + +**Additive.** `list` stays OPTIONAL, like every other capability on this contract: a +third-party adapter that cannot enumerate is unaffected and still compiles. Making it +required would be a major-version act, and enumeration is genuinely optional for a +backend. + +Shipped with it: the S3 adapter loops `ListObjectsV2` with `ContinuationToken` inside a +single call so a `limit` past the 1000-key `MaxKeys` ceiling is served in full, and +resumes across calls with `StartAfter`; the local adapter emulates the S3 key space with +a pruned walk whose memory is bounded by `limit` rather than by the size of the tree; +`SwappableStorageService` forwards it. `storage-adapter-list-retirement.test.ts` is +renamed to `storage-adapter-list-contract.test.ts` and **flipped** rather than deleted — +it used to hold "the retired shape has not crept back", it now holds "both adapters +carry the restored member, in the cursor shape and not the array one". + +ADR-0087 note: the `storage-service-list-retired` ledger entry is amended, not withdrawn. +The single-argument `list(prefix)` stays retired and a call written against it still +fails to compile; what changed is the entry's `replacement`, which said "no replacement" +and would otherwise have shipped in the same release as the replacement — sending an +upgrader to hand-roll S3 pagination, which is precisely the option the ruling rejected. diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 154a8e1861..aa6c0f0975 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -366,9 +366,9 @@ One entry in this step is not a removal at all but a SECURE-DEFAULT FLIP, the sh - **`actor-user-roles-to-positions`** — `action body / AI route: ctx.user.roles (req.user.roles)` → ctx.user.positions (an AI route handler reads `req.user.positions`) — the same array, under the one spelling ADR-0090 D3 sanctions - Why not automatic: The THIRD face of the ADR-0090 `roles` → `positions` rename, and the only one whose surface the spec never declared. `ActorUser` (`packages/runtime/src/security/actor-user.ts`) is the ONE producer of the `user` envelope handed to an action body as `ctx.user` and to an AI route handler as `req.user`; it declared `positions` and `roles` side by side and filled them from a SINGLE assignment (`roles: core.positions`), so the two keys were verbatim identical on every dispatch — a second spelling of the vocabulary ADR-0090 D3 reserves and bans, published straight into author-written code. The maintainer ruled it closed IMMEDIATELY (2026-08-06 14:49Z, #6011): no deprecation window, no dual-emit, the alias simply gone in 17 (PR #6048). ⚠️ Do not read this entry across to its neighbour above: `action-session-roles-to-positions` governs `ctx.session`, a DIFFERENT object reached through the same `ctx`, and that one KEEPS its one-window dual-emit (#5613). Same word, same dispatch, two faces, two schedules — `ctx.user.roles` is absent in 17 while `ctx.session.roles` still answers for the length of its window. What makes this entry different in KIND from both session-side siblings: `ctx.user` has no spec schema and never had one. It is a runtime TS interface, so unlike `HookContext.session.roles` (tombstoned on a deliberately non-strict `HookContextSchema`, #5050) and unlike `ActionSessionSchema` (declared contract-first at #5697 precisely so its key could be renamed), there is no schema key here to tombstone and no `retiredKey()` prescription that could reach anybody — nothing ever ran an `ActorUser` through a `.parse()`, so a prescription there would have no one to reach. The enforced channel is tsc, and it reports at the READ site inside the author's own body; for an untyped or sandboxed body there is no enforced channel at all, which is exactly why this ledger entry has to exist — `spec-changes.json` and the generated upgrade guide are the ONLY way such a reader learns of the rename. It is the `findStream` (#4484) / `IStorageService.list` (#5540) disposition — a TS/API contract, no stored source, no tombstone, tsc at the call site — applied to a surface that lives one layer further out than either: those two are at least DECLARED in `packages/spec/src/contracts`, this one only in `packages/runtime`. Why it is a D3 semantic TODO and not a D2 conversion, on the same two independent grounds as its session sibling: FIRST, there is no source to convert — an `ActorUser` is constructed per dispatch and never persisted, so no `sys_metadata` row, example or template can carry the key (the `openApi31` (#4579) / `activationEvents` (#4657) / `hook-context-session-roles-retired` (#5050) shape). SECOND, the only place the key is ever SPELLED is inside an action body or an AI route handler: author-written JS/TS, or a sandboxed script. A declarative transform cannot safely rewrite an identifier inside free-form code — the same reason the ADR-0090 wave delegated `current_user.roles` to the author at step 13 (`cel-current-user-roles-to-positions`) instead of substituting text. The removal's hard precondition was met before it landed, and the result is recorded here because the ledger is where an upgrading consumer meets it: the declaration's own comment claimed the alias was "kept for the REST/AI shapes", and that claim was DISPROVEN face by face against `origin/main` — repo-wide `user.roles` was 4 hits, all of them in the pins PR #6048 flipped; the four `ActorUser` construction sites build server-side envelopes that never enter a response body; objectui's `.roles` reads belong to two unrelated producers (the better-auth session, and the `/auth/me/permissions` payload). The `cloud` repo was NOT reachable in that session and is the one consumer face left unverified — this entry, and the changeset's FROM/TO prescription, are its disposition. ADR-0090 D3 / ADR-0049 / ADR-0087, #6011 (PR #6048). - Done when: No action body reads `ctx.user.roles` and no AI route handler reads `req.user.roles`; every such read is `.positions` and observes the SAME array — the value was `ExecutionContext.positions` on both sides, so this is a pure key rename and no value has to be re-derived. Privilege is NOT re-derived from either spelling: a read that was `roles.includes('admin')` as an access check is rewritten to ask the security service (capability grants / placements / derived posture, ADR-0095), never renamed to `positions.includes('admin')` — renaming that read migrates the defect rather than the code. Unlike `ctx.session` there is NO window to migrate inside: in 17 the key is already absent, so a typed body fails `tsc` at the read while an untyped or sandboxed one silently sees `undefined` — move the read AS you upgrade, not after it. Verify against a real dispatch rather than a fixture: invoke an action (and an AI route) as a caller holding positions, assert the body observed them under the canonical key, and assert the old key is ABSENT by key existence (`'roles' in ctx.user === false`) rather than by `undefined`, which cannot tell a removed key from one left behind holding nothing — the runtime pin `action-ctx-user-shape.test.ts` asserts both halves that way. -- **`storage-service-list-retired`** — `contracts.IStorageService.list` → no replacement — track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket +- **`storage-service-list-retired`** — `contracts.IStorageService.list` → track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket — and where no such record exists, the cursor-shaped `list(prefix, { cursor, limit })` this entry reserved, restored in #6781 - Why not automatic: `list(prefix)` was an OPTIONAL contract method documented as "List files in a directory/prefix", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the "all files" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266). - - Done when: No code calls `storage.list(...)` on the `file-storage` service or on any `IStorageService` value. Code that needed "which files are under this prefix" reads the records it wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL — rather than asking the bucket, which is also the only form that stays correct past 1000 objects and across both adapters. An adapter that still IMPLEMENTS `list` keeps compiling (an extra method is not an error on a class) and is simply unreachable through the contract, so deleting it is cleanup that can follow. The break is on the CALLER side: `storage.list(...)` no longer type-checks, and a PROXY typed against `IStorageService` that forwards to `inner.list` is exactly such a caller — the one in `@objectstack/service-storage` goes with the adapters (#5541). + - Done when: No code calls `storage.list(...)` on the `file-storage` service or on any `IStorageService` value. Code that needed "which files are under this prefix" reads the records it wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL — rather than asking the bucket, which is also the only form that stays correct past 1000 objects and across both adapters. An adapter that still IMPLEMENTS `list` keeps compiling (an extra method is not an error on a class) and is simply unreachable through the contract, so deleting it is cleanup that can follow. The break is on the CALLER side: `storage.list(...)` no longer type-checks, and a PROXY typed against `IStorageService` that forwards to `inner.list` is exactly such a caller — the one in `@objectstack/service-storage` goes with the adapters (#5541). ⚠️ AMENDED 2026-08-09 (#6781, maintainer ruling on cloud#1203, option B): the RESERVED route in the paragraph above was taken. `list` exists again on the contract, cursor-shaped — `list(prefix, { cursor, limit })` returning `{ items, nextCursor }` — because cloud had two first-party callers this repo could not see when the measurement said "nothing calls it" (tenant attachment reclamation, marketplace snapshot GC). This does NOT un-retire anything and the acceptance criterion above is unchanged for what it actually governs: the single-argument `list(prefix): StorageFileInfo[]` is gone for good, a call written against it still fails to compile, and the two dialects it had are now pinned against each other in `storage-adapter-list.conformance.test.ts` rather than left to diverge. What changed for an upgrader is only the destination: prefer the records you wrote, and reach for the restored member when there are none. - **`driver-aggregate-undeclared-key-aliases-removed`** — `driver aggregate() call argument — query.aggregate and aggregations[].func` → query.aggregations and aggregations[].function — the spellings QueryASTSchema and AggregationNodeSchema have always declared - Why not automatic: `SqlDriver.aggregate` and `RemoteTransport.aggregate` each read two aliases the Query Protocol has never declared: `query.aggregations || query.aggregate` and `agg.function || agg.func`. "Never declared" is measured, not assumed — `git log -S` over `data/query.zod.ts` finds no commit that ever introduced either name, there is no `retiredKey()` tombstone and no alias-table entry for them (the file's only alias table is `SortNode`'s `direction` → `order`), and neither appears in any upgrade guide or release note. So this entry does not record a declared surface being withdrawn; it records a LENIENCY being withdrawn, which is why it is here rather than behind a tombstone. The only writers in this repository were the two driver packages' own fixtures — #4984's family, where a fixture spelling the alias keeps the tolerant limb green forever and no test in existence can go red on its deletion — so ADR-0049 enforce-or-remove applies once those are re-spelt. ⚠️ Do NOT read this across to `dashboard`/`page` measures: `aggregate` IS the canonical key there and `func` IS a declared, loudly-suggesting alias (`DatasetMeasureSchema`, ui/dataset.zod.ts). That neighbouring vocabulary is untouched, and it is the most likely reason an off-repo caller ever wrote these keys on a QUERY — one habit, two surfaces, only one of which declared it. This is a driver CALL ARGUMENT — code, never stack metadata — so there is no source for the D2 chain to rewrite and deliberately no schema tombstone: nothing ever ran a query through `QueryASTSchema.parse()` on this path. The enforced channel is tsc at the call site, once the parameter is `DriverQuery` — and for an untyped JS caller there is no enforced channel at all, which is exactly why this ledger entry has to exist: the generated upgrade guide is the only way such a reader learns of the rename. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011). ADR-0049 / ADR-0087, #6321 (PR #6404). - Done when: No caller passes `aggregate:` to a driver's `aggregate()`, and no aggregation entry spells its function `func:`; both are written `aggregations:` / `function:`. An inline literal still using either old spelling no longer type-checks (TS2353 at the call site). An untyped JS caller that keeps writing `aggregate:` silently receives no aggregate column — the grouping still happens, the measure is simply absent — and one that keeps writing `func:` receives INVALID_QUERY / 400 naming the undeclared function, identically on the local driver and the Turso remote transport. diff --git a/packages/services/service-storage/src/local-storage-adapter.test.ts b/packages/services/service-storage/src/local-storage-adapter.test.ts index 2c4bca84a0..bf00830d05 100644 --- a/packages/services/service-storage/src/local-storage-adapter.test.ts +++ b/packages/services/service-storage/src/local-storage-adapter.test.ts @@ -31,9 +31,12 @@ describe('LocalStorageAdapter', () => { expect(typeof storage.delete).toBe('function'); expect(typeof storage.exists).toBe('function'); expect(typeof storage.getInfo).toBe('function'); - // `list` is deliberately absent: IStorageService no longer declares it - // (#5540) and the adapter no longer implements it (#5541). The absence is - // pinned in `storage-adapter-list-retirement.test.ts`. + // `list` is back, cursor-shaped (#6781). Its presence and shape are pinned + // in `storage-adapter-list-contract.test.ts` (the flipped #5540/#5541 + // retirement pin) and its behaviour in + // `storage-adapter-list.conformance.test.ts`, which asserts this backend + // and the S3 one answer identically. + expect(typeof storage.list).toBe('function'); }); it('should upload and download a file', async () => { diff --git a/packages/services/service-storage/src/local-storage-adapter.ts b/packages/services/service-storage/src/local-storage-adapter.ts index 91b7f21865..978654fed4 100644 --- a/packages/services/service-storage/src/local-storage-adapter.ts +++ b/packages/services/service-storage/src/local-storage-adapter.ts @@ -12,10 +12,17 @@ import type { IStorageService, StorageUploadOptions, StorageFileInfo, + StorageListOptions, + StorageListPage, PresignedUploadDescriptor, PresignedDownloadDescriptor, PresignedDownloadOptions, } from '@objectstack/spec/contracts'; +import { + decodeStorageListCursor, + encodeStorageListCursor, + resolveStorageListLimit, +} from '@objectstack/spec/contracts'; /** * Configuration options for LocalStorageAdapter. @@ -46,6 +53,35 @@ export interface LocalStorageAdapterOptions { metrics?: MetricsRegistry; } +/** + * Directory under `rootDir` holding in-flight multipart chunks. Skipped by + * `list()` — chunks are not stored objects, and S3 does not surface multipart + * parts through `ListObjectsV2` either. + */ +const PARTS_DIR_NAME = '.parts'; + +/** + * Insert `value` into the ascending array `out`, keeping it sorted and no + * longer than `max`. + * + * Bounds `list()`'s memory to the page size instead of the size of the tree, + * while still producing a globally ordered result from a traversal that does + * not visit keys in order. + */ +function insertBounded(out: string[], value: string, max: number): void { + if (out.length >= max && value >= out[out.length - 1]!) return; + + let low = 0; + let high = out.length; + while (low < high) { + const mid = (low + high) >> 1; + if (out[mid]! < value) low = mid + 1; + else high = mid; + } + out.splice(low, 0, value); + if (out.length > max) out.length = max; +} + interface PresignTokenPayload { k: string; // storage key ct?: string; // content-type @@ -76,7 +112,7 @@ export class LocalStorageAdapter implements IStorageService { constructor(options: LocalStorageAdapterOptions) { this.rootDir = options.rootDir; - this.partsDir = join(this.rootDir, '.parts'); + this.partsDir = join(this.rootDir, PARTS_DIR_NAME); this.baseUrl = options.baseUrl ?? ''; this.basePath = options.basePath ?? '/api/v1/storage'; this.signingSecret = options.signingSecret ?? randomUUID(); @@ -87,7 +123,7 @@ export class LocalStorageAdapter implements IStorageService { * Wrap a storage operation with metrics instrumentation. Never swallows * the underlying error; instrumentation failures are silently ignored. */ - private async track(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise): Promise { + private async track(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise): Promise { const started = Date.now(); const baseLabels = { adapter: 'local', op } as const; try { @@ -189,17 +225,132 @@ export class LocalStorageAdapter implements IStorageService { }); } - // `list(prefix)` is gone (#5541), following its removal from IStorageService - // (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation was - // a single-level `readdir` that reported subdirectories as files, so it and the - // S3 adapter's recursive-but-truncated one answered the same call differently - // and neither said so. Nothing in the repo called either. Enumerate the records - // you wrote (`sys_file` / file references, paginated through ObjectQL) instead - // of the bucket; if a first-party caller ever needs real bucket enumeration it - // returns cursor-shaped — `list(prefix, { cursor, limit })` — with - // adapter-conformance cases proving both backends agree before it ships. - // Absence is pinned in `storage-adapter-list-retirement.test.ts`: an excess - // method on a class is not a type error, so tsc cannot hold this line. + // --------------------------------------------------------------------------- + // Prefix enumeration (#6781) + // --------------------------------------------------------------------------- + + /** + * Cursor-shaped prefix enumeration — see `IStorageService.list` for the + * contract every adapter shares. + * + * This backend EMULATES the S3 key-space semantics rather than the reverse, + * which is the whole reason the restored member is safe where the retired one + * was not (#5266 / #5540). Three consequences worth stating in code: + * + * - `prefix` is a **raw string prefix over keys**, not a directory path. The + * walk therefore filters on `key.startsWith(prefix)` and never resolves + * `prefix` as a path — `list('a')` sees `ab.txt`, exactly as S3 does. The + * retired implementation did `readdir(rootDir/prefix)`, which is where the + * one-level-deep dialect came from. + * - Only `isFile()` entries are emitted. Directories are recursed into and + * never stat'd into results (the retired implementation returned them as + * `StorageFileInfo` values whose `size` was a directory inode). + * - The adapter's own `.parts/` multipart staging area is skipped: those bytes + * are in-flight chunks, not stored objects, and S3 does not expose its + * multipart parts through `ListObjectsV2` either. + * + * Memory is bounded by `limit`, not by the size of the tree: the walk keeps at + * most `limit + 1` keys (the extra one answers "is there a next page?") and + * only stats the ones that survive. + * + * Shape and behaviour are pinned in `storage-adapter-list-contract.test.ts` + * and `storage-adapter-list.conformance.test.ts` — the latter runs every case + * against the S3 adapter too, because tsc cannot notice two backends drifting + * apart (an optional member may simply be absent, and an extra method on a + * class is never an error). + */ + async list(prefix: string, options?: StorageListOptions): Promise { + // Argument refusals come from the contract's shared helpers so this adapter + // and the S3 one cannot answer a bad `limit`/`cursor` two different ways. + // Deliberately OUTSIDE `track()`: a refused call never reached the disk, so + // counting it as a failed storage operation would misreport the backend. + const limit = resolveStorageListLimit(options?.limit); + const after = options?.cursor === undefined ? undefined : decodeStorageListCursor(options.cursor); + + return this.track('list', async () => { + const keys: string[] = []; + await this.collectListKeys('', prefix, after, limit + 1, keys); + + const hasMore = keys.length > limit; + const page = hasMore ? keys.slice(0, limit) : keys; + + const items: StorageFileInfo[] = []; + for (const key of page) { + try { + // Inline stat to avoid double-counting `head` operations. + const stat = await fs.stat(this.resolvePath(key)); + items.push({ key, size: stat.size, lastModified: stat.mtime }); + } catch { + // Deleted between the walk and the stat. Dropping it shortens the + // page but must NOT suppress `nextCursor` — which is why the cursor + // below is taken from `page` (the keys) rather than from `items`. + } + } + + return hasMore + ? { items, nextCursor: encodeStorageListCursor(page[page.length - 1]!) } + : { items }; + }); + } + + /** + * Depth-first walk collecting at most `want` matching keys in ascending key + * order. + * + * ⚠️ Directory-entry order is NOT global key order — `readdir` of `a` sorted + * gives `a` before `a.txt`, yet `a.txt` sorts BEFORE `a/x` (`.` is 0x2E, `/` + * is 0x2F). So results are merged through a bounded sorted insert rather than + * appended; a plain "take the first N encountered" would page out of order and + * a cursor built from it would skip keys. + */ + private async collectListKeys( + dir: string, + prefix: string, + after: string | undefined, + want: number, + out: string[], + ): Promise { + const absolute = dir ? join(this.rootDir, dir) : this.rootDir; + + let entries; + try { + entries = await fs.readdir(absolute, { withFileTypes: true }); + } catch (err: any) { + // A missing root (nothing uploaded yet) enumerates empty, like an empty + // bucket. Anything else is a real I/O fault and must surface. + if (err?.code === 'ENOENT' || err?.code === 'ENOTDIR') return; + throw err; + } + + entries.sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)); + + for (const entry of entries) { + const key = dir ? `${dir}/${entry.name}` : entry.name; + + if (entry.isDirectory()) { + // Multipart staging is adapter-internal, never a stored object. + if (dir === '' && entry.name === PARTS_DIR_NAME) continue; + + const branch = `${key}/`; + // Prune by prefix: every key under `branch` starts with `branch`, so the + // subtree can hold a match only if one string prefixes the other. + if (!branch.startsWith(prefix) && !prefix.startsWith(branch)) continue; + // Prune by cursor: if `branch` sorts before `after` and `after` is not + // inside it, then every key under it also sorts before `after`. + if (after !== undefined && !after.startsWith(branch) && branch <= after) continue; + + await this.collectListKeys(key, prefix, after, want, out); + continue; + } + + // Symlinks, sockets and FIFOs are not stored objects either. + if (!entry.isFile()) continue; + if (!key.startsWith(prefix)) continue; + if (after !== undefined && key <= after) continue; + + insertBounded(out, key, want); + } + } // --------------------------------------------------------------------------- // Presigned URL helpers diff --git a/packages/services/service-storage/src/s3-storage-adapter.ts b/packages/services/service-storage/src/s3-storage-adapter.ts index 97e98e9a06..ec020fdf24 100644 --- a/packages/services/service-storage/src/s3-storage-adapter.ts +++ b/packages/services/service-storage/src/s3-storage-adapter.ts @@ -9,12 +9,27 @@ import type { IStorageService, StorageUploadOptions, StorageFileInfo, + StorageListOptions, + StorageListPage, PresignedUploadDescriptor, PresignedDownloadDescriptor, PresignedDownloadOptions, } from '@objectstack/spec/contracts'; +import { + decodeStorageListCursor, + encodeStorageListCursor, + resolveStorageListLimit, +} from '@objectstack/spec/contracts'; import { contentDispositionValue } from './content-disposition.js'; +/** + * Hard ceiling `ListObjectsV2` applies to `MaxKeys`, regardless of what the + * caller asks for. It is the number that silently truncated the retired + * `list(prefix)` (#5266); here it only ever bounds ONE round-trip, and `list()` + * loops until the caller's page is full. + */ +const S3_LIST_MAX_KEYS = 1000; + /** * Configuration for the S3 storage adapter. */ @@ -72,7 +87,7 @@ export class S3StorageAdapter implements IStorageService { * Records ok/error counters, a duration histogram, and an error counter * keyed by error class on failure. Never swallows the underlying error. */ - private async track(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise): Promise { + private async track(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise): Promise { const started = Date.now(); const baseLabels = { adapter: 's3', op } as const; try { @@ -211,19 +226,101 @@ export class S3StorageAdapter implements IStorageService { }); } - // `list(prefix)` is gone (#5541), following its removal from IStorageService - // (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation - // issued one `ListObjectsV2` and read neither `IsTruncated` nor - // `ContinuationToken`, so past 1000 objects it returned the first page with - // nothing to distinguish it from a complete answer — while the local adapter - // answered the same call one level deep. Nothing in the repo called either. - // Enumerate the records you wrote (`sys_file` / file references, paginated - // through ObjectQL) instead of the bucket; if a first-party caller ever needs - // real bucket enumeration it returns cursor-shaped — - // `list(prefix, { cursor, limit })` — with adapter-conformance cases proving - // both backends agree before it ships. Absence is pinned in - // `storage-adapter-list-retirement.test.ts`: an excess method on a class is - // not a type error, so tsc cannot hold this line. + // --------------------------------------------------------------------------- + // Prefix enumeration (#6781) + // --------------------------------------------------------------------------- + + /** + * Cursor-shaped prefix enumeration — see `IStorageService.list` for the + * contract every adapter shares. + * + * Two paging mechanisms, one per layer, and confusing them is the defect this + * method exists to make impossible: + * + * - **Inside one call**, `ContinuationToken` loops over `ListObjectsV2`'s own + * pages until the CALLER's page is full. The retired implementation issued + * exactly one request and read neither `IsTruncated` nor + * `ContinuationToken`, so past `MaxKeys` it returned a partial answer that + * looked complete (#5266). + * - **Between calls**, the caller-facing cursor is the last key returned, + * resumed with `StartAfter`. It is deliberately NOT the S3 continuation + * token: a key-based cursor means the SAME thing on every backend, so the + * local adapter issues and accepts byte-identical cursors, a token is + * refused identically by both, and a `SwappableStorageService` adapter swap + * mid-sweep resumes instead of restarting. An opaque S3 token would have + * made the cursor a second, per-backend dialect — the exact shape of the + * original defect, moved onto the continuation. + * + * Shape and behaviour are pinned in `storage-adapter-list-contract.test.ts` + * and `storage-adapter-list.conformance.test.ts`, the latter driving this + * adapter against a fake bucket that enforces the real 1000-key `MaxKeys` + * ceiling — so an implementation that issued one request per call could not + * pass it. + */ + async list(prefix: string, options?: StorageListOptions): Promise { + // Refusals come from the contract's shared helpers, outside `track()`: a + // refused call never reached S3, so it is not a failed storage operation. + const limit = resolveStorageListLimit(options?.limit); + const startAfter = options?.cursor === undefined ? undefined : decodeStorageListCursor(options.cursor); + + return this.track('list', async () => { + const client = await this.getClient(); + const s3 = await this.s3Mod(); + + const items: StorageFileInfo[] = []; + let continuationToken: string | undefined; + let more = false; + // The last key EXAMINED, which is not always the last key emitted: a page + // whose trailing entries are all directory markers still advanced the + // scan past them, and resuming from the last emitted key instead would + // re-read them forever. + let lastKeySeen: string | undefined; + + while (items.length < limit) { + const res = await client.send( + new s3.ListObjectsV2Command({ + Bucket: this.bucket, + Prefix: prefix, + MaxKeys: Math.min(limit - items.length, S3_LIST_MAX_KEYS), + // `StartAfter` is honoured only on the first request of a run; S3 + // ignores it once `ContinuationToken` is present, which is correct + // — the token already encodes a position past it. + ...(continuationToken + ? { ContinuationToken: continuationToken } + : startAfter !== undefined + ? { StartAfter: startAfter } + : {}), + }), + ); + + for (const object of res.Contents ?? []) { + const key: string | undefined = object?.Key; + if (!key) continue; + lastKeySeen = key; + // A zero-byte key ending in `/` is a console-created directory + // marker, not a file. The local backend cannot represent one at all, + // so emitting it here would be a per-backend dialect. + if (key.endsWith('/')) continue; + items.push({ + key, + size: object.Size ?? 0, + lastModified: object.LastModified ?? new Date(), + }); + } + + if (!res.IsTruncated || !res.NextContinuationToken) { + more = false; + break; + } + continuationToken = res.NextContinuationToken; + more = true; + } + + return more && lastKeySeen !== undefined + ? { items, nextCursor: encodeStorageListCursor(lastKeySeen) } + : { items }; + }); + } // --------------------------------------------------------------------------- // Presigned URLs diff --git a/packages/services/service-storage/src/storage-adapter-list-contract.test.ts b/packages/services/service-storage/src/storage-adapter-list-contract.test.ts new file mode 100644 index 0000000000..bdd7edbe19 --- /dev/null +++ b/packages/services/service-storage/src/storage-adapter-list-contract.test.ts @@ -0,0 +1,109 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Shape pin — `list(prefix, { cursor, limit })` on the shipped storage adapters. + * + * This file was `storage-adapter-list-retirement.test.ts` and it is the SAME + * pin flipped, not a new one (#6781). It used to hold "the retired + * `list(prefix)` has not crept back into either adapter"; it now holds "both + * adapters implement the restored member, and they implement the CURSOR shape + * rather than the retired array one". The load it bears did not go away when + * `list` came back — it moved, and deleting the file would have dropped it. + * + * **Why a runtime pin and not `@ts-expect-error`.** On the *contract* side tsc + * is the enforced channel and + * `packages/spec/src/contracts/storage-service.test.ts` carries the directives + * there. Neither reaches an adapter: a **class** that `implements` an interface + * is only checked for the members the interface requires, and `list` is + * OPTIONAL, so an adapter that quietly drops it — or reintroduces the + * single-argument array version alongside — compiles perfectly. That is the + * same tsc blind spot #5541 documented, in mirror image: it could not see the + * method coming back, and it cannot see it going away either. + * + * `SwappableStorageService` gets no pin here: it forwards through an `inner` + * typed as `IStorageService`, so a passthrough whose signature drifts from the + * contract fails to compile. tsc holds that line already. + * + * Behaviour — pagination, ordering, prefix semantics, both backends agreeing — + * is pinned next door in `storage-adapter-list.conformance.test.ts`. This file + * only answers "is the member there, on both, in the right shape". + */ + +import { describe, it, expect } from 'vitest'; +import { LocalStorageAdapter } from './local-storage-adapter'; +import { S3StorageAdapter } from './s3-storage-adapter'; + +/** Every method name reachable on an instance, own + prototype chain. */ +function reachableMethodNames(instance: object): string[] { + const names = new Set(); + for ( + let cursor: object | null = instance; + cursor && cursor !== Object.prototype; + cursor = Object.getPrototypeOf(cursor) + ) { + for (const name of Object.getOwnPropertyNames(cursor)) names.add(name); + } + return [...names]; +} + +const adapters = [ + { + name: 'LocalStorageAdapter', + make: () => new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' }), + }, + { + // The constructor only records options; the AWS SDK is imported lazily on + // first use, so this never touches the network or the peer dependency. + name: 'S3StorageAdapter', + make: () => new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' }), + }, +] as const; + +describe('storage adapters implement cursor-shaped list(prefix, opts) (#6781)', () => { + it.each(adapters)('$name exposes list as a method', ({ make }) => { + const adapter = make(); + + expect(reachableMethodNames(adapter)).toContain('list'); + expect('list' in adapter).toBe(true); + expect(typeof (adapter as unknown as Record).list).toBe('function'); + }); + + it.each(adapters)('$name declares list with the cursor arity (prefix, options)', ({ make }) => { + // MEASURED, not assumed: a TypeScript `options?` compiles to a plain + // parameter with no default, so `Function.length` counts it — the cursor + // shape reports 2 where the retired `list(prefix)` reported 1. (The first + // draft of this case asserted 1 on the reasoning that `length` stops at the + // first optional parameter; that rule is about DEFAULTED parameters, and + // the test said so.) Arity is a cheap discriminator, not a sufficient one: + // the return VALUE is asserted below and exhaustively in the conformance + // suite. + const list = (make() as unknown as Record unknown>).list!; + expect(list.length).toBe(2); + }); + + it('LocalStorageAdapter.list answers a page object, never the retired array', async () => { + // Runs against a root that does not exist: an empty bucket enumerates + // empty, and the SHAPE of "empty" is the thing under test. The retired + // implementation would have answered `[]` here — truthy-empty and + // indistinguishable from a complete listing. + const adapter = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' }); + + const page = await adapter.list(''); + + expect(Array.isArray(page)).toBe(false); + expect(page.items).toEqual([]); + expect(page.nextCursor).toBeUndefined(); + expect(Object.keys(page)).toEqual(['items']); + }); + + it('still exposes the per-key contract members the retirement kept', () => { + // Guards the pin against the mirror failure: a pin that passes because the + // adapter has no methods at all would be green and meaningless. + for (const { make } of adapters) { + const adapter = make(); + for (const member of ['upload', 'download', 'delete', 'exists', 'getInfo'] as const) { + expect(typeof (adapter as unknown as Record)[member]).toBe('function'); + } + } + }); +}); diff --git a/packages/services/service-storage/src/storage-adapter-list-retirement.test.ts b/packages/services/service-storage/src/storage-adapter-list-retirement.test.ts deleted file mode 100644 index 9501210898..0000000000 --- a/packages/services/service-storage/src/storage-adapter-list-retirement.test.ts +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * Retirement pin — `list(prefix)` on the shipped storage adapters. - * - * `IStorageService.list?(prefix)` was removed from the contract in #5540 - * (ADR-0049 enforce-or-remove; analysis #5266), and the two shipped adapters' - * own implementations were removed in #5541. This file keeps the retired - * surface retired. - * - * **Why a runtime pin and not `@ts-expect-error`.** On the *contract* side tsc - * is the enforced channel and `packages/spec/src/contracts/storage-service.test.ts` - * already carries both directives — reading `storage.list` is a type error, and - * an object literal typed `IStorageService` that declares `list` is an excess - * property. Neither reaches an adapter: a **class** that `implements` an - * interface is only checked for the members the interface requires, so a class - * may carry any number of extra methods without a single type error. That is - * exactly what the retirement changeset promises adapter authors ("an - * implementation left in place still compiles"), and it is also why tsc cannot - * notice these two coming back. The pin has to read the shape at runtime. - * - * `SwappableStorageService` deliberately gets no pin here: it forwards to an - * `inner` typed as `IStorageService`, so a re-added `list` passthrough fails to - * compile — which is how #5540 found it in the first place. tsc holds that line - * already; duplicating it here would pin nothing new. - * - * If prefix enumeration ever comes back it comes back cursor-shaped — - * `list(prefix, { cursor, limit })` returning a page plus a continuation token, - * with adapter-conformance cases (nested keys, directory entries, more than - * 1000 objects) proving both backends agree. Restoring the old single-argument - * shape to satisfy this file is the one fix that is not a fix. - */ - -import { describe, it, expect } from 'vitest'; -import { LocalStorageAdapter } from './local-storage-adapter'; -import { S3StorageAdapter } from './s3-storage-adapter'; - -/** Every method name reachable on an instance, own + prototype chain. */ -function reachableMethodNames(instance: object): string[] { - const names = new Set(); - for ( - let cursor: object | null = instance; - cursor && cursor !== Object.prototype; - cursor = Object.getPrototypeOf(cursor) - ) { - for (const name of Object.getOwnPropertyNames(cursor)) names.add(name); - } - return [...names]; -} - -describe('storage adapters no longer implement list(prefix) (#5540 / #5541)', () => { - it('LocalStorageAdapter exposes no list member', () => { - const adapter = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' }); - - expect(reachableMethodNames(adapter)).not.toContain('list'); - expect('list' in adapter).toBe(false); - expect((adapter as unknown as Record).list).toBeUndefined(); - }); - - it('S3StorageAdapter exposes no list member', () => { - // The constructor only records options; the AWS SDK is imported lazily on - // first use, so this never touches the network or the peer dependency. - const adapter = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' }); - - expect(reachableMethodNames(adapter)).not.toContain('list'); - expect('list' in adapter).toBe(false); - expect((adapter as unknown as Record).list).toBeUndefined(); - }); - - it('still exposes the per-key contract members the retirement kept', () => { - // Guards the pin against the mirror failure: a pin that passes because the - // adapter has no methods at all would be green and meaningless. - const local = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' }); - const s3 = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' }); - - for (const adapter of [local, s3]) { - for (const member of ['upload', 'download', 'delete', 'exists', 'getInfo'] as const) { - expect(typeof (adapter as unknown as Record)[member]).toBe('function'); - } - } - }); -}); diff --git a/packages/services/service-storage/src/storage-adapter-list.conformance.test.ts b/packages/services/service-storage/src/storage-adapter-list.conformance.test.ts new file mode 100644 index 0000000000..1ec64e9796 --- /dev/null +++ b/packages/services/service-storage/src/storage-adapter-list.conformance.test.ts @@ -0,0 +1,589 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Adapter conformance — `list(prefix, { cursor, limit })` on BOTH shipped + * backends (#6781). + * + * ## Why this file is the deliverable, not a nicety + * + * `list(prefix)` was retired (#5266 / #5540 / #5541) because one contract + * method had two implementations that answered the same call differently, and + * *nothing in the repository could tell*: the local adapter listed one level + * deep and returned directories as files, the S3 adapter recursed and stopped + * at 1000 objects. Each had tests. Each was green. Neither test ever compared + * the two, so the divergence was invisible until a real caller would have hit + * it on one deployment and not the other. An interface only one driver honours + * is a second de-facto contract (AGENTS.md PD #12). + * + * So every behavioural case below runs against BOTH backends from one table, + * and the last block compares them key-for-key and cursor-for-cursor on an + * identical key set. A case that only one backend can express (a real + * filesystem directory; an S3 zero-byte directory marker) is written as the + * backend-specific half of the SAME semantic — "directories never appear" — + * rather than left out. + * + * ## The S3 side is a fake bucket, and the fake is the load-bearing part + * + * There is no real S3 in CI, so `@aws-sdk/client-s3` is mocked with a bucket + * that implements the documented `ListObjectsV2` contract rather than a + * convenient one: + * + * - keys answered in ascending (UTF-8 byte) order; + * - `Prefix` as a RAW STRING prefix — `Prefix: 'a'` matches `ab.txt`; + * - `MaxKeys` honoured **and capped at 1000**, the real service ceiling. This + * is what makes ">1000 objects enumerated via cursor" a real test: an + * adapter that issues one request per `list()` call cannot pass it; + * - `IsTruncated` + `NextContinuationToken` for intra-call paging, and + * `StartAfter` for resuming a later call; + * - the continuation token is an OPAQUE HANDLE (`ct-3`), deliberately NOT + * derived from a key. If the adapter ever leaked it out as the caller-facing + * `nextCursor`, the contract's `decodeStorageListCursor` would refuse it and + * the cross-backend cursor comparison would fail — which is the point. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { promises as fs } from 'node:fs'; +import { join, dirname } from 'node:path'; +import { tmpdir } from 'node:os'; +import { randomUUID } from 'node:crypto'; +import type { IStorageService } from '@objectstack/spec/contracts'; +import { + DEFAULT_STORAGE_LIST_LIMIT, + encodeStorageListCursor, +} from '@objectstack/spec/contracts'; + +const fakeS3 = vi.hoisted(() => { + /** The real `ListObjectsV2` ceiling on `MaxKeys`, whatever the caller asks. */ + const MAX_KEYS_CEILING = 1000; + + const objects = new Map(); + const listInputs: Array> = []; + const tokens = new Map(); + let tokenSeq = 0; + + function listObjectsV2(input: Record): Record { + listInputs.push(input); + + const prefix: string = input.Prefix ?? ''; + const maxKeys = Math.min( + typeof input.MaxKeys === 'number' ? input.MaxKeys : MAX_KEYS_CEILING, + MAX_KEYS_CEILING, + ); + + const all = [...objects.keys()] + .filter((key) => key.startsWith(prefix)) + .sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)); + + // `ContinuationToken` wins over `StartAfter`, as the real service does. + let after: string | undefined; + if (input.ContinuationToken !== undefined) { + after = tokens.get(input.ContinuationToken); + if (after === undefined) { + const err = new Error('The continuation token provided is incorrect') as Error & { + name: string; + }; + err.name = 'InvalidArgument'; + throw err; + } + } else if (input.StartAfter !== undefined) { + after = input.StartAfter; + } + + const start = after === undefined ? 0 : all.filter((key) => key <= after!).length; + const window = all.slice(start, start + maxKeys); + const isTruncated = start + window.length < all.length; + + let nextContinuationToken: string | undefined; + if (isTruncated) { + nextContinuationToken = `ct-${++tokenSeq}`; + tokens.set(nextContinuationToken, window[window.length - 1]!); + } + + return { + Prefix: prefix, + MaxKeys: maxKeys, + KeyCount: window.length, + IsTruncated: isTruncated, + NextContinuationToken: nextContinuationToken, + Contents: window.map((key) => ({ + Key: key, + Size: objects.get(key)!.size, + LastModified: objects.get(key)!.lastModified, + })), + }; + } + + function reset(): void { + objects.clear(); + listInputs.length = 0; + tokens.clear(); + tokenSeq = 0; + } + + return { MAX_KEYS_CEILING, objects, listInputs, listObjectsV2, reset }; +}); + +vi.mock('@aws-sdk/client-s3', () => { + class ListObjectsV2Command { + constructor(public readonly input: Record) {} + } + class S3Client { + constructor(_config: unknown) {} + async send(command: unknown): Promise { + if (command instanceof ListObjectsV2Command) { + return fakeS3.listObjectsV2(command.input); + } + throw new Error( + `fake S3 bucket: this suite only serves ListObjectsV2, got ${(command as any)?.constructor?.name}`, + ); + } + } + return { + S3Client, + ListObjectsV2Command, + PutObjectCommand: class {}, + GetObjectCommand: class {}, + DeleteObjectCommand: class {}, + HeadObjectCommand: class {}, + CreateMultipartUploadCommand: class {}, + UploadPartCommand: class {}, + CompleteMultipartUploadCommand: class {}, + AbortMultipartUploadCommand: class {}, + }; +}); + +// `vi.mock` is hoisted above every import in this file, so the lazy +// `await import('@aws-sdk/client-s3')` inside `S3StorageAdapter` resolves to the +// fake above no matter where these two sit. +import { LocalStorageAdapter } from './local-storage-adapter'; +import { S3StorageAdapter } from './s3-storage-adapter'; + +// --------------------------------------------------------------------------- +// Backend harness +// --------------------------------------------------------------------------- + +interface Backend { + readonly adapter: IStorageService; + seed(keys: string[]): Promise; + dispose(): Promise; +} + +async function makeLocalBackend(): Promise { + const rootDir = join(tmpdir(), `os-list-conformance-${randomUUID()}`); + await fs.mkdir(rootDir, { recursive: true }); + const adapter = new LocalStorageAdapter({ rootDir }); + const madeDirs = new Set(); + + return { + adapter, + rootDir, + async seed(keys) { + for (const key of keys) { + const path = join(rootDir, key); + const dir = dirname(path); + if (!madeDirs.has(dir)) { + await fs.mkdir(dir, { recursive: true }); + madeDirs.add(dir); + } + // Content is the key itself, so `size` is a value the assertions can + // predict without reading the file back. + await fs.writeFile(path, Buffer.from(key, 'utf8')); + } + }, + async dispose() { + await fs.rm(rootDir, { recursive: true, force: true }); + }, + }; +} + +async function makeS3Backend(): Promise { + fakeS3.reset(); + const adapter = new S3StorageAdapter({ bucket: 'conformance-bucket', region: 'us-east-1' }); + + return { + adapter, + async seed(keys) { + for (const key of keys) { + fakeS3.objects.set(key, { + size: Buffer.byteLength(key, 'utf8'), + lastModified: new Date('2026-01-01T00:00:00.000Z'), + }); + } + }, + async dispose() { + fakeS3.reset(); + }, + }; +} + +const BACKENDS = [ + { name: 'local', make: makeLocalBackend }, + { name: 's3', make: makeS3Backend }, +] as const; + +/** Page a prefix to exhaustion, returning one entry per page. */ +async function pageThrough( + adapter: IStorageService, + prefix: string, + limit: number, +): Promise> { + const pages: Array<{ keys: string[]; nextCursor?: string }> = []; + let cursor: string | undefined; + let guard = 0; + + do { + const page = await adapter.list!(prefix, { limit, cursor }); + pages.push({ keys: page.items.map((item) => item.key), nextCursor: page.nextCursor }); + cursor = page.nextCursor; + if (++guard > 500) throw new Error('list() pagination did not terminate'); + } while (cursor !== undefined); + + return pages; +} + +/** Capture a rejection's ADR-0112 envelope without asserting on a throw alone. */ +async function captureRefusal(fn: () => Promise): Promise { + try { + await fn(); + } catch (err) { + return err as Error & { code?: string; status?: number }; + } + // Reaching here means the call ANSWERED. Named explicitly, because "it did + // not refuse at all" and "it refused with the wrong envelope" are two + // different defects and a bare `rejects.toThrow()` cannot separate them. + throw new Error('expected the call to be refused, but it resolved'); +} + +/** + * `a.txt` earns its place: on the local backend `readdir` yields the directory + * `a` BEFORE the file `a.txt`, yet `a.txt` sorts before every key inside `a/` + * (`.` is 0x2E, `/` is 0x2F). So directory-traversal order and key order + * disagree here and nowhere else in this fixture — without it, a local + * implementation that simply emits keys as it walks them passes every + * single-backend case and is caught only by the cross-backend comparison. + * Measured, not reasoned: dropping the ordered insert turned exactly four + * cross-backend cases red and left the local suite green until this key existed. + */ +const SMALL_SET = [ + 'a.txt', + 'a/b/c.txt', + 'a/b/d.txt', + 'a/z.txt', + 'ab.txt', + 'b.txt', +]; + +/** 2500 keys — comfortably past the S3 `MaxKeys` ceiling of 1000. */ +const BULK_SET = Array.from({ length: 2500 }, (_, i) => { + const bucket = String(Math.floor(i / 100)).padStart(2, '0'); + return `bulk/${bucket}/${String(i).padStart(4, '0')}.bin`; +}); + +// --------------------------------------------------------------------------- +// Behaviour, asserted identically on every backend +// --------------------------------------------------------------------------- + +describe.each(BACKENDS)('$name adapter — list() conformance', ({ make }) => { + let backend: Backend; + + beforeAll(async () => { + backend = await make(); + await backend.seed(SMALL_SET); + await backend.seed(BULK_SET); + }, 60_000); + + afterAll(async () => { + await backend.dispose(); + }); + + it('sees nested keys under a bare prefix (the local one-level dialect, #5266)', async () => { + const page = await backend.adapter.list!('a'); + + // `a/b/c.txt` is TWO levels below `a`. The retired local implementation + // returned `a/b` — a directory — and never reached the file. + expect(page.items.map((item) => item.key)).toEqual([ + 'a.txt', + 'a/b/c.txt', + 'a/b/d.txt', + 'a/z.txt', + 'ab.txt', + ]); + }); + + it('matches a RAW key prefix, so a trailing slash is what scopes to a folder', async () => { + // `ab.txt` is in scope for `list('a')` and out of scope for `list('a/')`. + // This is S3's meaning of "prefix", and the local adapter emulates it + // rather than resolving the argument as a directory path. + const bare = await backend.adapter.list!('a'); + const scoped = await backend.adapter.list!('a/'); + + expect(bare.items.map((i) => i.key)).toContain('ab.txt'); + expect(scoped.items.map((i) => i.key)).toEqual(['a/b/c.txt', 'a/b/d.txt', 'a/z.txt']); + }); + + it('answers in ascending key order', async () => { + const page = await backend.adapter.list!('a'); + const keys = page.items.map((i) => i.key); + + expect(keys).toEqual([...keys].sort((x, y) => (x < y ? -1 : x > y ? 1 : 0))); + }); + + it('never returns a directory entry as a file', async () => { + // `a/b` is a real directory on the local backend and has no object at all + // on S3. Neither may surface: the retired local implementation stat'd it + // into the result with a directory inode as `size`. + const page = await backend.adapter.list!(''); + const keys = page.items.map((i) => i.key); + + expect(keys).not.toContain('a/b'); + expect(keys).not.toContain('a'); + expect(keys.every((key) => !key.endsWith('/'))).toBe(true); + }); + + it('reports a real byte size for each item', async () => { + const page = await backend.adapter.list!('ab.txt'); + + expect(page.items).toHaveLength(1); + expect(page.items[0]!.size).toBe(Buffer.byteLength('ab.txt', 'utf8')); + expect(page.items[0]!.lastModified).toBeInstanceOf(Date); + }); + + it('pages a set larger than `limit` with an EXACT, non-overlapping union', async () => { + const pages = await pageThrough(backend.adapter, 'bulk/', 400); + const seen = pages.flatMap((page) => page.keys); + + // 1. Union is exactly the seeded set — nothing missed. + expect(new Set(seen)).toEqual(new Set(BULK_SET)); + // 2. Non-overlapping — nothing returned twice. + expect(seen).toHaveLength(BULK_SET.length); + expect(new Set(seen).size).toBe(seen.length); + // 3. Globally ordered across page boundaries, not merely within a page. + expect(seen).toEqual([...BULK_SET].sort((x, y) => (x < y ? -1 : x > y ? 1 : 0))); + // 4. Every page full except the last, and ONLY the last lacks a cursor. + expect(pages).toHaveLength(Math.ceil(BULK_SET.length / 400)); + for (const page of pages.slice(0, -1)) { + expect(page.keys).toHaveLength(400); + expect(page.nextCursor).toBeDefined(); + } + expect(pages.at(-1)!.keys).toHaveLength(BULK_SET.length % 400); + expect(pages.at(-1)!.nextCursor).toBeUndefined(); + }, 60_000); + + it('fills one page past the backend\'s own page size (>1000 in a single call)', async () => { + // The retired S3 implementation issued exactly ONE ListObjectsV2 and never + // read `IsTruncated`, so it could not answer more than 1000 whatever the + // caller asked. A short page here is that defect, restored. + const page = await backend.adapter.list!('bulk/', { limit: 1500 }); + + expect(page.items).toHaveLength(1500); + expect(page.nextCursor).toBeDefined(); + expect(page.items.map((i) => i.key)).toEqual(BULK_SET.slice(0, 1500)); + }, 60_000); + + it('an OMITTED limit pages at the contract default instead of truncating', async () => { + // The old defect in one sentence: 1000 objects came back and the caller had + // no way to know there were 2500. Same number here, opposite meaning — the + // page is capped AND says so. + const first = await backend.adapter.list!('bulk/'); + + expect(first.items).toHaveLength(DEFAULT_STORAGE_LIST_LIMIT); + expect(first.nextCursor).toBeDefined(); + + const seen = [...first.items.map((i) => i.key)]; + let cursor = first.nextCursor; + while (cursor !== undefined) { + const page: Awaited>> = + await backend.adapter.list!('bulk/', { cursor }); + seen.push(...page.items.map((i) => i.key)); + cursor = page.nextCursor; + } + + expect(seen).toHaveLength(BULK_SET.length); + expect(new Set(seen)).toEqual(new Set(BULK_SET)); + }, 60_000); + + it('answers an empty page — never a cursor — for a prefix that matches nothing', async () => { + const page = await backend.adapter.list!('no-such-prefix/'); + + expect(page.items).toEqual([]); + expect(page.nextCursor).toBeUndefined(); + }); + + it('refuses a non-positive-integer `limit` with the ADR-0112 envelope', async () => { + for (const limit of [0, -1, 1.5, Number.NaN]) { + const err = await captureRefusal(() => backend.adapter.list!('a', { limit })); + + // code AND status: the envelope is the contract, and a bare `toThrow()` + // would pass on any incidental error from deeper in the backend. + expect(err.code).toBe('VALIDATION_ERROR'); + expect(err.status).toBe(400); + expect(err.message).toContain(`'limit' must be a positive integer`); + } + }); + + it('refuses a cursor it did not issue with the ADR-0112 envelope', async () => { + // A silently-ignored bad cursor restarts the sweep from key zero, which for + // a paging reclamation job is an infinite loop that looks like progress. + const err = await captureRefusal(() => backend.adapter.list!('a', { cursor: 'not-a-cursor!' })); + + expect(err.code).toBe('VALIDATION_ERROR'); + expect(err.status).toBe(400); + expect(err.message).toContain(`'cursor' is not a continuation token`); + }); + + it('issues a cursor the contract codec can read back', async () => { + const page = await backend.adapter.list!('bulk/', { limit: 10 }); + + expect(page.nextCursor).toBe(encodeStorageListCursor(page.items.at(-1)!.key)); + }); +}); + +// --------------------------------------------------------------------------- +// The halves only one backend can express — same semantic, two shapes +// --------------------------------------------------------------------------- + +describe('list() — backend-specific expressions of the shared semantics', () => { + it('local: the adapter\'s own .parts multipart staging is not a stored object', async () => { + const backend = await makeLocalBackend(); + try { + await backend.seed(['real.bin']); + // Written the way `initiateChunkedUpload` writes it, so this pins the + // real staging layout rather than a stand-in. + const uploadId = await backend.adapter.initiateChunkedUpload!('pending.bin'); + await backend.adapter.uploadChunk!(uploadId, 1, Buffer.from('chunk')); + + const page = await backend.adapter.list!(''); + + expect(page.items.map((i) => i.key)).toEqual(['real.bin']); + } finally { + await backend.dispose(); + } + }); + + it('s3: a zero-byte directory marker is skipped, as a filesystem directory is', async () => { + const backend = await makeS3Backend(); + try { + await backend.seed(['a/b/c.txt']); + // What the AWS console creates when you "make a folder". The local + // backend cannot represent it at all, so returning it here would be a + // per-backend dialect — the exact defect class that retired `list`. + fakeS3.objects.set('a/', { size: 0, lastModified: new Date(0) }); + fakeS3.objects.set('a/b/', { size: 0, lastModified: new Date(0) }); + + const page = await backend.adapter.list!(''); + + expect(page.items.map((i) => i.key)).toEqual(['a/b/c.txt']); + } finally { + await backend.dispose(); + } + }); + + it('s3: one list() call loops ListObjectsV2 rather than truncating at MaxKeys', async () => { + const backend = await makeS3Backend(); + try { + await backend.seed(BULK_SET); + fakeS3.listInputs.length = 0; + + const page = await backend.adapter.list!('bulk/', { limit: 1500 }); + + expect(page.items).toHaveLength(1500); + // 1000 (the ceiling) + 500. A single request here is the #5266 defect. + expect(fakeS3.listInputs).toHaveLength(2); + expect(fakeS3.listInputs[0]!.MaxKeys).toBe(1000); + expect(fakeS3.listInputs[1]!.MaxKeys).toBe(500); + // Intra-call paging uses the S3 continuation token... + expect(fakeS3.listInputs[1]!.ContinuationToken).toBe('ct-1'); + // ...and that opaque handle never escapes as the caller-facing cursor. + expect(page.nextCursor).not.toBe('ct-1'); + expect(page.nextCursor).toBe(encodeStorageListCursor(BULK_SET[1499]!)); + } finally { + await backend.dispose(); + } + }, 60_000); + + it('s3: a caller-supplied cursor resumes through StartAfter, not a leaked token', async () => { + const backend = await makeS3Backend(); + try { + await backend.seed(BULK_SET); + const first = await backend.adapter.list!('bulk/', { limit: 5 }); + fakeS3.listInputs.length = 0; + + const second = await backend.adapter.list!('bulk/', { limit: 5, cursor: first.nextCursor }); + + expect(fakeS3.listInputs[0]!.StartAfter).toBe(BULK_SET[4]); + expect(fakeS3.listInputs[0]!.ContinuationToken).toBeUndefined(); + expect(second.items.map((i) => i.key)).toEqual(BULK_SET.slice(5, 10)); + } finally { + await backend.dispose(); + } + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// The comparison the retired `list` never had: both backends, same key set +// --------------------------------------------------------------------------- + +describe('local and s3 agree on an identical key set', () => { + const KEYS = [ + 'a/b/c.txt', + 'a/b/d.txt', + 'a/z.txt', + 'ab.txt', + 'a.txt', + 'b/1.txt', + 'b/2.txt', + 'c.txt', + ]; + + let local: Backend; + let s3: Backend; + + beforeAll(async () => { + local = await makeLocalBackend(); + s3 = await makeS3Backend(); + await local.seed(KEYS); + await s3.seed(KEYS); + }); + + afterAll(async () => { + await local.dispose(); + await s3.dispose(); + }); + + it.each([ + ['everything', ''], + ['a bare prefix that also matches siblings', 'a'], + ['a folder-scoped prefix', 'a/'], + ['a prefix matching one key', 'c.txt'], + ['a prefix matching nothing', 'zzz'], + ])('page-by-page equality for %s', async (_label, prefix) => { + const localPages = await pageThrough(local.adapter, prefix, 2); + const s3Pages = await pageThrough(s3.adapter, prefix, 2); + + // Keys AND cursors. Cursor equality is the strong half: it proves the two + // backends encode the same continuation, so a `SwappableStorageService` + // swap mid-sweep resumes where it left off instead of silently restarting. + expect(localPages).toEqual(s3Pages); + }); + + it('the two backends observe the same total key set', async () => { + const localKeys = (await local.adapter.list!('')).items.map((i) => i.key); + const s3Keys = (await s3.adapter.list!('')).items.map((i) => i.key); + + expect(localKeys).toEqual(s3Keys); + expect(new Set(localKeys)).toEqual(new Set(KEYS)); + }); + + it('a cursor issued by one backend is accepted by the other', async () => { + const fromLocal = await local.adapter.list!('', { limit: 3 }); + const continuedOnS3 = await s3.adapter.list!('', { limit: 3, cursor: fromLocal.nextCursor }); + + const fromS3 = await s3.adapter.list!('', { limit: 3 }); + const continuedOnLocal = await local.adapter.list!('', { limit: 3, cursor: fromS3.nextCursor }); + + expect(continuedOnS3.items.map((i) => i.key)).toEqual( + continuedOnLocal.items.map((i) => i.key), + ); + expect(continuedOnS3.items.map((i) => i.key)).toEqual(KEYS.slice().sort().slice(3, 6)); + }); +}); diff --git a/packages/services/service-storage/src/swappable-storage-service.test.ts b/packages/services/service-storage/src/swappable-storage-service.test.ts index 04de3e706a..84be489d8a 100644 --- a/packages/services/service-storage/src/swappable-storage-service.test.ts +++ b/packages/services/service-storage/src/swappable-storage-service.test.ts @@ -1,7 +1,17 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import type { IStorageService, StorageFileInfo } from '@objectstack/spec/contracts'; +import type { + IStorageService, + StorageFileInfo, + StorageListOptions, + StorageListPage, +} from '@objectstack/spec/contracts'; +import { + decodeStorageListCursor, + encodeStorageListCursor, + resolveStorageListLimit, +} from '@objectstack/spec/contracts'; import { SwappableStorageService } from './swappable-storage-service'; class FakeAdapter implements IStorageService { @@ -24,9 +34,28 @@ class FakeAdapter implements IStorageService { if (!b) throw new Error('not found'); return { key, size: b.length, lastModified: new Date(), contentType: 'application/octet-stream' }; } - // No `list(prefix)`: the contract dropped it in #5540 and the shipped adapters - // dropped their implementations in #5541, so a fake that still advertised one - // would model a surface no real adapter has. + /** + * Cursor-shaped `list` (#6781), built on the contract's own helpers — the + * same ones both shipped adapters use, so this fake cannot model a cursor + * dialect no real adapter has. + */ + async list(prefix: string, options?: StorageListOptions): Promise { + const limit = resolveStorageListLimit(options?.limit); + const after = options?.cursor === undefined ? undefined : decodeStorageListCursor(options.cursor); + const matched = [...this.store.keys()] + .filter((key) => key.startsWith(prefix)) + .filter((key) => after === undefined || key > after) + .sort((x, y) => (x < y ? -1 : x > y ? 1 : 0)); + const page = matched.slice(0, limit); + const items = page.map((key) => ({ + key, + size: this.store.get(key)!.length, + lastModified: new Date(0), + })); + return matched.length > limit + ? { items, nextCursor: encodeStorageListCursor(page[page.length - 1]!) } + : { items }; + } } /** Adapter that omits the optional methods to exercise the proxy's @@ -84,9 +113,45 @@ describe('SwappableStorageService', () => { await expect(proxy.getSignedUrl('k', 60)).rejects.toThrow(/does not support getSignedUrl/); await expect(proxy.getPresignedUpload('k', 60)).rejects.toThrow(/does not support getPresignedUpload/); await expect(proxy.initiateChunkedUpload('k')).rejects.toThrow(/does not support initiateChunkedUpload/); + await expect(proxy.list('k')).rejects.toThrow(/does not support list/); + }); + + // The `forwards list() to the active adapter when supported` case was deleted + // with the proxy method it exercised in #5540 and is restored here with the + // cursor shape (#6781). + it('forwards list() to the active adapter, cursor and all', async () => { + const a = new FakeAdapter('A'); + for (const key of ['docs/a.txt', 'docs/b.txt', 'docs/c.txt', 'images/x.png']) { + await a.upload(key, Buffer.from(key)); + } + const proxy = new SwappableStorageService(a); + + const first = await proxy.list('docs/', { limit: 2 }); + expect(first.items.map((i) => i.key)).toEqual(['docs/a.txt', 'docs/b.txt']); + expect(first.nextCursor).toBeDefined(); + + const second = await proxy.list('docs/', { limit: 2, cursor: first.nextCursor }); + expect(second.items.map((i) => i.key)).toEqual(['docs/c.txt']); + expect(second.nextCursor).toBeUndefined(); }); - // The `forwards list() to the active adapter when supported` case was - // deleted with the proxy method it exercised (#5540): IStorageService no - // longer declares `list`, so there is nothing for the proxy to forward. + it('a cursor issued before swap() still resumes after it', async () => { + // Only true because the cursor codec lives on the CONTRACT rather than in + // each adapter. A per-adapter token would either be refused here or — + // worse — silently restart the sweep at key zero. + const a = new FakeAdapter('A'); + const b = new FakeAdapter('B'); + for (const key of ['k/1', 'k/2', 'k/3', 'k/4']) { + await a.upload(key, Buffer.from(key)); + await b.upload(key, Buffer.from(key)); + } + + const proxy = new SwappableStorageService(a); + const first = await proxy.list('k/', { limit: 2 }); + proxy.swap(b); + const second = await proxy.list('k/', { limit: 2, cursor: first.nextCursor }); + + expect(first.items.map((i) => i.key)).toEqual(['k/1', 'k/2']); + expect(second.items.map((i) => i.key)).toEqual(['k/3', 'k/4']); + }); }); diff --git a/packages/services/service-storage/src/swappable-storage-service.ts b/packages/services/service-storage/src/swappable-storage-service.ts index 5690ead032..a62f31fa1d 100644 --- a/packages/services/service-storage/src/swappable-storage-service.ts +++ b/packages/services/service-storage/src/swappable-storage-service.ts @@ -3,6 +3,8 @@ import type { IStorageService, StorageFileInfo, + StorageListOptions, + StorageListPage, StorageUploadOptions, PresignedUploadDescriptor, PresignedDownloadDescriptor, @@ -71,10 +73,23 @@ export class SwappableStorageService implements IStorageService { return this.inner.getInfo(key); } - // `list(prefix)` was removed from IStorageService in #5540 (ADR-0049 - // enforce-or-remove; analysis #5266), so there is no contract member left to - // forward to. The adapters' own `list` implementations are retired - // separately in #5541. + /** + * Forward cursor-shaped prefix enumeration to the active adapter (#6781). + * + * ⚠️ A `nextCursor` stays valid across a `swap()` only because both shipped + * adapters encode the same thing — the last key, resumed in ascending key + * order. An adapter whose cursor means something else would resume a + * half-finished sweep in the wrong place, silently; that is why the cursor + * codec lives on the contract (`encodeStorageListCursor`) instead of in each + * adapter, and why a token an adapter did not issue is refused rather than + * treated as "start from the beginning". + */ + list(prefix: string, options?: StorageListOptions): Promise { + if (typeof this.inner.list !== 'function') { + return Promise.reject(new Error('Active storage adapter does not support list()')); + } + return this.inner.list(prefix, options); + } getSignedUrl(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise { if (typeof this.inner.getSignedUrl !== 'function') { diff --git a/packages/spec/api-surface/contracts.json b/packages/spec/api-surface/contracts.json index 4f51224719..205b12e561 100644 --- a/packages/spec/api-surface/contracts.json +++ b/packages/spec/api-surface/contracts.json @@ -61,6 +61,7 @@ "CryptoContext (interface)", "CryptoHandle (interface)", "CubeMeta (interface)", + "DEFAULT_STORAGE_LIST_LIMIT (const)", "DatasetCompareTo (interface)", "DatasetSelection (interface)", "DefineSharingRuleInput (interface)", @@ -271,6 +272,8 @@ "StartupOptions (type)", "StartupOptionsParsed (type)", "StorageFileInfo (interface)", + "StorageListOptions (interface)", + "StorageListPage (interface)", "StorageUploadOptions (interface)", "StrategyContext (interface)", "SubscribeOptions (interface)", @@ -287,6 +290,9 @@ "UploadArtifactResult (interface)", "UserModelMessage (type)", "ValidationResult (type)", - "WriteObservabilityOptions (interface)" + "WriteObservabilityOptions (interface)", + "decodeStorageListCursor (function)", + "encodeStorageListCursor (function)", + "resolveStorageListLimit (function)" ] } diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 3bd80030b9..43eee52a44 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -645,7 +645,7 @@ }, { "surface": "contracts.IStorageService.list", - "replacement": "no replacement — track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket", + "replacement": "track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket — and where no such record exists, the cursor-shaped `list(prefix, { cursor, limit })` this entry reserved, restored in #6781", "migrationId": "storage-service-list-retired", "toMajor": 17, "rationale": "`list(prefix)` was an OPTIONAL contract method documented as \"List files in a directory/prefix\", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the \"all files\" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266)." @@ -1416,7 +1416,7 @@ }, { "surface": "contracts.IStorageService.list", - "replacement": "no replacement — track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket", + "replacement": "track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket — and where no such record exists, the cursor-shaped `list(prefix, { cursor, limit })` this entry reserved, restored in #6781", "migrationId": "storage-service-list-retired", "toMajor": 17, "rationale": "`list(prefix)` was an OPTIONAL contract method documented as \"List files in a directory/prefix\", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the \"all files\" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266)." diff --git a/packages/spec/src/contracts/storage-service.test.ts b/packages/spec/src/contracts/storage-service.test.ts index 5474ed48bc..3392da86a7 100644 --- a/packages/spec/src/contracts/storage-service.test.ts +++ b/packages/spec/src/contracts/storage-service.test.ts @@ -1,5 +1,15 @@ import { describe, it, expect } from 'vitest'; -import type { IStorageService, StorageFileInfo } from './storage-service'; +import type { + IStorageService, + StorageFileInfo, + StorageListPage, +} from './storage-service'; +import { + DEFAULT_STORAGE_LIST_LIMIT, + decodeStorageListCursor, + encodeStorageListCursor, + resolveStorageListLimit, +} from './storage-service'; describe('Storage Service Contract', () => { it('should allow a minimal IStorageService implementation with required methods', () => { @@ -109,53 +119,153 @@ describe('Storage Service Contract', () => { }); // --------------------------------------------------------------------- - // Retirement pin — `list?(prefix)` removed in #5540 (ADR-0049 - // enforce-or-remove; the two-dialect analysis is #5266). + // Contract pin — `list(prefix, { cursor, limit })`. // - // `IStorageService` is a pure TypeScript contract: nothing parses it, so - // this retirement has no `retiredKey()` tombstone and no parse-time - // prescription to assert (spec-property-retirement §2, "nothing parses it" - // route). tsc is the only channel the removal has — and `tsconfig.test.json` - // puts this file in front of tsc (#5286), so the two `@ts-expect-error` - // directives below are real checks that go red the day the member returns, - // not phantom ones. Restoring `list?()` to the interface turns both into - // "unused '@ts-expect-error' directive". + // These three cases are the FLIP of the #5540 retirement pins that stood + // here (#6781): the single-argument `list?(prefix)` stayed retired, and the + // cursor-shaped member the retirement notes reserved took its place. The + // pins are kept rather than deleted because the load they bear only moved — + // it used to be "the retired shape has not crept back", it is now "the + // restored shape is the cursor one and NOT the array one". // - // The test these replaced exercised the removed member itself; keeping it - // green would have meant keeping the member. + // `IStorageService` is a pure TypeScript contract: nothing parses it, so + // tsc is the only channel it has — and `tsconfig.test.json` puts this file + // in front of tsc (#5286), so the `@ts-expect-error` directive below is a + // real check that goes red the day the old array shape returns, not a + // phantom one. // --------------------------------------------------------------------- - it('no longer declares list(prefix) — reading it is a type error', () => { + it('declares list(prefix, { cursor, limit }) returning a page plus a cursor', async () => { + const stored = ['a/b/c.txt', 'a/b/d.txt', 'ab.txt']; + const storage: IStorageService = { upload: async () => {}, download: async () => Buffer.from(''), delete: async () => {}, exists: async () => true, getInfo: async (key) => ({ key, size: 0, lastModified: new Date() }), + list: async (prefix, options): Promise => { + const limit = resolveStorageListLimit(options?.limit); + const after = options?.cursor ? decodeStorageListCursor(options.cursor) : undefined; + const matched = stored + .filter((key) => key.startsWith(prefix)) + .filter((key) => after === undefined || key > after) + .sort(); + const page = matched.slice(0, limit); + const items = page.map((key) => ({ key, size: 0, lastModified: new Date() })); + return matched.length > limit + ? { items, nextCursor: encodeStorageListCursor(page[page.length - 1]!) } + : { items }; + }, }; - // @ts-expect-error — `list` was removed from IStorageService (#5540). - // Prefix enumeration returns cursor-shaped when a caller needs it: - // `list(prefix, { cursor, limit })`. It is not on the contract today. - const retired = storage.list; + const first = await storage.list!('a', { limit: 2 }); + expect(first.items.map((i) => i.key)).toEqual(['a/b/c.txt', 'a/b/d.txt']); + expect(first.nextCursor).toBeDefined(); - expect(retired).toBeUndefined(); + const second = await storage.list!('a', { limit: 2, cursor: first.nextCursor }); + // `list('a')` is a RAW prefix match, so `ab.txt` is in scope — the one + // semantic a caller most often assumes away. + expect(second.items.map((i) => i.key)).toEqual(['ab.txt']); + expect(second.nextCursor).toBeUndefined(); }); - it('no longer accepts an implementation that declares list(prefix)', () => { + it('still rejects the retired array shape — list(prefix) returning StorageFileInfo[]', () => { const storage: IStorageService = { upload: async () => {}, download: async () => Buffer.from(''), delete: async () => {}, exists: async () => true, getInfo: async (key) => ({ key, size: 0, lastModified: new Date() }), - // @ts-expect-error — excess property: the contract has no `list` member, - // so an adapter can no longer advertise one through it (#5540). + // @ts-expect-error — the restored member is cursor-shaped. An adapter + // carrying the #5540-retired `(prefix) => StorageFileInfo[]` is a type + // error, not a tolerated dialect (#6781). list: async (_prefix: string): Promise => [], }; expect(typeof storage.getInfo).toBe('function'); }); + it('list stays OPTIONAL — an adapter that cannot enumerate still satisfies the contract', () => { + // The restoration is additive (minor). Making `list` required would break + // every third-party adapter, which is a major-version act this card is not. + const storage: IStorageService = { + upload: async () => {}, + download: async () => Buffer.from(''), + delete: async () => {}, + exists: async () => true, + getInfo: async (key) => ({ key, size: 0, lastModified: new Date() }), + }; + + expect(storage.list).toBeUndefined(); + }); + + // --------------------------------------------------------------------- + // The shared argument discipline. It lives on the CONTRACT precisely so + // two adapters cannot answer the same bad argument two ways — the failure + // mode that retired the old `list` (#5266). + // --------------------------------------------------------------------- + describe('resolveStorageListLimit', () => { + it('defaults an omitted limit to DEFAULT_STORAGE_LIST_LIMIT', () => { + expect(resolveStorageListLimit(undefined)).toBe(DEFAULT_STORAGE_LIST_LIMIT); + expect(DEFAULT_STORAGE_LIST_LIMIT).toBe(1000); + }); + + it('passes a positive integer through', () => { + expect(resolveStorageListLimit(1)).toBe(1); + expect(resolveStorageListLimit(2500)).toBe(2500); + }); + + it.each([ + ['zero', 0], + ['negative', -1], + ['fractional', 1.5], + ['NaN', Number.NaN], + ['Infinity', Number.POSITIVE_INFINITY], + ])('refuses a %s limit with the ADR-0112 envelope', (_label, limit) => { + // Rejection-class: assert the ENVELOPE (code + status), not merely that + // something threw. A bare `toThrow()` cannot tell a refusal apart from a + // clamp that later blew up somewhere else. + let caught: (Error & { code?: string; status?: number }) | undefined; + try { + resolveStorageListLimit(limit); + } catch (err) { + caught = err as Error & { code?: string; status?: number }; + } + + expect(caught).toBeInstanceOf(Error); + expect(caught?.code).toBe('VALIDATION_ERROR'); + expect(caught?.status).toBe(400); + expect(caught?.message).toContain(`'limit' must be a positive integer`); + }); + }); + + describe('storage list cursor codec', () => { + it('round-trips a key', () => { + for (const key of ['a/b/c.txt', 'tenants/t-1/файл.bin', 'x'.repeat(300)]) { + expect(decodeStorageListCursor(encodeStorageListCursor(key))).toBe(key); + } + }); + + it('refuses a token it did not issue with the ADR-0112 envelope', () => { + // Node's base64url decoder DROPS characters it does not recognise, so a + // corrupted token would otherwise decode to a different key and resume + // the sweep in the wrong place. Re-encoding catches that. + for (const bogus of ['not a cursor', '', 'YQ==', '!!!!']) { + let caught: (Error & { code?: string; status?: number }) | undefined; + try { + decodeStorageListCursor(bogus); + } catch (err) { + caught = err as Error & { code?: string; status?: number }; + } + + expect(caught, `expected "${bogus}" to be refused`).toBeInstanceOf(Error); + expect(caught?.code).toBe('VALIDATION_ERROR'); + expect(caught?.status).toBe(400); + expect(caught?.message).toContain(`'cursor' is not a continuation token`); + } + }); + }); + it('should generate signed URLs', async () => { const storage: IStorageService = { upload: async () => {}, diff --git a/packages/spec/src/contracts/storage-service.ts b/packages/spec/src/contracts/storage-service.ts index eac0434180..55af13ab62 100644 --- a/packages/spec/src/contracts/storage-service.ts +++ b/packages/spec/src/contracts/storage-service.ts @@ -13,6 +13,8 @@ * Aligned with CoreServiceName 'file-storage' in core-services.zod.ts. */ +import type { StandardErrorCode } from '../api/errors.zod'; + /** * Options for uploading a file */ @@ -41,6 +43,140 @@ export interface StorageFileInfo { metadata?: Record; } +/** + * Options for one page of `IStorageService.list()`. + * + * The whole point of this shape is that a truncated answer is IMPOSSIBLE to + * mistake for a complete one — the defect that retired the old `list(prefix)` + * (#5266 / #5540): the S3 adapter stopped at 1000 objects and returned an + * array indistinguishable from "that is all of them". + */ +export interface StorageListOptions { + /** + * Continuation token taken verbatim from a previous page's `nextCursor`. + * Omit for the first page. + * + * OPAQUE: never construct, parse, compare or persist-and-reinterpret one. + * It encodes a position in the backend's key order, and an adapter refuses + * a token it cannot decode (`VALIDATION_ERROR` / 400) rather than silently + * restarting from the beginning — a silent restart is how a paging sweep + * loops forever. + */ + cursor?: string; + /** + * Maximum number of items in the returned page. Must be a positive + * integer; anything else is refused (`VALIDATION_ERROR` / 400) rather than + * clamped. Defaults to {@link DEFAULT_STORAGE_LIST_LIMIT}. + * + * Adapters resolve it through {@link resolveStorageListLimit} so every + * backend answers an invalid `limit` the same way. + */ + limit?: number; +} + +/** + * One page of `IStorageService.list()`. + */ +export interface StorageListPage { + /** + * The page's files, in ascending key order. + * + * A page is FULL (`items.length === limit`) unless it is the last one, so + * an adapter that has to make several backend round-trips to fill a page + * makes them — a caller never sees a short page that merely reflects the + * backend's own page size. + */ + items: StorageFileInfo[]; + /** + * Continuation token for the next page — present **if and only if** more + * items remain. + * + * ⚠️ Page until `nextCursor` is absent. Never infer completeness from + * `items.length < limit`: under concurrent deletion a final `stat` can drop + * an item from a page that still has a successor. + */ + nextCursor?: string; +} + +/** + * Page size used when `StorageListOptions.limit` is omitted. + * + * 1000 is deliberately the same number as S3's `ListObjectsV2` `MaxKeys` cap — + * the cap that used to truncate `list(prefix)` in silence. Here it truncates + * nothing: the page comes back with a `nextCursor`, so the caller is told there + * is more instead of having to guess. + */ +export const DEFAULT_STORAGE_LIST_LIMIT = 1000; + +/** + * Error code every adapter uses to refuse a malformed `list()` argument. + * + * Typed as `StandardErrorCode` so a misspelling fails `tsc` rather than + * shipping a code `ApiErrorSchema` rejects (ADR-0112). + */ +const STORAGE_LIST_INVALID_ARGUMENT: StandardErrorCode = 'VALIDATION_ERROR'; + +/** An ADR-0112-enveloped refusal: `VALIDATION_ERROR` / 400. */ +function storageListRefusal(message: string): Error { + const err = new Error(message) as Error & { code?: string; status?: number }; + err.code = STORAGE_LIST_INVALID_ARGUMENT; + err.status = 400; + return err; +} + +/** + * Resolve `StorageListOptions.limit` into the page size an adapter must honour. + * + * Lives on the CONTRACT, not in each adapter, for the reason `list` was retired + * in the first place: two adapters validating independently is two dialects + * waiting to happen. Every backend — including third-party ones — gets the + * default and the refusal from here. + * + * @throws `VALIDATION_ERROR` / 400 when `limit` is present and is not a + * positive integer. Refused, never clamped: a clamped `limit: 0` would answer + * an empty page that reads exactly like "this prefix is empty". + */ +export function resolveStorageListLimit(limit: number | undefined): number { + if (limit === undefined) return DEFAULT_STORAGE_LIST_LIMIT; + if (!Number.isInteger(limit) || limit < 1) { + throw storageListRefusal( + `storage list: 'limit' must be a positive integer (received ${String(limit)}).`, + ); + } + return limit; +} + +/** + * Encode a resume position (the last key of a page) as a `nextCursor`. + * + * Shared by every adapter so a cursor means ONE thing across backends: "resume + * strictly after this key, in ascending key order". base64url is not security — + * it exists so the token cannot be confused with a storage key and hand-built + * by a caller who then depends on the encoding. + */ +export function encodeStorageListCursor(key: string): string { + return Buffer.from(key, 'utf8').toString('base64url'); +} + +/** + * Decode a `StorageListOptions.cursor` back into a resume key. + * + * @throws `VALIDATION_ERROR` / 400 when the token is not one this contract + * issued. Node's base64url decoder is lenient — it drops characters it does not + * recognise — so the decode is verified by re-encoding: without that check a + * corrupted token silently decodes to a DIFFERENT key and the sweep resumes in + * the wrong place, which is worse than refusing. + */ +export function decodeStorageListCursor(cursor: string): string { + const decoded = Buffer.from(cursor, 'base64url').toString('utf8'); + if (decoded === '' || Buffer.from(decoded, 'utf8').toString('base64url') !== cursor) { + throw storageListRefusal( + `storage list: 'cursor' is not a continuation token issued by this contract.`, + ); + } + return decoded; +} + /** * Descriptor returned by `IStorageService.getPresignedUpload()`. * @@ -123,22 +259,59 @@ export interface IStorageService { */ getInfo(key: string): Promise; - // `list?(prefix: string): Promise` was REMOVED in - // @objectstack/spec 5.x (#5540, ADR-0049 enforce-or-remove; analysis in - // #5266). It had no consumer — the only in-repo call site was a proxy - // pass-through — and the two shipped adapters answered the same call with - // two different semantics, both silently incomplete: the local adapter - // listed one level and reported directories as files, the S3 adapter - // recursed and truncated at 1000 objects without reading - // `IsTruncated`/`ContinuationToken`. One contract method, two dialects, - // no signal. - // - // There is no replacement, deliberately: a prefix enumeration that cannot - // paginate is the wrong shape to inherit. When a real caller needs one, it - // comes back cursor-shaped — `list(prefix, { cursor, limit })` returning a - // page plus a continuation token — with adapter-conformance cases (nested - // keys, directory entries, >1000 objects) proving both backends agree. - // Until then the storage contract only exposes per-key operations. + /** + * Enumerate the stored files whose key begins with `prefix`, one page at a + * time. + * + * ## Lineage — read this before changing the signature + * + * The single-argument `list?(prefix): Promise< StorageFileInfo[] >` was + * retired in #5540 / #5541 (ADR-0049 enforce-or-remove; the two-dialect + * measurement is #5266): the local adapter listed ONE level deep and pushed + * directories into the result as files, while the S3 adapter recursed and + * stopped at 1000 objects without reading `IsTruncated` — one method, two + * silently-incomplete answers. Both retirement notes reserved exactly one + * route back, and this is it (#6781, cloud#1203 maintainer ruling option B): + * cursor-shaped, with adapter-conformance cases proving both backends agree. + * + * ## The semantics an adapter must implement + * + * 1. **`prefix` is a raw key-string prefix, matched recursively** — S3 + * `ListObjectsV2` semantics, which the local adapter emulates rather than + * the other way round. `list('a')` returns `a/b/c` AND `ab.txt`. ⚠️ To + * scope to a folder, pass the trailing slash: `list('a/')`. `list('t/1')` + * also matches `t/10/...`, which for a DELETING sweep is the difference + * between reclaiming one tenant and reclaiming eleven. + * 2. **Only files come back.** Filesystem directories are never stat'd into + * results, and an S3 zero-byte directory marker (a key ending in `/`) is + * skipped — neither is downloadable, and returning one on one backend + * only is precisely the old dialect split. + * 3. **Ascending key order**, stable across pages. + * 4. **Pages are full** (`items.length === limit`) except the last: an + * adapter loops over its backend's own paging to fill one page. + * 5. **`nextCursor` is present iff more items remain** (see + * {@link StorageListPage}). + * 6. **No duplicates and no gaps** across a pagination run over an unchanged + * key set. Keys written or deleted mid-run may or may not appear. + * 7. **`limit` and `cursor` are validated, not coerced** — see + * {@link resolveStorageListLimit} and {@link decodeStorageListCursor}, + * which every adapter shares so the refusals cannot diverge. + * + * Optional, like every other capability on this contract: an adapter that + * cannot enumerate simply omits it, and `SwappableStorageService` answers a + * clear "does not support list()" for it. It is NOT a required member on + * purpose — requiring it would break every third-party adapter, which is a + * major-version act, and enumeration is genuinely optional for a backend. + * + * ⚠️ Prefer enumerating the RECORDS you wrote (`sys_file` / file-reference + * rows, paginated through ObjectQL) when they exist. Bucket enumeration is + * for the cases where they do not: reclaiming storage a deleted tenant left + * behind, or GC-ing blobs whose index is the thing that went away. + * + * @param prefix - Raw key prefix; `''` enumerates everything. + * @param options - Page size and continuation token. + */ + list?(prefix: string, options?: StorageListOptions): Promise; /** * Generate a pre-signed URL for temporary access diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 95363245d2..1fd40f434c 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -2266,8 +2266,10 @@ const step17: MigrationStep = { id: 'storage-service-list-retired', surface: 'contracts.IStorageService.list', replacement: - 'no replacement — track the keys you wrote (sys_file / file-reference records, ' - + 'queryable through ObjectQL with real pagination) instead of enumerating the bucket', + 'track the keys you wrote (sys_file / file-reference records, queryable through ' + + 'ObjectQL with real pagination) instead of enumerating the bucket — and where no ' + + 'such record exists, the cursor-shaped `list(prefix, { cursor, limit })` this ' + + 'entry reserved, restored in #6781', reason: '`list(prefix)` was an OPTIONAL contract method documented as "List files in a ' + 'directory/prefix", and the two shipped adapters answered the same call with two ' @@ -2311,7 +2313,19 @@ const step17: MigrationStep = { + 'contract, so deleting it is cleanup that can follow. The break is on the CALLER ' + 'side: `storage.list(...)` no longer type-checks, and a PROXY typed against ' + '`IStorageService` that forwards to `inner.list` is exactly such a caller — the ' - + 'one in `@objectstack/service-storage` goes with the adapters (#5541).', + + 'one in `@objectstack/service-storage` goes with the adapters (#5541). ' + + '⚠️ AMENDED 2026-08-09 (#6781, maintainer ruling on cloud#1203, option B): the ' + + 'RESERVED route in the paragraph above was taken. `list` exists again on the ' + + 'contract, cursor-shaped — `list(prefix, { cursor, limit })` returning ' + + '`{ items, nextCursor }` — because cloud had two first-party callers this repo ' + + 'could not see when the measurement said "nothing calls it" (tenant attachment ' + + 'reclamation, marketplace snapshot GC). This does NOT un-retire anything and the ' + + 'acceptance criterion above is unchanged for what it actually governs: the ' + + 'single-argument `list(prefix): StorageFileInfo[]` is gone for good, a call written ' + + 'against it still fails to compile, and the two dialects it had are now pinned ' + + 'against each other in `storage-adapter-list.conformance.test.ts` rather than left ' + + 'to diverge. What changed for an upgrader is only the destination: prefer the ' + + 'records you wrote, and reach for the restored member when there are none.', }, { id: 'driver-aggregate-undeclared-key-aliases-removed',