From c80f33a90165797abce26b3b6aa124c57f103ad8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 16:23:18 +0000 Subject: [PATCH 1/2] fix(service-storage): clean up the storage/test probe in the store it wrote to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `settings.registerAction('storage', 'test', …)` handler builds a temporary adapter when the form posts values, so an operator can validate unsaved credentials, and probes that adapter instead of the persisted one. Two paths left the probe object behind. Defect 1: `let target` was declared inside the `try`, so at the `catch` the cleanup could only name `proxy` — the persisted adapter — while the probe had written to the temporary one. Deleting an absent key is a no-op on both shipped adapters, so the wrong-store delete "succeeded" and nothing looked wrong. The declaration is now resolved before the try, which makes "the cleanup names the store the upload named" true by construction, and settles the half-built-adapter question: a construction failure returns before anything is written, so no cleanup is attempted on that path. Defect 2: the content-mismatch `return` walked past the delete on the next line, after an upload that had by definition already succeeded — a guaranteed leak on the one path that only fires when the adapter is misbehaving. It now runs the same best-effort cleanup, which carries #12981 batch 7's refusal warning to this path for the first time. What the probe reports to the operator is unchanged on every path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- ...service-plugin.probe-cleanup-store.test.ts | 361 ++++++++++++++++++ .../src/storage-service-plugin.ts | 110 ++++-- 2 files changed, 435 insertions(+), 36 deletions(-) create mode 100644 packages/services/service-storage/src/storage-service-plugin.probe-cleanup-store.test.ts diff --git a/packages/services/service-storage/src/storage-service-plugin.probe-cleanup-store.test.ts b/packages/services/service-storage/src/storage-service-plugin.probe-cleanup-store.test.ts new file mode 100644 index 0000000000..1b2f99d823 --- /dev/null +++ b/packages/services/service-storage/src/storage-service-plugin.probe-cleanup-store.test.ts @@ -0,0 +1,361 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13726] The `storage/test` probe cleans up in the store it WROTE to. + * + * The handler exists so an operator can validate credentials that are typed + * into the form but not yet saved, so when the form posts values it builds a + * TEMPORARY adapter and probes that instead of the persisted one. Two paths + * left the probe object behind: + * + * 1. the failure cleanup deleted from `proxy` — the PERSISTED adapter — + * while the probe had written to the temporary one. Deleting an absent + * key is a no-op on both shipped adapters, so the wrong-store delete + * "succeeded" and nothing looked wrong; + * 2. the content-mismatch `return` walked straight past the delete on the + * next line, after an upload that by definition had already succeeded — + * a guaranteed leak rather than a best-effort one. + * + * ⚠️ Both credential cases are pinned SEPARATELY, and only one of the two + * directions can catch defect 1: with no overrides `target === proxy`, so the + * old code deleted from the right store by accident and a single-direction pin + * passes on the defect. The case that matters is a failed probe WITH edited + * credentials. + * + * ## How a failure is induced + * + * Every store below is a REAL `LocalStorageAdapter` on its own directory, with + * exactly one verb overridden (`Object.create`, so every other member stays the + * real one). PUT allowed / GET refused is the ordinary shape of a half-right + * credential, and it is what makes the leak observable: the bytes really land + * on disk, and then the probe really fails. The assertions are therefore about + * the FILESYSTEM — what is left under `__objectstack_probe__/` when the handler + * returns — not about a call counter that could agree with a store nobody + * wrote to. + * + * ⚠️ Two cases below are CONTROLS, not pins, and are labelled: they are green + * in both directions by construction (the pre-repair code already deleted from + * the right store when there were no overrides, and already attempted no + * cleanup when the adapter failed to build). They are here so the pins cannot + * pass on a handler that deletes from everything, or on one that cleans up + * after a store it never wrote to. ⛔ Not ablation evidence. + */ + +import { describe, it, expect } from 'vitest'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import type { IStorageService } from '@objectstack/spec/contracts'; +import { LocalStorageAdapter } from './local-storage-adapter.js'; +import { StorageServicePlugin } from './storage-service-plugin.js'; +import type { SwappableStorageService } from './swappable-storage-service.js'; + +const PROBE_PREFIX = '__objectstack_probe__'; +const CLEANUP_HEADLINE = 'was NOT removed'; +const MISMATCH_MESSAGE = 'Probe download did not match upload.'; + +function makeCtx() { + const services = new Map(); + const hooks: Array<() => Promise | void> = []; + const logs: { info: string[]; warn: string[]; error: string[] } = { info: [], warn: [], error: [] }; + const ctx: any = { + 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: unknown) => { services.set(name, svc); }, + getService: (name: string): T => { + const s = services.get(name); + if (!s) throw new Error(`service '${name}' not registered`); + return s as T; + }, + hook: (event: string, fn: () => Promise | void) => { + if (event === 'kernel:ready') hooks.push(fn); + }, + _flushReady: async () => { for (const h of hooks) await h(); }, + }; + return ctx; +} + +/** A settings service that keeps the registered action so a test can run it. */ +function makeFakeSettings() { + const actions = new Map Promise>(); + return { + createClient: (_ns: string) => ({}), + getNamespace: async (_ns: string) => ({ values: {} }), + subscribe: (_ns: string, _fn: () => void) => {}, + registerAction: (ns: string, id: string, fn: (input: unknown) => Promise) => { + actions.set(`${ns}/${id}`, fn); + }, + _runAction: async (ns: string, id: string, input: unknown) => { + const fn = actions.get(`${ns}/${id}`); + if (!fn) throw new Error(`no action ${ns}/${id}`); + return await fn(input); + }, + }; +} + +async function tmpRoot(prefix: string): Promise { + return await fs.mkdtemp(join(tmpdir(), prefix)); +} + +/** A real local adapter rooted at `rootDir` — the store, not a stand-in. */ +function localAdapterAt(rootDir: string): IStorageService { + return new LocalStorageAdapter({ rootDir, basePath: '/api/v1/storage' }); +} + +/** + * The real store with ONE verb replaced. `Object.create` rather than a + * hand-written stand-in, deliberately: every member this test does not name + * stays the adapter's own, so a probe object written through the wrapper is a + * real file and the assertions can read the filesystem. + */ +function withRefusedDownload(real: IStorageService, message: string): IStorageService { + const store: IStorageService = Object.create(real); + store.download = async () => { throw new Error(message); }; + return store; +} + +function withMangledDownload(real: IStorageService): IStorageService { + const store: IStorageService = Object.create(real); + store.download = async () => Buffer.from('not-what-was-uploaded', 'utf-8'); + return store; +} + +function withRefusedDelete(real: IStorageService, message: string): IStorageService { + const store: IStorageService = Object.create(real); + store.delete = async () => { throw new Error(message); }; + return store; +} + +/** The real store, recording every key it is ASKED to delete. */ +function withCountedDeletes(real: IStorageService): { store: IStorageService; deleted: string[] } { + const deleted: string[] = []; + const store: IStorageService = Object.create(real); + store.delete = async (key: string) => { deleted.push(key); await real.delete(key); }; + return { store, deleted }; +} + +/** Probe objects currently on disk under `rootDir`. */ +async function probeObjectsIn(rootDir: string): Promise { + try { + return (await fs.readdir(join(rootDir, PROBE_PREFIX))).sort(); + } catch (err: any) { + if (err?.code === 'ENOENT') return []; + throw err; + } +} + +/** + * The factory the handler calls when the form posts values, substituted so a + * test can hand it a store whose behaviour it controls. + * + * Named as a seam rather than reached for with `as any`: `buildAdapterFromValues` + * itself is covered by its own tests (`storage-service-plugin.metrics.test.ts` + * and the S3-misconfiguration case in `storage-service-plugin.test.ts`), and + * what is under test HERE is which store the handler cleans up in — not how the + * temporary one is constructed. + */ +interface AdapterFactorySeam { + buildAdapterFromValues(values: Record): Promise; +} + +function substituteAdapterFactory( + plugin: StorageServicePlugin, + temporary: IStorageService, +): Array> { + const calls: Array> = []; + const seam = plugin as unknown as AdapterFactorySeam; + seam.buildAdapterFromValues = async (values: Record) => { + calls.push(values); + return temporary; + }; + return calls; +} + +async function bootedPlugin(persistedRoot: string) { + const plugin = new StorageServicePlugin({ + adapter: 'local', + local: { rootDir: persistedRoot }, + registerRoutes: false, + }); + const ctx = makeCtx(); + const settings = makeFakeSettings(); + ctx.registerService('settings', settings); + await plugin.init(ctx); + await plugin.start(ctx); + await ctx._flushReady(); + // Typed here rather than at the call site: the fake ctx is `any`, so + // `ctx.getService(…)` would be a type argument on an untyped call. + const storage: SwappableStorageService = ctx.getService('storage'); + return { plugin, ctx, settings, storage }; +} + +/** The shape the settings form posts when the operator edited the fields. */ +function editedCredentials(localRoot: string) { + return { values: {}, payload: { values: { adapter: 'local', local_root: localRoot } } }; +} + +describe('#13726 defect 1 — the failure cleanup names the store the probe wrote to', () => { + it('a failed probe with EDITED credentials leaves nothing behind in the TEMPORARY store', async () => { + const persistedRoot = await tmpRoot('oss-13726-persisted-'); + const temporaryRoot = await tmpRoot('oss-13726-temporary-'); + const { plugin, ctx, settings, storage } = await bootedPlugin(persistedRoot); + + // The persisted store, watching for deletes it should never be asked for. + const persisted = withCountedDeletes(localAdapterAt(persistedRoot)); + storage.swap(persisted.store); + + // The store the edited credentials build: a different directory, and a GET + // that is refused after the PUT has already landed the bytes. + const temporary = withRefusedDownload( + localAdapterAt(temporaryRoot), + 'download refused: GET denied for this key', + ); + const calls = substituteAdapterFactory(plugin, temporary); + + const result = await settings._runAction('storage', 'test', editedCredentials(temporaryRoot)); + + // The temporary-adapter branch really ran — without it this pin would be + // measuring the no-overrides case under an overrides-shaped name. + expect(calls).toHaveLength(1); + expect(calls[0]).toMatchObject({ adapter: 'local', local_root: temporaryRoot }); + + // THE PIN: the store the probe wrote to holds nothing afterwards. + expect(await probeObjectsIn(temporaryRoot)).toEqual([]); + + // …and the persisted store was neither written to nor asked to delete: the + // old cleanup issued a delete here, against a key this store never held. + expect(await probeObjectsIn(persistedRoot)).toEqual([]); + expect(persisted.deleted).toEqual([]); + + // ⛔ What the operator is told is unchanged by the repair. + expect(result.ok).toBe(false); + expect(result.severity).toBe('error'); + expect(result.message).toBe('download refused: GET denied for this key'); + // The cleanup succeeded, so #12981's refusal line stays quiet. + expect(ctx._logs.warn.join('\n')).not.toContain(CLEANUP_HEADLINE); + }); + + // ⚠️ CONTROL, not a pin — green in BOTH directions. With no overrides + // `target === proxy`, so the pre-repair `proxy.delete` was already the right + // store. It is here so the pin above cannot pass on a handler that stopped + // cleaning up the persisted store when it repaired the temporary one. + it('CONTROL: a failed probe with NO edited credentials leaves nothing behind in the PERSISTED store', async () => { + const persistedRoot = await tmpRoot('oss-13726-persisted-only-'); + const { ctx, settings, storage } = await bootedPlugin(persistedRoot); + + storage.swap(withRefusedDownload(localAdapterAt(persistedRoot), 'download refused: GET denied')); + + const result = await settings._runAction('storage', 'test', { values: {} }); + + expect(await probeObjectsIn(persistedRoot)).toEqual([]); + expect(result.ok).toBe(false); + expect(result.message).toBe('download refused: GET denied'); + expect(ctx._logs.warn.join('\n')).not.toContain(CLEANUP_HEADLINE); + }); + + // ⚠️ CONTROL, not a pin — green in both directions. It pins the judgement + // this card turns on: `target` is resolved BEFORE the try whose catch cleans + // up, so the catch can never see a half-built adapter or the adapter whose + // construction threw. A build failure returns before anything is written, and + // the handler must therefore attempt NO cleanup — not against the persisted + // store (nothing was written there) and not against the adapter that failed + // to construct (there is none). Uses the REAL factory, which rejects an S3 + // configuration with no bucket or region. + it('CONTROL: an adapter that fails to BUILD is reported, and no cleanup is attempted anywhere', async () => { + const persistedRoot = await tmpRoot('oss-13726-nobuild-'); + const { ctx, settings, storage } = await bootedPlugin(persistedRoot); + + const persisted = withCountedDeletes(localAdapterAt(persistedRoot)); + storage.swap(persisted.store); + + const result = await settings._runAction('storage', 'test', { + values: {}, + payload: { values: { adapter: 's3', s3_bucket: '', s3_region: '' } }, + }); + + expect(result.ok).toBe(false); + expect(result.severity).toBe('error'); + expect(result.message).toContain('S3 adapter requires s3_bucket and s3_region'); + expect(persisted.deleted).toEqual([]); + expect(await probeObjectsIn(persistedRoot)).toEqual([]); + expect(ctx._logs.warn.join('\n')).not.toContain(CLEANUP_HEADLINE); + }); +}); + +describe('#13726 defect 2 — the content-mismatch path cleans up', () => { + it('a mismatch on EDITED credentials leaves nothing behind in the TEMPORARY store', async () => { + const persistedRoot = await tmpRoot('oss-13726-mismatch-persisted-'); + const temporaryRoot = await tmpRoot('oss-13726-mismatch-temporary-'); + const { plugin, settings, storage } = await bootedPlugin(persistedRoot); + + const persisted = withCountedDeletes(localAdapterAt(persistedRoot)); + storage.swap(persisted.store); + + // The upload SUCCEEDS here — that is the precondition for reaching the + // comparison at all — and the download answers other bytes. + const temporary = withMangledDownload(localAdapterAt(temporaryRoot)); + substituteAdapterFactory(plugin, temporary); + + const result = await settings._runAction('storage', 'test', editedCredentials(temporaryRoot)); + + // THE PIN: the upload landed, and nothing is left of it. + expect(await probeObjectsIn(temporaryRoot)).toEqual([]); + expect(persisted.deleted).toEqual([]); + + // ⛔ The message the operator reads is unchanged. + expect(result.ok).toBe(false); + expect(result.severity).toBe('error'); + expect(result.message).toBe(MISMATCH_MESSAGE); + }); + + it('a mismatch with NO edited credentials leaves nothing behind in the PERSISTED store', async () => { + const persistedRoot = await tmpRoot('oss-13726-mismatch-only-'); + const { settings, storage } = await bootedPlugin(persistedRoot); + + const counted = withCountedDeletes(localAdapterAt(persistedRoot)); + storage.swap(withMangledDownload(counted.store)); + + const result = await settings._runAction('storage', 'test', { values: {} }); + + expect(await probeObjectsIn(persistedRoot)).toEqual([]); + expect(counted.deleted).toHaveLength(1); + expect(counted.deleted[0]).toContain(`${PROBE_PREFIX}/`); + expect(result.ok).toBe(false); + expect(result.message).toBe(MISMATCH_MESSAGE); + }); + + // #12981 batch 7 made a REFUSED cleanup name the key it left behind. That + // repair could not reach this path, because no cleanup was attempted on it. + // Now that one is, the refusal is reported here too — the same line, from the + // same helper — and the probe's own verdict is still the one returned. + it('a mismatch whose cleanup is REFUSED names the stray key, and still reports the mismatch', async () => { + const persistedRoot = await tmpRoot('oss-13726-mismatch-refused-'); + const { ctx, settings, storage } = await bootedPlugin(persistedRoot); + + storage.swap( + withRefusedDelete( + withMangledDownload(localAdapterAt(persistedRoot)), + 'delete refused: bucket is read-only', + ), + ); + + const result = await settings._runAction('storage', 'test', { values: {} }); + + const warned = ctx._logs.warn.filter((l: string) => l.includes(CLEANUP_HEADLINE)); + expect(warned).toHaveLength(1); + expect(warned[0]).toContain(`${PROBE_PREFIX}/`); + expect(warned[0]).toContain('delete refused: bucket is read-only'); + + // The object really is still there — the warning is not decorative. + expect(await probeObjectsIn(persistedRoot)).toHaveLength(1); + + // ⛔ The probe's own result is untouched by the cleanup's failure. + expect(result.ok).toBe(false); + expect(result.severity).toBe('error'); + expect(result.message).toBe(MISMATCH_MESSAGE); + }); +}); diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 51f623729c..56697fe055 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -544,21 +544,83 @@ export class StorageServicePlugin implements Plugin { const merged: Record = { ...(values ?? {}), ...overrides }; const probeKey = `__objectstack_probe__/${Date.now()}-${Math.random().toString(36).slice(2, 10)}`; const probeBytes = Buffer.from(`probe@${new Date().toISOString()}`, 'utf-8'); - try { - // If merged values are present, build a temporary adapter - // so we can validate user-typed credentials without - // committing them. - let target: IStorageService = proxy; - if (merged && Object.keys(merged).length > 0) { - try { - target = await this.buildAdapterFromValues(merged); - } catch (err: any) { - return { ok: false, severity: 'error', message: err?.message ?? String(err) }; - } + // [#13726] Resolve the store the probe writes to BEFORE entering + // the region whose failure path has to clean up after it. This + // declaration used to sit inside that `try`, which left `target` + // out of scope at the `catch` -- so the cleanup deleted from + // `proxy`, the PERSISTED adapter, even when the probe had written + // to the temporary one. On exactly the case the temporary adapter + // exists for -- an operator validating unsaved credentials -- the + // object leaked in the store that held it while a delete went to a + // store that never did, and since deleting an absent key is a no-op + // on both shipped adapters, nothing looked wrong. + // + // Resolving it here makes "the cleanup names the store the upload + // named" true by CONSTRUCTION rather than by two expressions that + // happen to agree and can drift apart in a later edit. It also + // settles the question the scoped-in version raised -- whether the + // catch could see a half-built adapter, or the very adapter whose + // construction threw. It cannot: a construction failure returns + // right below, BEFORE the probe writes anything, so there is + // deliberately no cleanup on that path -- there is nothing to clean + // up, and a delete against an adapter that failed to build is a + // call we have no reason to make. Past this point `target` is + // either `proxy` or a fully constructed temporary adapter, and + // either way it is the one every verb below uses. + let target: IStorageService = proxy; + if (merged && Object.keys(merged).length > 0) { + try { + target = await this.buildAdapterFromValues(merged); + } catch (err: any) { + return { ok: false, severity: 'error', message: err?.message ?? String(err) }; + } + } + + // Best-effort removal of the probe object from the store it was + // written to -- `target`, never `proxy` (see above). + // + // [#12981] The cleanup's OWN failure used to be swallowed by + // `catch { /* ignore */ }`. The `return` beside each call site + // reports the PROBE's failure, which is a different failure: the + // operator is told the round-trip did not work and is told nothing + // at all about the probe object that is still sitting in the + // bucket. One stray key accrues per failed test, with the only + // record of its name -- `probeKey` is minted per call from a + // timestamp and a random suffix -- lost when this frame returns. + // + // `warn`, not `error`: nothing the system claims to a caller as + // persisted was lost. This is housekeeping that did not happen, + // and the AGENTS.md rule is explicit that escalating a + // non-durability degradation is how `error` stops being read. + const removeProbeObject = async (): Promise => { + try { + await target.delete(probeKey); + } catch (cleanupErr: any) { + ctx.logger.warn( + `StorageServicePlugin: the storage/test probe object '${probeKey}' was NOT removed — ` + + `the cleanup delete was refused (${cleanupErr?.message ?? cleanupErr}). The probe ` + + 'result returned below is unaffected and still reports the original failure. ' + + 'This object is inert probe content that no record references and nothing ' + + 'retries or later collects; delete it by hand if stray keys under ' + + '`__objectstack_probe__/` matter in this bucket.', + ); } + }; + + try { await target.upload(probeKey, probeBytes, { contentType: 'text/plain' }); const got = await target.download(probeKey); if (!got || !Buffer.isBuffer(got) || got.toString('utf-8') !== probeBytes.toString('utf-8')) { + // [#13726] The upload SUCCEEDED -- reaching this comparison at + // all is the proof -- so the object is in `target` right now, + // and this `return` used to walk straight past the delete on + // the next line: a GUARANTEED leak, not a best-effort one, on + // the one path that by construction only fires when the adapter + // is misbehaving. Cleaned up HERE rather than in a `finally`, + // deliberately: the success path below keeps its single + // unguarded delete, whose failure IS a probe failure the + // operator must be told about, and no path deletes twice. + await removeProbeObject(); return { ok: false, severity: 'error', message: 'Probe download did not match upload.' }; } await target.delete(probeKey); @@ -569,31 +631,7 @@ export class StorageServicePlugin implements Plugin { message: `Storage round-trip succeeded (adapter=${adapter}).`, }; } catch (err: any) { - // Best-effort cleanup. [#12981] The cleanup's OWN failure used to - // be swallowed by `catch { /* ignore */ }`. The `return` below - // reports the PROBE's failure, which is a different failure: the - // operator is told the round-trip did not work and is told - // nothing at all about the probe object that is still sitting in - // the bucket. One stray key accrues per failed test, with the - // only record of its name -- `probeKey` is minted per call from a - // timestamp and a random suffix -- lost when this frame returns. - // - // `warn`, not `error`: nothing the system claims to a caller as - // persisted was lost. This is housekeeping that did not happen, - // and the AGENTS.md rule is explicit that escalating a - // non-durability degradation is how `error` stops being read. - try { - await (proxy as IStorageService).delete(probeKey); - } catch (cleanupErr: any) { - ctx.logger.warn( - `StorageServicePlugin: the storage/test probe object '${probeKey}' was NOT removed — ` - + `the cleanup delete was refused (${cleanupErr?.message ?? cleanupErr}). The probe ` - + 'result returned below is unaffected and still reports the original failure. ' - + 'This object is inert probe content that no record references and nothing ' - + 'retries or later collects; delete it by hand if stray keys under ' - + '`__objectstack_probe__/` matter in this bucket.', - ); - } + await removeProbeObject(); return { ok: false, severity: 'error', message: err?.message ?? String(err) }; } }); From 11c802f5d386cce8bf70439694cfe01d9a0b8913 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 16:47:36 +0000 Subject: [PATCH 2/2] chore(changeset): the storage/test probe cleans up in the store it wrote to Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- .../storage-probe-cleanup-target-store.md | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 .changeset/storage-probe-cleanup-target-store.md diff --git a/.changeset/storage-probe-cleanup-target-store.md b/.changeset/storage-probe-cleanup-target-store.md new file mode 100644 index 0000000000..eb5c1701ae --- /dev/null +++ b/.changeset/storage-probe-cleanup-target-store.md @@ -0,0 +1,35 @@ +--- +"@objectstack/service-storage": patch +--- + +fix(service-storage): the `storage/test` probe cleans up in the store it wrote to (#13726) + +The settings action behind the storage screen's "Test" button writes a small +`__objectstack_probe__/…` object, reads it back, and deletes it. When the form +posts values it builds a **temporary** adapter first, so an operator can +validate credentials that are typed but not yet saved, and probes that adapter +instead of the persisted one. Two paths left the probe object behind in the +customer's bucket. + +- **The failure cleanup deleted from the wrong store.** `target` was declared + inside the `try`, so the `catch` could only name the persisted adapter — even + when the probe had written to the temporary one, which is the whole case the + temporary adapter exists for. Deleting a key that was never there is a no-op + on both shipped adapters, so the wrong-store delete "succeeded" and nothing + looked wrong. The adapter is now resolved before that `try`, which makes the + cleanup name the store the upload named by construction. +- **The content-mismatch return path cleaned up nothing.** Reaching that + comparison means the upload already succeeded, so the object is definitely + there — and the `return` walked straight past the delete on the next line. It + now runs the same best-effort cleanup as the failure path, which also carries + the "cleanup refused — here is the key it left behind" warning to this path + for the first time. + +One stray object accrued per failed test, under a name minted per call from a +timestamp and a random suffix and recorded nowhere, in whichever store the probe +actually wrote to — a button whose entire purpose is to be pressed repeatedly +while credentials are being got right. + +An adapter that fails to *construct* still attempts no cleanup: nothing has been +written at that point, and the delete would have to name an adapter that does +not exist. What the probe reports to the operator is unchanged on every path.