Uh oh!
There was an error while loading. Please reload this page.
fix(service-storage): the storage/test probe cleans up in the store it wrote to - #13921
Merged
Conversation
… wrote to
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs…ote to Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This was referenced Aug 31, 2026
os-steve
marked this pull request as ready for review
August 31, 2026 17:36
os-steve
enabled auto-merge
August 31, 2026 17:36
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#13726
The
settings.registerAction('storage', 'test', …)handler writes a 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 whichever store it was actually written to. Both were surfaced by the #12981 batch-7 dev inside the lines that batch edits, and correctly left alone there; re-verified by symbol on this branch's base (46b53a25b), since PR #13725 moved the line numbers.Defect 1 — the failure cleanup deleted from a store the probe never wrote to
let target: IStorageService = proxywas declared inside thetry, so at thecatchthe only name in scope wasproxy, the persisted adapter. On the case the temporary adapter exists for — a failed test with edited credentials — the object leaked in the store that held it while a delete was issued against a store that never did. Deleting an absent key is a no-op on both shipped adapters, so the wrong-store delete "succeeded" and nothing looked wrong.Repaired by resolving the adapter before the
trywhosecatchhas to clean up after it. That makes "the cleanup names the store the upload named" true by construction rather than by two expressions that happen to agree today.Defect 2 — the content-mismatch return path cleaned up nothing
Reaching the round-trip comparison means the upload already succeeded, so the object is definitely present — and the
returnwalked straight past thedeleteon the very next line. A guaranteed leak, not a best-effort one, on the one path that by construction only fires when the adapter is misbehaving. It now runs the same best-effort cleanup as the failure path.The two judgement calls
1. "
targetmay be unassigned, or may be the very thing that threw." The card is right that a delete against a half-built adapter is its own question, and the answer here is that the question is dissolved rather than answered:buildAdapterFromValuesalready had its own innercatchthat returns, so a construction failure never reached the outercatch— but the reader could not see that from thecatch, because the declaration was out of scope there. Hoisting the resolution above thetrystates the invariant where it can be read: past that pointtargetis eitherproxyor a fully constructed temporary adapter. A construction failure still returns before anything is written, and on that path no cleanup is attempted at all — nothing was written, and a delete would have to name an adapter that does not exist. That decision is pinned (as a declared control, below), so a later "tidy-up" that adds afinallyreddens it.2. Where defect 2 cleans up. A
finallywas rejected: the success path already deletes on its own line, and its failure IS a probe failure the operator must be told about, so afinallywould either delete twice or need a flag to know not to. "Leave it for inspection" was rejected because nothing records the key — it is minted per call from a timestamp and a random suffix — so the object would be un-findable litter rather than evidence. The mismatch path therefore calls the same cleanup helper thecatchdoes, immediately before itsreturn.Built on batch 7, not over it
#12981 batch 7 made a refused cleanup name the key it left behind. That repair is intact — it moved into the shared
removeProbeObjecthelper, warn level and message text unchanged — and it now covers the mismatch path too, which it could not reach before because no cleanup was attempted there. One pin drives exactly that case.What did NOT change
No sweep for the
__objectstack_probe__/prefix — out of scope by the card. What the probe reports to the operator is unchanged on every path: the same result shape and the same messages, pinned in every case below. No published surface changes and no accept/reject behaviour moves.Verification
All numbers from head
11c802f5d.Pins —
packages/services/service-storage/src/storage-service-plugin.probe-cleanup-store.test.tsEvery store in the file is a real
LocalStorageAdapteron its own temp directory with exactly one verb replaced (viaObject.create, so every other member stays the adapter's own). 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, then the probe really fails. The assertions read the filesystem — what is left under__objectstack_probe__/when the handler returns — rather than a call counter that could agree with a store nobody wrote to.The two controls are green in both directions by construction — with no overrides
target === proxy, so the old code deleted from the right store by accident, and the old code also attempted no cleanup after a build failure. They are declared as controls in the file's header and are not ablation evidence; they are there 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.Ablation
Predicted before the run: reverting only
storage-service-plugin.tsreddens the 4 pins and leaves the 2 controls green.Procedure: the repair was committed first (
c80f33a90); the mutation reverted the single file to its pre-fix blob in the worktree only; anEXIT INT TERMtrap restored it with an absolute path; the mutation was confirmed on disk before measuring, by blob hash and by marker counts, never by an editor's exit code. No rebuild is in the loop: the pins import the module under test by relative path within the same package, so vitest reads the source.ts, and this package'svitest.config.tscarries no alias that could route it todist/.The mutated failures are the leak itself, named on disk:
Direction observed = direction predicted. The restore leg is proven by state — hash back to the HEAD blob, empty
git diff HEAD, emptygit status --porcelain— and re-measured green afterwards, so no later reading was taken on a mutated tree.Suites and gates
pnpm --filter @objectstack/service-storage exec vitest run --maxWorkers=2— 35 files, 528 tests passed (the whole package, including The durability log-level gate cannot see thecatch { return null; }seeder family — 15 files outside #12923's five, and neither widening path is cheap #12981 batch 7's own file).pnpm --filter @objectstack/service-storage exec tsc --noEmit— 51 errors, zero of them in either changed file. That is exactly this package's frozen DEBT ledger count, socheck:type-check-debt's re-measure cannot move up on this diff. Run with the dependency closure built (pnpm --filter '@objectstack/service-storage^...' build), which is the ledger's own measurement condition.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(both output sections read whole; the family is identical at the final head). All 36 harvested commands run; 32 green, and 4 report PREREQUISITE NOT MET, i.e. NOT MEASURED, never a pass and never a red:check:test-completeness(exit 3 — grades a saved turbo test log, none exists locally),check:dual-build-cjs-loads(exit 3 — needs a fullpnpm build),check:i18nandcheck:type-check-debt(both need a built workspace closure; both refuse rather than measure). CI builds the workspace and runs all four.slot-lookuperasure ratchet did not move; the gate's own verdict line:✓ slot-lookup ratchet holds: 106 unswept site(s) in 25 file(s), none new, and every file in the population parsed.pnpm lint, which CI owns and runs regardless.pnpm exec eslint --no-inline-config --format jsonon both paths — 2 files linted, 0 errors, 0 warnings; both were accepted into eslint's own population rather than ignored (they are returned as results). The narrowing excludes nothing: this repo runs oneeslint.config.mjswhich never enables type-aware linting for any file (noparserOptions.project, no typed rules — stated and measured in the config's own header), so no rule's verdict on an untouched file is a function of this diff. Independently,check:slot-lookupexecutes ESLint across all ofpackages/with the baseline's ignores lifted, and holds.Changeset
.changeset/storage-probe-cleanup-target-store.md,patchon@objectstack/service-storage: the behaviour an operator can observe does move — the probe no longer leaves objects in the store it wrote to — even though no API surface does.Generated by Claude Code
Generated by Claude Code