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; +}