From 8cf4d0fc0f8417280a896f2214a6550b35e8668c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 13:15:03 +0000 Subject: [PATCH] =?UTF-8?q?fix(cli,service-storage):=20stop=20re-pointing?= =?UTF-8?q?=20the=20storage=20adapter=20every=20boot=20=E2=80=94=20and=20m?= =?UTF-8?q?ean=20it=20when=20it=20warns=20(#4096)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every `os dev` / `os serve` boot printed "storage adapter swapped (LocalStorageAdapter → LocalStorageAdapter). Existing files were NOT migrated and may be unreachable through the new adapter." The warning was telling the truth. `serve` constructed the plugin with `{ driver: 'local', root }` and `StorageServicePluginOptions` declares neither key, so both were dropped silently: the plugin applied its own `./storage` default, OS_STORAGE_ROOT changed nothing, and uploads landed in a directory nobody named. The `storage` settings namespace then corrected the root on its first read — its manifest default is `./.objectstack/data/uploads` — genuinely moving the backing store, every boot, forever. Three defects, three fixes: - serve passes options the plugin reads: `{ adapter: 'local', local: { rootDir } }`. OS_STORAGE_ROOT takes effect and local uploads land under `.objectstack/data/uploads` from the first byte. Extracted as `resolveStorageCapabilityArg` so the option SHAPE is pinned — a mismatch like this type-checks fine as an argument and does nothing at runtime. - A swap is skipped when nothing changed. The plugin records what the running adapter points at and compares resolved CONFIGURATIONS, rather than rebuilding whenever the settings namespace held any value at all (true on every boot once that namespace has persisted its own defaults). - The warning means what it says: it fires when the BACKING STORE moved, not when the adapter object was replaced. A credential rotation swaps so the new key takes effect and logs at info — same bucket, nothing stranded. A swap from a caller that resolved no target still warns; ignorance must not silence it. Path spellings are normalised, so the platform writing one default two ways (`./.objectstack/data/uploads` vs `.objectstack/data/uploads`) is not read as a migration between a directory and itself. Verified on examples/app-todo: boot diagnostics went from four warnings to three, the storage line is gone, and `./storage` is no longer created. 19 unit cases on the target resolver and the swap/warn split, 4 plugin-level cases on what a boot does and says, 7 on the CLI option shape. service-storage 241 tests, cli 917 tests, eslint clean, tsc error count unchanged from main. `config.storage` authored with the `driver`/`root` dialect is still forwarded verbatim and still unread by the plugin — the same mismatch one layer up, filed separately rather than papered over with a lenient alias here (Prime Directive #12). Closes #4096 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HrRNgrWaRtggzmrHpbomyh --- .changeset/storage-adapter-swap-verdict.md | 58 ++++++ .../commands/serve-storage-capability.test.ts | 81 +++++++++ packages/cli/src/commands/serve.ts | 62 +++++-- .../src/storage-service-plugin.test.ts | 107 +++++++++++- .../src/storage-service-plugin.ts | 73 +++++++- .../src/storage-target.test.ts | 165 ++++++++++++++++++ .../service-storage/src/storage-target.ts | 135 ++++++++++++++ 7 files changed, 668 insertions(+), 13 deletions(-) create mode 100644 .changeset/storage-adapter-swap-verdict.md create mode 100644 packages/cli/src/commands/serve-storage-capability.test.ts create mode 100644 packages/services/service-storage/src/storage-target.test.ts create mode 100644 packages/services/service-storage/src/storage-target.ts diff --git a/.changeset/storage-adapter-swap-verdict.md b/.changeset/storage-adapter-swap-verdict.md new file mode 100644 index 0000000000..916afee8b4 --- /dev/null +++ b/.changeset/storage-adapter-swap-verdict.md @@ -0,0 +1,58 @@ +--- +'@objectstack/service-storage': patch +'@objectstack/cli': patch +--- + +**The storage adapter stops being rebuilt and re-pointed on every boot, and the +"files may be unreachable" warning stops firing at a healthy server (#4096).** + +Every `os dev` / `os serve` boot printed: + +``` +WARN StorageServicePlugin: storage adapter swapped (LocalStorageAdapter → +LocalStorageAdapter). Existing files were NOT migrated and may be unreachable +through the new adapter. +``` + +The warning was telling the truth. `serve` constructed the plugin with +`{ driver: 'local', root }` — and `StorageServicePluginOptions` declares +neither key. Both were dropped silently, so the plugin applied its own +`./storage` default, `OS_STORAGE_ROOT` changed nothing, and uploads landed in a +directory nobody named. The `storage` settings namespace then corrected the root +on its first read (its manifest default is `./.objectstack/data/uploads`), +genuinely moving the backing store — every boot, forever. + +Three fixes, because there were three defects: + +- **`serve` now passes options the plugin reads** — `{ adapter: 'local', + local: { rootDir } }`. `OS_STORAGE_ROOT` takes effect, and local uploads land + under `.objectstack/data/uploads` from the first byte instead of `./storage`. + Extracted as `resolveStorageCapabilityArg` so the option SHAPE is pinned by + tests: a mismatch like this type-checks fine and does nothing at runtime. +- **A swap is skipped when nothing changed.** The plugin records what the + running adapter points at and compares resolved configurations, instead of + rebuilding whenever the settings namespace held any value at all — which is + every boot once that namespace has persisted its own defaults. +- **The warning now means what it says.** It fires when the BACKING STORE moved + (kind change, different root, different bucket/region/endpoint), not merely + when the adapter object was replaced. A credential rotation swaps the adapter + so the new key takes effect and logs at info: same bucket, nothing stranded. + A swap from a caller that resolved no target still warns — ignorance must not + silence it. + +Path spellings are normalised, so the platform writing the same default two ways +(`./.objectstack/data/uploads` in the settings manifest, +`.objectstack/data/uploads` in the CLI) is no longer read as a migration between +a directory and itself. + +Verified on `examples/app-todo`: the boot-diagnostics block went from four +warnings to three, with the storage line gone and `./storage` no longer created. +19 unit cases cover the target resolver and the swap/warn split (including the +refusals), 4 plugin-level cases pin what a boot does and says, and 7 pin the CLI +option shape. + +`config.storage` authored with the `driver`/`root` dialect is still forwarded +verbatim and still not read by the plugin — the same mismatch one layer up. +Correcting it means deciding whether the plugin accepts that dialect or the +config schema is wrong, so it is filed rather than papered over with a lenient +alias here (AGENTS.md Prime Directive #12). diff --git a/packages/cli/src/commands/serve-storage-capability.test.ts b/packages/cli/src/commands/serve-storage-capability.test.ts new file mode 100644 index 0000000000..16f65594b5 --- /dev/null +++ b/packages/cli/src/commands/serve-storage-capability.test.ts @@ -0,0 +1,81 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * framework#4096 — what `StorageServicePlugin` is actually constructed with. + * + * The fallback used to be `{ driver: 'local', root }`, and + * `StorageServicePluginOptions` declares neither key. Both were dropped + * silently, so the plugin applied its own `./storage` default, + * `OS_STORAGE_ROOT` changed nothing, and uploads landed somewhere the operator + * never named. The `storage` settings namespace then corrected the root on its + * first read — its manifest default is `./.objectstack/data/uploads` — which + * swapped the adapter and warned "existing files were NOT migrated" on every + * boot of a healthy server. + * + * The warning was telling the truth; the configuration was wrong. These pin the + * option SHAPE, because a shape mismatch is exactly the failure a passing type + * check does not catch when the receiving interface has no index signature and + * the value is built as a plain object literal. + */ + +import { describe, it, expect } from 'vitest'; +import { resolveStorageCapabilityArg } from './serve.js'; + +describe('resolveStorageCapabilityArg', () => { + it('builds options StorageServicePlugin actually reads', () => { + // `adapter` + `local.rootDir` — NOT `driver` + `root`. + expect(resolveStorageCapabilityArg(undefined).options).toEqual({ + adapter: 'local', + local: { rootDir: '.objectstack/data/uploads' }, + }); + }); + + it('never emits the keys the plugin ignores', () => { + // The regression guard proper: `{driver, root}` type-checks fine as an + // argument and does nothing at runtime. + const { options } = resolveStorageCapabilityArg(undefined); + expect(options).not.toHaveProperty('driver'); + expect(options).not.toHaveProperty('root'); + }); + + it('honours OS_STORAGE_ROOT, which the old shape discarded', () => { + const { options, localRoot } = resolveStorageCapabilityArg(undefined, '/srv/uploads'); + expect(options).toEqual({ adapter: 'local', local: { rootDir: '/srv/uploads' } }); + expect(localRoot).toBe('/srv/uploads'); + }); + + it('ignores a blank or whitespace-only env root', () => { + for (const blank of ['', ' ']) { + expect(resolveStorageCapabilityArg(undefined, blank).options).toEqual({ + adapter: 'local', + local: { rootDir: '.objectstack/data/uploads' }, + }); + } + }); + + it('reports the local root so only the fallback triggers the production warning', () => { + // A host that configured its own backend must not be told it is on local disk. + expect(resolveStorageCapabilityArg(undefined).localRoot).toBe('.objectstack/data/uploads'); + expect(resolveStorageCapabilityArg({ adapter: 's3', s3: { bucket: 'b', region: 'r' } }).localRoot) + .toBeUndefined(); + }); + + it('forwards a host-configured storage block verbatim', () => { + const cfg = { adapter: 's3', s3: { bucket: 'b', region: 'r' } }; + expect(resolveStorageCapabilityArg(cfg).options).toBe(cfg); + // The `driver` dialect is still forwarded untouched — the plugin does not + // read it either, but rewriting it here would fossilize the wrong contract + // rather than fix it. Tracked separately. + const legacy = { driver: 's3', bucket: 'b' }; + expect(resolveStorageCapabilityArg(legacy).options).toBe(legacy); + }); + + it('falls back when the block names no backend at all', () => { + // `config.storage = { presignedTtl: 60 }` configures no backend, so the + // local default still applies rather than being replaced by a partial block. + expect(resolveStorageCapabilityArg({ presignedTtl: 60 }).options).toEqual({ + adapter: 'local', + local: { rootDir: '.objectstack/data/uploads' }, + }); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 07d210fdee..0864028a34 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -2190,17 +2190,15 @@ export default class Serve extends Command { // In production mode we emit a single loud warning so the // operator knows to point storage at S3 / GCS / Azure before // shipping (data on a single pod is volatile / non-replicated). - const cfgStorage = (config as any).storage; - if (cfgStorage && (cfgStorage.driver || cfgStorage.adapter)) { - arg = cfgStorage; - } else { - const root = process.env.OS_STORAGE_ROOT || '.objectstack/data/uploads'; - arg = { driver: 'local', root }; - if (!isDev) { - console.warn(chalk.yellow( - ` ⚠ StorageServicePlugin using local driver (${root}) — switch to S3/GCS/Azure for production (set config.storage or OS_STORAGE_*).`, - )); - } + const storageArg = resolveStorageCapabilityArg( + (config as any).storage, + process.env.OS_STORAGE_ROOT, + ); + arg = storageArg.options; + if (storageArg.localRoot && !isDev) { + console.warn(chalk.yellow( + ` ⚠ StorageServicePlugin using local driver (${storageArg.localRoot}) — switch to S3/GCS/Azure for production (set config.storage or OS_STORAGE_*).`, + )); } } await kernel.use(arg !== undefined ? new Ctor(arg) : new Ctor()); @@ -2649,6 +2647,48 @@ export default class Serve extends Command { } } +/** + * Constructor options for `StorageServicePlugin`, plus the local root to name in + * the production warning (absent when the host configured a backend itself). + */ +export interface StorageCapabilityArg { + options: Record; + localRoot?: string; +} + +/** + * Resolve what `StorageServicePlugin` is constructed with (#4096). + * + * Storage is in the default capability slate, so a host that configures nothing + * still gets local disk under `.objectstack/data/uploads/` and avatars / + * attachments / report files work out of the box. + * + * The fallback used to be `{ driver: 'local', root }` — neither of which + * `StorageServicePluginOptions` declares. Both were dropped on the floor, so the + * plugin applied its OWN default (`./storage`), `OS_STORAGE_ROOT` changed + * nothing, and uploads landed somewhere the operator never named. The `storage` + * settings namespace then corrected the root on its first read (its manifest + * default IS `./.objectstack/data/uploads`), which swapped the adapter and + * warned about stranded files — on every boot of a healthy server. + * + * A caller-supplied `config.storage` is still forwarded verbatim, including the + * `driver`/`root` dialect, which the plugin does not read either. That is the + * same mismatch one layer up and is tracked separately: correcting it means + * deciding whether the plugin accepts that dialect or the config schema is + * wrong, and a lenient alias here would fossilize the wrong contract + * (AGENTS.md Prime Directive #12). + */ +export function resolveStorageCapabilityArg( + cfgStorage: any, + envRoot?: string, +): StorageCapabilityArg { + if (cfgStorage && (cfgStorage.driver || cfgStorage.adapter)) { + return { options: cfgStorage }; + } + const rootDir = envRoot?.trim() || '.objectstack/data/uploads'; + return { options: { adapter: 'local', local: { rootDir } }, localRoot: rootDir }; +} + /** * Best-effort driver introspection. * diff --git a/packages/services/service-storage/src/storage-service-plugin.test.ts b/packages/services/service-storage/src/storage-service-plugin.test.ts index 79bd0bd39a..5b6027601b 100644 --- a/packages/services/service-storage/src/storage-service-plugin.test.ts +++ b/packages/services/service-storage/src/storage-service-plugin.test.ts @@ -18,8 +18,16 @@ import { SwappableStorageService } from './swappable-storage-service'; function makeCtx() { const services = new Map(); const hooks: Array<() => Promise | void> = []; + // Captured so tests can assert on what a boot SAID, not just what it built — + // #4096 is entirely about a warning that should not have been emitted. + const logs: { info: string[]; warn: string[]; error: string[] } = { info: [], warn: [], error: [] }; const ctx: any = { - logger: { info: () => {}, warn: () => {}, error: () => {} }, + logger: { + info: (m: string) => { logs.info.push(String(m)); }, + warn: (m: string) => { logs.warn.push(String(m)); }, + error: (m: string) => { logs.error.push(String(m)); }, + }, + _logs: logs, registerService: (name: string, svc: any) => { services.set(name, svc); }, getService: (name: string): T => { const s = services.get(name); @@ -121,6 +129,103 @@ describe('StorageServicePlugin: settings live-wire', () => { expect(proxy.getInner()).toBe(before); }); + // ── #4096: the swap decision, and what it says out loud ────────────────── + // + // `hasAny` is true on every boot once the settings service has persisted its + // own defaults, so this used to rebuild and swap the adapter unconditionally + // and warn that "existing files were NOT migrated" — about a swap from an + // adapter to an identically-configured one, on a healthy server, forever. + + it('neither swaps nor warns when persisted settings match the running adapter', async () => { + const dir = await fs.mkdtemp(join(tmpdir(), 'oss-same-')); + const plugin = new StorageServicePlugin({ + adapter: 'local', + local: { rootDir: dir }, + registerRoutes: false, + }); + const ctx = makeCtx(); + // Exactly what the settings namespace holds after it persists its defaults: + // values present (so `hasAny` is true) and identical to what is running. + ctx.registerService('settings', makeFakeSettings({ adapter: 'local', local_root: dir })); + + await plugin.init(ctx); + await plugin.start(ctx); + const proxy = ctx.getService('file-storage') as SwappableStorageService; + const before = proxy.getInner(); + + await ctx._flushReady(); + + expect(proxy.getInner()).toBe(before); + expect(ctx._logs.warn.join('\n')).not.toContain('adapter swapped'); + }); + + it('treats an implicit local root and its explicit default as the same store', async () => { + // The real boot shape: the host leaves `local.rootDir` unset while the + // settings namespace persists the schema default, so the two spellings of + // one location must not read as a move. + const plugin = new StorageServicePlugin({ adapter: 'local', registerRoutes: false }); + const ctx = makeCtx(); + ctx.registerService('settings', makeFakeSettings({ adapter: 'local', local_root: './storage' })); + + await plugin.init(ctx); + await plugin.start(ctx); + const proxy = ctx.getService('file-storage') as SwappableStorageService; + const before = proxy.getInner(); + + await ctx._flushReady(); + + expect(proxy.getInner()).toBe(before); + expect(ctx._logs.warn.join('\n')).not.toContain('adapter swapped'); + }); + + it('still warns when the backing store really moves', async () => { + // The guard has to survive the fix: Local → another root strands whatever + // the old one held, and that is the whole point of the message. + const dirA = await fs.mkdtemp(join(tmpdir(), 'oss-move-a-')); + const dirB = await fs.mkdtemp(join(tmpdir(), 'oss-move-b-')); + const plugin = new StorageServicePlugin({ + adapter: 'local', + local: { rootDir: dirA }, + registerRoutes: false, + }); + const ctx = makeCtx(); + ctx.registerService('settings', makeFakeSettings({ adapter: 'local', local_root: dirB })); + + await plugin.init(ctx); + await plugin.start(ctx); + await ctx._flushReady(); + + const warned = ctx._logs.warn.join('\n'); + expect(warned).toContain('adapter swapped'); + expect(warned).toContain('were NOT migrated'); + }); + + it('warns again on a later real move, having stayed quiet for the no-op', async () => { + // The verdict is per-swap state, so a quiet boot must not disarm the next + // genuine migration. + const dir = await fs.mkdtemp(join(tmpdir(), 'oss-seq-')); + const moved = await fs.mkdtemp(join(tmpdir(), 'oss-seq-moved-')); + const plugin = new StorageServicePlugin({ + adapter: 'local', + local: { rootDir: dir }, + registerRoutes: false, + }); + const ctx = makeCtx(); + const settings = makeFakeSettings({ adapter: 'local', local_root: dir }); + ctx.registerService('settings', settings); + + await plugin.init(ctx); + await plugin.start(ctx); + await ctx._flushReady(); + expect(ctx._logs.warn.join('\n')).not.toContain('adapter swapped'); + + settings.values = { adapter: 'local', local_root: moved }; + settings._emit('storage'); + await new Promise((r) => setTimeout(r, 20)); + + expect(ctx._logs.warn.join('\n')).toContain('adapter swapped'); + }); + it('registers a working storage/test action handler that round-trips a probe blob', async () => { const dir = await fs.mkdtemp(join(tmpdir(), 'oss-probe-')); const plugin = new StorageServicePlugin({ diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index a4f50afd2f..4b810e1e55 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -36,6 +36,12 @@ import { SysAttachment } from '@objectstack/platform-objects/audit'; // value-shape default (#3438). import { SysMigration } from '@objectstack/platform-objects/system'; import { SwappableStorageService } from './swappable-storage-service.js'; +import { + resolveStorageTarget, + needsStorageSwap, + movesStorageLocation, + type StorageTarget, +} from './storage-target.js'; /** * Configuration options for the StorageServicePlugin. @@ -116,6 +122,19 @@ export class StorageServicePlugin implements Plugin { private storage: SwappableStorageService | null = null; private store: StorageMetadataStore | null = null; private metrics: MetricsRegistry = new NoopMetricsRegistry(); + /** + * What the CURRENTLY installed adapter points at (#4096). Set beside every + * adapter this plugin builds, so a settings re-read can tell "nothing + * changed" and "the store moved" apart instead of warning on both. + */ + private target?: StorageTarget; + /** + * Verdict for the swap in flight, set by the caller that resolved both + * configurations. Absent means a caller we know nothing about, and the + * migration warning must not be silenced by ignorance — see + * {@link movesStorageLocation}. + */ + private pendingSwapMovesStore?: boolean; constructor(options: StorageServicePluginOptions = {}) { this.options = { adapter: 'local', ...options }; @@ -168,10 +187,34 @@ export class StorageServicePlugin implements Plugin { const basePath = this.options.basePath ?? '/api/v1/storage'; initial = new LocalStorageAdapter({ rootDir, basePath, ...this.options.local, metrics: this.metrics }); } + // #4096 — record what this adapter points at, so a settings re-read can + // recognise an identical configuration instead of swapping and warning. + // `options.s3` carries only bucket/region/endpoint: constructor-configured + // S3 leaves credentials to the AWS SDK's own resolution chain, so there are + // none to fingerprint here. + this.target = resolveStorageTarget({ + kind: adapter, + rootDir: this.options.local?.rootDir, + basePath: this.options.basePath, + bucket: this.options.s3?.bucket, + region: this.options.s3?.region, + endpoint: this.options.s3?.endpoint, + }); this.storage = new SwappableStorageService(initial, (prev, next) => { const prevName = (prev as any)?.constructor?.name ?? 'unknown'; const nextName = (next as any)?.constructor?.name ?? 'unknown'; + // #4096 — the hazard this warns about is a MOVED backing store, not the + // act of swapping: a credential rotation replaces the adapter while every + // existing object stays exactly where it was. `undefined` means a caller + // that resolved no target, and an unknown swap still warns. + if (this.pendingSwapMovesStore === false) { + ctx.logger.info( + `StorageServicePlugin: storage adapter replaced (${prevName} → ${nextName}) — ` + + 'same backing store, existing files unaffected.', + ); + return; + } ctx.logger.warn( `StorageServicePlugin: storage adapter swapped (${prevName} → ${nextName}). ` + 'Existing files were NOT migrated and may be unreachable through the new adapter.', @@ -331,8 +374,36 @@ export class StorageServicePlugin implements Plugin { // No persisted values yet → keep the constructor-built adapter. const hasAny = Object.values(values).some((v) => v !== undefined && v !== null && v !== ''); if (!hasAny) return; + + // #4096 — `hasAny` is true on every boot once the settings service + // has persisted its own defaults, so this used to rebuild and swap + // unconditionally, warning about stranded files on a swap from an + // adapter to an identically-configured one. Compare the resolved + // CONFIGURATIONS instead: skip entirely when nothing changed, and + // only call it a migration hazard when the backing store moved. + const nextTarget = resolveStorageTarget({ + kind: values.adapter, + rootDir: values.local_root, + basePath: this.options.basePath, + bucket: values.s3_bucket, + region: values.s3_region, + endpoint: values.s3_endpoint, + forcePathStyle: !!values.s3_force_path_style, + accessKeyId: values.s3_access_key_id, + secretAccessKey: values.s3_secret_access_key, + }); + if (!needsStorageSwap(this.target, nextTarget)) return; + const next = await this.buildAdapterFromValues(values); - this.storage.swap(next); + this.pendingSwapMovesStore = movesStorageLocation(this.target, nextTarget); + try { + this.storage.swap(next); + } finally { + // Only the swap this block resolved may claim the verdict; a + // later `swap()` from anywhere else must fall back to warning. + this.pendingSwapMovesStore = undefined; + } + this.target = nextTarget; } catch (err: any) { ctx.logger.warn( 'StorageServicePlugin: failed to apply storage settings: ' + (err?.message ?? err), diff --git a/packages/services/service-storage/src/storage-target.test.ts b/packages/services/service-storage/src/storage-target.test.ts new file mode 100644 index 0000000000..f382d9b326 --- /dev/null +++ b/packages/services/service-storage/src/storage-target.test.ts @@ -0,0 +1,165 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * framework#4096 — "storage adapter swapped (LocalStorageAdapter → + * LocalStorageAdapter) … files may be unreachable" on every clean boot. + * + * `StorageServicePlugin` re-reads the `storage` settings namespace at boot and + * rebuilt + swapped the adapter whenever ANY value was persisted. Once the + * settings service has persisted its own defaults that is every boot, so a + * healthy server warned about stranded files on a swap from an adapter to an + * identically-configured one — and since #4012 that lands in the + * boot-diagnostics block where the real warnings are. + * + * The two questions the plugin now asks are pinned here: is a swap needed at + * all, and did the BACKING STORE move (the only case where existing bytes stop + * being reachable). A credential rotation is the interesting split — swap yes, + * warn no. + */ + +import { describe, it, expect } from 'vitest'; +import { + DEFAULT_BASE_PATH, + DEFAULT_LOCAL_ROOT, + movesStorageLocation, + needsStorageSwap, + resolveStorageTarget, +} from './storage-target.js'; + +describe('resolveStorageTarget', () => { + it('normalises an implicit local root to the same target as an explicit one', () => { + // The #4096 shape: the constructor left `rootDir` implicit and the settings + // namespace persists the schema default. Same store, so this must not read + // as a change. + const implicit = resolveStorageTarget({ kind: 'local' }); + const explicit = resolveStorageTarget({ kind: 'local', rootDir: DEFAULT_LOCAL_ROOT }); + expect(implicit).toEqual(explicit); + expect(needsStorageSwap(implicit, explicit)).toBe(false); + }); + + it('treats `./x` and `x` as one directory — the real #4096 mismatch', () => { + // The platform spells the same default both ways: the settings manifest + // stores `./.objectstack/data/uploads`, the CLI computes + // `.objectstack/data/uploads`. Raw string comparison reported a migration + // between a directory and itself. + const withDot = resolveStorageTarget({ kind: 'local', rootDir: './.objectstack/data/uploads' }); + const without = resolveStorageTarget({ kind: 'local', rootDir: '.objectstack/data/uploads' }); + expect(needsStorageSwap(withDot, without)).toBe(false); + expect(movesStorageLocation(withDot, without)).toBe(false); + }); + + it('still separates genuinely different directories after normalising', () => { + expect(needsStorageSwap( + resolveStorageTarget({ kind: 'local', rootDir: './a/b' }), + resolveStorageTarget({ kind: 'local', rootDir: './a/c' }), + )).toBe(true); + // …including a relative vs absolute spelling, which are not the same path + // without knowing the cwd — this deliberately does not guess. + expect(needsStorageSwap( + resolveStorageTarget({ kind: 'local', rootDir: 'uploads' }), + resolveStorageTarget({ kind: 'local', rootDir: '/var/uploads' }), + )).toBe(true); + }); + + it('normalises an implicit basePath the same way', () => { + expect(resolveStorageTarget({ kind: 'local' }).fingerprint).toBe( + resolveStorageTarget({ kind: 'local', basePath: DEFAULT_BASE_PATH }).fingerprint, + ); + }); + + it('defaults an absent or unknown kind to local', () => { + expect(resolveStorageTarget({}).kind).toBe('local'); + expect(resolveStorageTarget({ kind: 'wat' }).kind).toBe('local'); + }); + + it('keeps credentials out of `location`, which is the loggable half', () => { + // `location` goes into the warning text; the fingerprint never does. + const t = resolveStorageTarget({ + kind: 's3', + bucket: 'b', + region: 'r', + accessKeyId: 'AKIA_PUBLIC', + secretAccessKey: 'super-secret', + }); + expect(t.location).not.toContain('super-secret'); + expect(t.location).not.toContain('AKIA_PUBLIC'); + expect(t.fingerprint).toContain('super-secret'); // compared, never printed + }); +}); + +describe('needsStorageSwap', () => { + const local = (rootDir?: string) => resolveStorageTarget({ kind: 'local', rootDir }); + + it('is false for an identical configuration — the boot that used to warn', () => { + expect(needsStorageSwap(local('/srv/files'), local('/srv/files'))).toBe(false); + }); + + it('is true when the local root changes', () => { + expect(needsStorageSwap(local('/srv/a'), local('/srv/b'))).toBe(true); + }); + + it('is true when only a credential changes — the new key must take effect', () => { + const before = resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'r', secretAccessKey: 'old' }); + const after = resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'r', secretAccessKey: 'new' }); + expect(needsStorageSwap(before, after)).toBe(true); + }); + + it('is true when basePath changes — the adapter signs different URLs', () => { + expect(needsStorageSwap( + resolveStorageTarget({ kind: 'local', rootDir: '/srv', basePath: '/api/v1/storage' }), + resolveStorageTarget({ kind: 'local', rootDir: '/srv', basePath: '/files' }), + )).toBe(true); + }); + + it('is true with no known starting point', () => { + expect(needsStorageSwap(undefined, local('/srv'))).toBe(true); + }); +}); + +describe('movesStorageLocation', () => { + it('is false for a credential rotation — same bucket, nothing stranded', () => { + // The split that makes this worth doing: swap, but do not claim files became + // unreachable when every object stayed where it was. + const before = resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'r', secretAccessKey: 'old' }); + const after = resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'r', secretAccessKey: 'new' }); + expect(needsStorageSwap(before, after)).toBe(true); + expect(movesStorageLocation(before, after)).toBe(false); + }); + + it('is false when only basePath changes — URLs move, bytes do not', () => { + expect(movesStorageLocation( + resolveStorageTarget({ kind: 'local', rootDir: '/srv', basePath: '/api/v1/storage' }), + resolveStorageTarget({ kind: 'local', rootDir: '/srv', basePath: '/files' }), + )).toBe(false); + }); + + it('is true when the local root moves', () => { + expect(movesStorageLocation( + resolveStorageTarget({ kind: 'local', rootDir: '/srv/a' }), + resolveStorageTarget({ kind: 'local', rootDir: '/srv/b' }), + )).toBe(true); + }); + + it('is true when the kind changes — local → s3 strands every file', () => { + expect(movesStorageLocation( + resolveStorageTarget({ kind: 'local', rootDir: '/srv' }), + resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'r' }), + )).toBe(true); + }); + + it('is true for a different bucket, and for the same bucket on another endpoint', () => { + const aws = resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'r' }); + expect(movesStorageLocation(aws, resolveStorageTarget({ kind: 's3', bucket: 'other', region: 'r' }))).toBe(true); + // Same bucket NAME on MinIO/R2 is a different store entirely. + expect(movesStorageLocation( + aws, + resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'r', endpoint: 'https://minio.local' }), + )).toBe(true); + expect(movesStorageLocation(aws, resolveStorageTarget({ kind: 's3', bucket: 'b', region: 'other' }))).toBe(true); + }); + + it('warns when the starting point is unknown — ignorance must not silence it', () => { + // A `swap()` from a caller that resolved no target still gets the warning. + expect(movesStorageLocation(undefined, resolveStorageTarget({ kind: 'local' }))).toBe(true); + }); +}); diff --git a/packages/services/service-storage/src/storage-target.ts b/packages/services/service-storage/src/storage-target.ts new file mode 100644 index 0000000000..50582914cf --- /dev/null +++ b/packages/services/service-storage/src/storage-target.ts @@ -0,0 +1,135 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +// --------------------------------------------------------------------------- +// What a resolved storage configuration points at (#4096). +// +// `StorageServicePlugin` binds to the `storage` settings namespace so an admin +// can change adapters without a restart. On every boot it reads that namespace +// and, if ANY value is persisted, rebuilt the adapter and swapped it in +// unconditionally — and the swap callback warns that "existing files were NOT +// migrated and may be unreachable". Once the settings service has persisted its +// defaults, that is every clean boot, so a healthy dev server printed +// +// WARN StorageServicePlugin: storage adapter swapped +// (LocalStorageAdapter → LocalStorageAdapter). Existing files were NOT +// migrated and may be unreachable through the new adapter. +// +// every time, about a swap from an adapter to an identically-configured one. The +// warning is worth keeping — Local → S3 really does strand every existing file — +// but crying wolf on every boot is how an operator learns to skim past it, and +// since #4012 it lands in the boot-diagnostics block where the real warnings are. +// +// So the decision needs two distinct questions, which the plugin can only answer +// by comparing the CONFIGURATIONS it resolved (both adapters keep their identity +// in private fields): +// +// 1. Is a swap needed at all? — did anything the adapter is built from change? +// 2. Should it warn? — did the BACKING STORE change, i.e. would +// existing bytes stop being reachable? +// +// A credential rotation answers yes/no: swap so the new key takes effect, but the +// bucket still holds the same objects, so nothing is stranded. +// --------------------------------------------------------------------------- + +import { normalize } from 'node:path'; + +/** A resolved storage configuration, reduced to what swap decisions need. */ +export interface StorageTarget { + kind: 'local' | 's3'; + /** + * Where the bytes live. A change here strands whatever the old target held — + * this is what the migration warning is actually about. Safe to log. + */ + location: string; + /** + * Everything the adapter is constructed from, including credentials, so an + * unchanged configuration can be recognised and skipped. + * + * NEVER log this: it embeds the S3 secret. It exists only to be compared. + */ + fingerprint: string; +} + +/** The fields a storage target is derived from, normalised across both sources. */ +export interface StorageTargetInput { + kind?: string; + /** Local adapter root directory (constructor `local.rootDir` / setting `local_root`). */ + rootDir?: string; + /** URL prefix the adapter signs against — not a storage location. */ + basePath?: string; + bucket?: string; + region?: string; + endpoint?: string; + forcePathStyle?: boolean; + accessKeyId?: string; + secretAccessKey?: string; +} + +/** `LocalStorageAdapter`'s default root, mirrored so both sources normalise alike. */ +export const DEFAULT_LOCAL_ROOT = './storage'; + +/** `StorageServicePlugin`'s default URL prefix. */ +export const DEFAULT_BASE_PATH = '/api/v1/storage'; + +/** + * Reduce a resolved storage configuration to its {@link StorageTarget}. + * + * Unset values are normalised to the same defaults the adapters apply, so a + * configuration that merely spells out a default does not read as a change — + * which is exactly the #4096 shape: the settings namespace persists + * `local_root: './storage'` and the constructor left it implicit. + */ +export function resolveStorageTarget(input: StorageTargetInput): StorageTarget { + const kind = String(input.kind ?? 'local') === 's3' ? 's3' : 'local'; + + if (kind === 's3') { + // Bucket identity is host + region + bucket. `endpoint` distinguishes an + // S3-compatible service (MinIO, R2) from AWS itself, and two different + // endpoints are two different stores even for the same bucket name. + const location = `s3://${input.endpoint || 'aws'}/${input.region ?? ''}/${input.bucket ?? ''}`; + return { + kind, + location, + fingerprint: [ + location, + `forcePathStyle=${input.forcePathStyle ? '1' : '0'}`, + `accessKeyId=${input.accessKeyId ?? ''}`, + `secretAccessKey=${input.secretAccessKey ?? ''}`, + ].join('|'), + }; + } + + // `./x` and `x` are the same directory, and the platform spells this default + // both ways: the settings manifest stores `./.objectstack/data/uploads` while + // the CLI computes `.objectstack/data/uploads`. Comparing the raw strings + // reported a migration between a directory and itself. + const rootDir = normalize(input.rootDir || DEFAULT_LOCAL_ROOT); + const location = `local://${rootDir}`; + return { + kind, + // `basePath` is deliberately NOT part of the location: it changes the URLs + // the adapter signs, not where the bytes sit, so changing it strands nothing. + location, + fingerprint: [location, `basePath=${input.basePath || DEFAULT_BASE_PATH}`].join('|'), + }; +} + +/** Whether a swap is needed: anything the adapter is built from differs. */ +export function needsStorageSwap(current: StorageTarget | undefined, next: StorageTarget): boolean { + return !current || current.fingerprint !== next.fingerprint; +} + +/** + * Whether a swap strands existing files — the backing store moved, so what the + * old adapter held is not reachable through the new one. + */ +export function movesStorageLocation( + current: StorageTarget | undefined, + next: StorageTarget, +): boolean { + // An unknown starting point is treated as a move: this is the warning that + // exists to be impossible to miss, so absence of information must not silence + // it (a `swap()` from a caller that resolved no target lands here). + if (!current) return true; + return current.kind !== next.kind || current.location !== next.location; +}