From a8252e150d4b1250cd89bd96a64383bdb6f02044 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 10 Aug 2026 15:08:38 +0000 Subject: [PATCH 1/2] =?UTF-8?q?docs:=20resolve=20discussion=20#14=20?= =?UTF-8?q?=E2=80=94=20keyed=20capability=20surface=20(ADR-0011,=20WIP)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds ADR-0011 and amends 0001/0005/0006/0007/0010 + architecture.md + state-of-play.md. Settled (D4-D9): - delete_many becomes a free function, not a store method. Destructive + key-taking + delegated is the shape already destroying data in sibling packages. - Option E (capability as a Mapping-valued ATTRIBUTE) deferred: verified that a wrapper does NOT re-wrap such an attribute, so it needs the same key resolution as everything else and buys ergonomics, not correctness. - Option D (rebind delegated methods) rejected, citing dol's own issue-18 design doc: rebinding binds to the INNERMOST wrap, so it does not fix the Pipe case it exists to fix. - Prefix pushdown closed rather than deferred: a hint protocol would have one implementer, violating ADR-0009's own rule. - New upstream findings: export dol.dig.inner_most_key; dol.content_url resolves with the outer key; dol.filesys.is_valid_key is confirmed-live broken; wrapped_self has a lost-reference hole. Key finding that reframes ADR-0001: azuredol is robust because its container store has ~zero key-taking methods (the rich surface is on BlobHandle, keyed at construction), not because its prefix lives in the leaf. Prefix-in-leaf is necessary, not sufficient. D1/D2/D3 (how capabilities are surfaced) left as Proposed. An adversarial review refuted three claims of the first draft -- corrections folded in: - "undetectable" was false; a guard is writable - free functions are not categorically safe (they break on non-Store layers, where the method form is right) - s3_store(...).handle(k) is NOT an instance of the bug; only a user-applied key codec triggers it and surfaced a fourth option (capability as a sibling STORE, keyed through __getitem__ -- correct by construction) that the draft never evaluated. See the "Open fork" section. Every empirical claim was executed against the local dol 0.3.58 checkout; repros are in state-of-play.md section 9. --- misc/docs/architecture.md | 90 +++- .../decisions/0001-layered-architecture.md | 37 ++ misc/docs/decisions/0005-large-object-io.md | 2 +- .../0006-key-scoping-and-dol-fixes.md | 113 ++++- .../0007-naming-and-compatibility.md | 2 +- .../0010-bucket-and-bulk-operations.md | 11 + .../0011-keyed-capability-surface.md | 418 ++++++++++++++++++ misc/docs/state-of-play.md | 174 +++++++- 8 files changed, 804 insertions(+), 43 deletions(-) create mode 100644 misc/docs/decisions/0011-keyed-capability-surface.md diff --git a/misc/docs/architecture.md b/misc/docs/architecture.md index 639d47d..ad16798 100644 --- a/misc/docs/architecture.md +++ b/misc/docs/architecture.md @@ -68,9 +68,14 @@ load-bearing rather than decorative: > above an absolute-keyed leaf. That does not work, and the reasons are recorded in > [ADR-0001](decisions/0001-layered-architecture.md) §"Why the prefix lives in the leaf". > In short: `dol` wrappers delegate unknown attributes with the **outer, unmapped** key, so - > every capability method (`url_for`, `info`, `handle`, `sub`, `prefixes`, `delete_many`) - > silently addresses the wrong object; and there is no channel for a wrapper to push its - > prefix into the leaf's listing call, so every scoped listing becomes a full-bucket scan. + > every capability method silently addresses the wrong object; and there is no channel for a + > wrapper to push its prefix into the leaf's listing call, so every scoped listing becomes a + > full-bucket scan. + > + > **Necessary, not sufficient.** Prefix-in-leaf removes the seam *we* introduce; a user who + > stacks a `dol` key codec on top reintroduces it. The second half of the answer is to have + > almost no keyed methods to begin with — + > [ADR-0011](decisions/0011-keyed-capability-surface.md). - **Layer C never subclasses.** If a recipe cannot be expressed as a composition of Layer B plus `dol` wrappers, that is a signal the capability belongs in Layer B as a parameter, @@ -109,8 +114,8 @@ Follows `dol.filesys`' triangle, in S3 vocabulary: ``` BucketCollection (Collection — __iter__ over object keys) - └── BucketReader (+ __getitem__ -> bytes, url_for, info, handle, sub, prefixes) - └── BucketStore (+ __setitem__ / __delitem__ / delete_many) + └── BucketReader (+ __getitem__ -> bytes) + └── BucketStore (+ __setitem__ / __delitem__) EndpointCollection (Collection — __iter__ over bucket names) └── EndpointReader (+ __getitem__ -> BucketReader) @@ -118,7 +123,23 @@ EndpointCollection (Collection — __iter__ over bucket names) ``` plus `ObjectHandle` — the escape hatch for one object, which is **not** a Mapping and is -where ranged reads, streaming, multipart and object metadata live. +where ranged reads, streaming, multipart, presigned URLs and object metadata live. +`ObjectHandle` binds its key **at construction**, which is what makes it immune to the +delegation trap ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D1, following +`azuredol`'s `BlobHandle`). + +> **Layer B has no key-taking public methods at all.** The Mapping dunders are the whole keyed +> surface, because `dol` maps *those* correctly at any wrapper depth. Every capability — +> `handle`, `sub`, `prefixes`, `url_for`, `info`, `delete_many` — is a **free function taking +> the store first**. +> +> This is not stylistic. A `dol` key wrapper hands any non-dunder method the *outer, unmapped* +> key; and the obvious hardening (`inner_most_key(wrapped_self(self), k)`) is **silently wrong +> when the store is a temporary**, as in `s3_store(...).handle(k)` — the wrapper is collected +> before the method body runs, and the failure is undetectable. Free functions receive the store +> as an argument, so it stays alive and the resolution is reliable: measured 6/6 correct versus +> 2/4 for the method form. See [ADR-0011](decisions/0011-keyed-capability-surface.md) §D1a. A +> reflective conformance test enforces an **empty** allowlist ([ADR-0011] §D5). `EndpointStore`, not `BucketsStore`: naming the *containing* resource (as `azuredol` does with `ContainerStore`/`AccountStore`) avoids shipping `BucketStore` and `BucketsStore` — two of the @@ -134,18 +155,34 @@ yields a working, silently-wrong store. | `__iter__()` | Lazy paginated `ListObjectsV2(Prefix=self.prefix)`. **Raises** if the bucket is missing or unlistable — never yields empty. | | `__len__()` | **Not implemented.** Raises `TypeError` with guidance. See [0008](decisions/0008-testing-architecture.md) §cost model. | | `__repr__` | bucket, prefix, endpoint host, mode. No secrets, no addresses. | -| `url_for(k, ...)` | Presigned URL. Zero object requests. **Always SigV4** — see [0003](decisions/0003-provider-presets-and-capabilities.md) §4. **The URL cannot outlive the signing credential**: with STS/SSO/instance-profile credentials it dies with the session (default 1 h) regardless of `expires_in`. Capped at 604800 s (SigV4's limit, which botocore does not enforce); clamps and warns when the resolved credentials expire sooner. | -| `sub(prefix)` | A `BucketReader`/`BucketStore` with an extended, normalized prefix. Zero round-trips. | -| `info(k)` | One `HeadObject` → size, mtime, etag, content-type, storage class, restore status. | -| `prefixes(p='')` | One `ListObjectsV2(Delimiter='/')` → `CommonPrefixes`, relative to the store's own prefix. | -| `delete_many(keys)` | See [0010](decisions/0010-bucket-and-bulk-operations.md). | - -**The rule for what may join this table** (it is otherwise how a surface grows to twenty): a -method may be added iff it takes a key and is either a pure read of metadata or an address -(`info`, `url_for`), or returns a store or handle (`sub`, `handle`, `prefixes`). Anything that -mutates, batches, or takes non-key arguments belongs on `ObjectHandle` or a recipe. -`delete_many` is an explicit, named exception, admitted only because the cost difference -against a `__delitem__` loop is an order of magnitude. + +The capabilities, as **free functions** ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D3): + +| Function | Contract | +|---|---| +| `s3dol.handle(store, k)` | An `ObjectHandle` bound to `k`. Zero round-trips. The per-object entry point. | +| `s3dol.sub(store, prefix)` | A store with an extended, normalized prefix, **in the caller's key space**. Zero round-trips on an unwrapped store, where it pushes down; on a wrapped store it composes `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` over the outer store and loses pushdown (§D2, §D8). | +| `s3dol.prefixes(store)` | One `ListObjectsV2(Delimiter='/')` → `CommonPrefixes`, mapped back out to the caller's key space (§D2). | +| `s3dol.url_for(store, k)` | `handle(store, k).url()`. | +| `s3dol.info(store, k)` | `handle(store, k).info()`. | +| `s3dol.delete_many(store, keys)` | See [0010](decisions/0010-bucket-and-bulk-operations.md). | + +On `ObjectHandle` (key bound at construction, so no delegation seam — these are methods): + +| Operation | Contract | +|---|---| +| `url(...)` | Presigned URL. Zero object requests. **Always SigV4** — see [0003](decisions/0003-provider-presets-and-capabilities.md) §4. **The URL cannot outlive the signing credential**: with STS/SSO/instance-profile credentials it dies with the session (default 1 h) regardless of `expires_in`. Capped at 604800 s (SigV4's limit, which botocore does not enforce); clamps and warns when the resolved credentials expire sooner. | +| `info()` | One `HeadObject` → size, mtime, etag, content-type, storage class, restore status. | + +**The rule for what may join the Layer B table** (it is otherwise how a surface grows to +twenty): **nothing that takes a key.** A method may be added iff it takes no key at all. +Anything addressing one object belongs on `ObjectHandle`; anything else belongs in a free +function or a recipe. + +The reason is mechanical, not aesthetic: `dol` hands any non-dunder method the outer, unmapped +key when the store is key-wrapped, and the standard workaround is itself silently wrong on +temporaries. `azuredol` — the reference implementation — has ~zero keyed methods for the same +reason. See [ADR-0011](decisions/0011-keyed-capability-surface.md). ### Layer C — `s3dol.recipes` @@ -188,7 +225,10 @@ s3dol/ values.py Filepath / Chunks / Streamable refs; as_fileobj singledispatch writes.py write strategies (simple / transfer / multipart) reads.py read strategies (bytes / stream / ranged / to-file) - base.py Layer B (owns prefix) + base.py Layer B (owns prefix). NO key-taking public methods — ADR-0011 §D1 + capabilities.py the keyed API as free functions: handle / sub / prefixes / + url_for / info / delete_many, plus _abs_key, the one + key-resolution primitive (ADR-0011 §D2/§D3) recipes.py Layer C — s3_store(...), codec facades diagnose.py s3dol.diagnose() — prints resolved endpoint/region/credential SOURCE store.py COMPAT SHIM — legacy S3Store, DeprecationWarning, removed in v2 @@ -201,7 +241,8 @@ s3dol/ can silently move a live data target ([ADR-0007](decisions/0007-naming-and-compatibility.md) §5). **Public API** — `s3_store`, `BucketStore`, `BucketReader`, `EndpointStore`, `ObjectHandle`, -`S3Connection`, `Filepath`/`Chunks`/`Streamable`, the error classes, `diagnose`. Everything +`S3Connection`, `Filepath`/`Chunks`/`Streamable`, the error classes, `diagnose`, and the keyed +free functions `handle` / `sub` / `prefixes` / `url_for` / `info` / `delete_many`. Everything else is implementation. `store.py` must survive as an importable module: both external dependents do @@ -235,8 +276,13 @@ else is implementation. question **we deliberately give the same answer** — the `*dol` family's value is that one adapter reads like the next. Concretely inherited: the layering, the Collection→Reader→Store triads, the single error-translation decorator, no `__len__`, no global client cache, real -reader classes, and the refusal to cascade-delete a container (its `design_decisions.md` §12 -cites s3dol v0 by name: *"This is convenient and dangerous. We refuse it."*). +reader classes, the refusal to cascade-delete a container (its `design_decisions.md` §12 +cites s3dol v0 by name: *"This is convenient and dangerous. We refuse it."*), and — the one that +took longest to see — **a container store with essentially no key-taking public methods**, with +the rich per-object surface on a handle that binds its key at construction +(`azuredol/base.py:233 BlobHandle`). That last property, not the prefix location, is what makes +it robust under user-applied key codecs +([ADR-0011](decisions/0011-keyed-capability-surface.md) §D1). **Read `azuredol`'s code, not only its docs.** Its `architecture.md` says container stores are wrapped with `dol`'s `mk_relative_path_store`; its actual `base.py` does prefix diff --git a/misc/docs/decisions/0001-layered-architecture.md b/misc/docs/decisions/0001-layered-architecture.md index 78c29d4..dc35460 100644 --- a/misc/docs/decisions/0001-layered-architecture.md +++ b/misc/docs/decisions/0001-layered-architecture.md @@ -3,6 +3,10 @@ - **Status:** Accepted - **Date:** 2026-08-10 - **Supersedes:** the v0.1.x `base.py` / `store.py` / `utility.py` split +- **Amended by:** [ADR-0011](0011-keyed-capability-surface.md) — Layer B has **no** key-taking + public methods. `url_for`/`info` move onto `ObjectHandle` (key bound at construction); + `handle`, `sub`, `prefixes` and `delete_many` become free functions taking the store first. + See §"Prefix-in-leaf is necessary, not sufficient" below. ## Context @@ -91,6 +95,39 @@ appears **zero times** in the package. azuredol has none of the six bugs above p because it does the thing this ADR now specifies. The lesson generalizes: **a sibling's design doc is a claim; its source is the evidence.** +## Prefix-in-leaf is necessary, not sufficient + +*(Added by [ADR-0011](0011-keyed-capability-surface.md). The section above is correct about why +the prefix must live in the leaf; it drew an incomplete lesson from `azuredol`.)* + +Putting the prefix in the leaf removes the key-mapping seam **for the prefix s3dol owns**. It +does nothing for a user who wraps an s3dol store with a `dol` key codec — every capability +method is then delegated with the outer, unmapped key again, by one of *two* routes +(`Store.__getattr__` at `dol/base.py:742` for instance-wraps and `mk_relative_path_store` +subclasses; `DelegatedAttribute.__get__` at `dol/base.py:279` for class-wraps). + +Re-reading `azuredol`'s code with that in mind gives the sharper finding: **`ContainerStore` has +essentially no key-taking public methods at all.** The rich per-object surface lives on +`BlobHandle` (`azuredol/base.py:233`), which binds its blob **at construction**, so a key codec +over the store cannot corrupt it. `azuredol`'s only residual exposures are +`ContainerCollection.walk` (`base.py:164`, leaf-keyed return) and `AccountStore.delete` +(`base.py:460`, keyed and destructive). + +So `azuredol` is not safe because of where its prefix lives. It is safe because **it has almost +no seam to get wrong.** The table above lists six methods this ADR originally proposed for +Layer B; [ADR-0011](0011-keyed-capability-surface.md) reduces that to **zero**, moving per-object +capabilities onto `ObjectHandle` (key bound at construction) and the rest into free functions. + +Two is not good enough, and the reason is worth knowing before you propose keeping one: the +obvious hardening — `inner_most_key(wrapped_self(self), k)` — is **silently wrong when the +wrapper is a temporary** (`s3_store(...).handle(k)`), because the delegated bound method holds +no reference to the wrapper and the weakref registry entry is removed when it dies. Verified; +see ADR-0011 §D1a. + +The "Consequences" claim below that *"every capability method is key-correct by construction"* +holds only for an unwrapped store. Read it as: *there is no seam **we** introduce* — a user can +still add one. + Reader-only classes are **real classes**, not instances with methods deleted. `dol`'s `mk_read_only` is not merely bypassable — it is **non-functional on a `dol` store**: verified, `ro['a'] = b'2'` on `mk_read_only(Store({...}))` succeeds silently, no exception. Real classes diff --git a/misc/docs/decisions/0005-large-object-io.md b/misc/docs/decisions/0005-large-object-io.md index 4aa9052..7c3241e 100644 --- a/misc/docs/decisions/0005-large-object-io.md +++ b/misc/docs/decisions/0005-large-object-io.md @@ -184,7 +184,7 @@ stream raises from `botocore/httpchecksum.py` *before the request is built*, so in increasing explicitness: a `reads=stream_reads()` strategy at construction (the store's values become chunk iterators — a **runtime** variation, *not* a static one: the class's value type does not change, and honest typing would need `BucketStore(Generic[VT])` plus overloaded -construction, which v1 does not do); `store.handle(k)` for `.open()` / `.stream()` / +construction, which v1 does not do); `s3dol.handle(store, k)` for `.open()` / `.stream()` / `.read(byte_range=...)`; or a `Filepath` destination for download-to-disk. `ObjectHandle` is not a Mapping and is the documented escape hatch — the same role diff --git a/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md b/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md index dcb719c..5600b58 100644 --- a/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md +++ b/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md @@ -76,7 +76,21 @@ Related upstream bug: `dol.trans.filter_prefixes(['logs/', 'tmp/'])` compiles to ### 2. The delegation trap (for anyone stacking `dol` on top) -`dol` wrappers delegate unknown attributes to the leaf **with the outer, unmapped key**: +> **Amended by [ADR-0011](0011-keyed-capability-surface.md), and the headline below is now +> wrong.** `inner_most_key(wrapped_self(self), k)` is **not** a correct escape: it is silently +> wrong whenever the wrapper is a temporary (§D1a), and the failure is undetectable. s3dol's +> answer is instead to have **no key-taking methods at all** and to resolve keys in free +> functions that receive the store as an argument. Read this section for the *mechanism* — it is +> accurate about how the trap works — but take the remedy from ADR-0011, not from here. + +`dol` wrappers delegate unknown attributes to the leaf **with the outer, unmapped key**. There +are **two** routes, and a fix that covers one is a silent no-op on the other: + +| route | when | site | +|---|---|---| +| `Store.__getattr__` | instance-wraps; `mk_relative_path_store` subclasses | `dol/base.py:742` | +| `DelegatedAttribute.__get__` | class-wraps (one descriptor per attr of `dir(wrapped)`) | `dol/base.py:279` | + ```python w = KeyCodecs.prefixed("a/")(WithUrl)(...) @@ -89,7 +103,9 @@ w.url_for("b") # -> https://x/b WRONG: should be https://x/a/b `isinstance` uses `getattr_static`, which sees a **class**-wrapped capability but not an **instance**-wrapped one. Capability detection must not rely on it.) -The correct escape is **`inner_most_key(wrapped_self(self), k)`**. +The escape this ADR originally prescribed was **`inner_most_key(wrapped_self(self), k)`**. +**Do not use it** — trap 4 below shows it is silently wrong on temporary wrappers. It is kept +here because traps 1–3 apply to the *replacement* too, and because the reasoning matters. > `inner_most_key(self, k)` — which an earlier revision of this ADR prescribed — returns > **`None`**, silently: inside a delegated method `self` *is* the unwrapped leaf (dol issue @@ -99,6 +115,61 @@ The correct escape is **`inner_most_key(wrapped_self(self), k)`**. project. `url_for` must additionally **raise** when the mapped key is not a `str`, so the `None` failure mode is impossible rather than merely documented. +**Four things about that form, all verified, none of them obvious. The fourth retires it.** + +1. **`inner_most_key` is not importable from `dol`.** It lives in `dol.dig` + (`dol/dig.py:69`); `dol/__init__.py` exports only `trace_getitem` from that module. The + import is `from dol.dig import inner_most_key`. (`wrapped_self` *is* exported from `dol`.) + Making it public is on the upstream list in §3. + +2. **It REPLACES `self._id_of_key(k)`; it never composes with it.** Because §1 of this ADR puts + the prefix in the leaf, `inner_most_key` walks the whole chain *including the leaf's own + `_id_of_key`*, so it already returns the fully-prefixed absolute key. Composing them + double-prefixes, silently — and `self._id_of_key(k)` is exactly what a capability method's + author will reach for: + + ```python + # store scoped to 'logs/', outer key 'a.txt' + inner_most_key(wrapped_self(self), k) # 'logs/a.txt' correct + self._id_of_key(inner_most_key(wrapped_self(self), k)) # 'logs/logs/a.txt' WRONG + ``` + + Verified correct in all five wrap shapes — unwrapped, `KeyCodecs.prefixed`, `filt_iter` + (key-identity layer), a `Pipe` stack, and a value-only `wrap_kvs`. + +3. **The `str` check is mandatory, not defensive.** `inner_most_key` returns `None` — silently, + via `last_element` over an empty generator — when no layer in the chain supplies + `_id_of_key`. That is what turns a wrong key into `https://…/None`, and in `dol`'s own + `filesys.MakeMissingDirsStoreMixin` it turns a write error into + `TypeError: expected str … not NoneType`. + +4. **It is silently wrong when the wrapper is a temporary — which retires the form.** + `wrapped_self` resolves through a weakref registry keyed by `id(inner)`. A delegated bound + method holds **no reference to the wrapper**, so in a chained expression the wrapper is freed + before the body runs, and `_register_wrapper_backref`'s cleanup callback *removes the + registry entry* — making it indistinguishable from "never wrapped": + + ```python + s = KeyCodecs.prefixed("x/")(BucketReader(data, prefix="logs/")) + s.m_abs("b.txt") # 'logs/x/b.txt' correct (wrapper is named, so alive) + KeyCodecs.prefixed("x/")(BucketReader(data, "logs/")).m_abs("b.txt") + # 'logs/b.txt' WRONG, silently (wrapper was a temporary) + ``` + + And because §1 puts the prefix in the leaf, the wrong answer is a **plausible `str`** — the + leaf's own `_id_of_key` still fires — so trap 3's type check does not catch it. Measured: + free-function form 6/6 correct across wrap × lifetime shapes; method form 2/4. + + Consequence: `wrapped_self` is a **best-effort guardrail, not a correctness mechanism** + (dol#18's own doc: *"a guardrail, not a cure"*). s3dol therefore resolves keys in **free + functions that receive the store as an argument**, where the caller keeps it alive and the + registry is never consulted — [ADR-0011](0011-keyed-capability-surface.md) §D2/§D3. The + temporary-wrapper hole itself goes upstream to dol#18 (ADR-0011 §D9). + +`wrapped_self` does survive `pickle` and `deepcopy` (dol#18's `__setstate__` re-registration +landed) — that part of the story is fine; it is object lifetime, not serialization, that breaks +it. + ### 3. What still goes upstream to `dol` Reduced, now that prefixing lives in the leaf and `wrapped_self` turns out to exist: @@ -112,6 +183,33 @@ Reduced, now that prefixing lives in the leaf and `wrapped_self` turns out to ex the wrapped class deliberately omits. 4. **Document `wrapped_self` as the delegation answer**, and make the family use it. +Added by [ADR-0011](0011-keyed-capability-surface.md), both **blocking for s3dol**: + +5. **Export `inner_most_key`** from `dol/__init__.py`, and **harden `dig.store_trans_path`** — + raise instead of returning `None` when no layer supplies `_id_of_key`, and fix `dol/dig.py:41` + hardcoding `unravel_key` for the recursive step (which is why `inner_most_val` does not do + what its name says — an acknowledged TODO at `dol/dig.py:71`). s3dol's `_abs_key` depends on + this function being both importable and non-silent. + +6. **Fix `dol.content_url`** (`dol/content.py:210-214`). It resolves with a flat + `getattr(store, 'url_for')(key)` — no chain walk — so through any key wrap it returns a URL + for the unmapped key. `dol/content.py`'s module docstring names an `s3dol` store as the + intended S3 backend and frames `url_for` as the presigned-URL seam, so this **blocks that + integration being correct at all**. Its existing tests are all identity-keyed, which is why + it was never caught. + +7. **Report the `wrapped_self` temporary-wrapper hole** on dol#18 (§2 trap 4). Not blocking — + s3dol routes around it via free functions — but dol ships `wrapped_self` as the *blessed* + pattern, and `xdol` and `unbox` have already adopted it, so they inherit a silent failure + mode. Not s3dol's to fix; s3dol's to report with the repro. + +Adjacent, non-blocking, and worth fixing because it is the best regression sentinel for any +future delegation fix: **`dol.filesys.FileSysCollection.is_valid_key`/`validate_key`** +(`dol/filesys.py:422,425`) are confirmed-live broken — `Files(d).is_valid_key(k)` is `False` +for a key that exists, because `Files` is a `mk_relative_path_store` and the leaf's regex matches +absolute paths. `dol/paths.py:1199-1206` already carries the hand-rolled fix for exactly this +shape. + **Policy on upstreams:** never block an s3dol release on a `dol` PR; never let a local copy diverge silently — raise the `dol` floor the day each lands and delete the workaround in the same commit. @@ -150,12 +248,13 @@ it, since botocore adds it. If a provider rejects botocore's auto-`EncodingType` expresses that by unregistering botocore's handler pair — and s3dol then owns the decode. **The trailing-`/` overload is removed**: `store[k]` always returns bytes; sub-stores come -from `store.sub('folder/')`. `store['folder/']` survives only on an explicitly-constructed +from `s3dol.sub(store, 'folder/')` ([ADR-0011](0011-keyed-capability-surface.md) — a free +function, not a method). `store['folder/']` survives only on an explicitly-constructed navigable reader for notebook use (`azuredol` keeps both, and so do we). One consequence to handle explicitly: a view scoped to `a/` relativizes the **exact-prefix marker object** `a/` to the key `''`, which this section forbids. Resolution: the scoped view -filters out the exact-prefix marker, and `store.sub(p)` documents that the parent's own marker +filters out the exact-prefix marker, and `s3dol.sub(store, p)` documents that the parent's own marker is not a key of the child. The marker stays addressable by its absolute key on an unscoped store. (Directory markers matter: a filesystem migration creates them for empty directories.) @@ -175,3 +274,9 @@ justified only because the alternative is a key the store iterates but refuses t 3. Never pass `EncodingType`. 4. Never assert on a presigned URL by substring ([ADR-0008](0008-testing-architecture.md)). 5. Never write `inner_most_key(self, k)` inside a delegated method. +6. Never resolve a key inside a delegated method **at all** — not even with + `wrapped_self`. Resolve it in a free function that receives the store (§2 trap 4, + [ADR-0011](0011-keyed-capability-surface.md) §D1a/§D3). +7. Never compose `inner_most_key(store, k)` with `store._id_of_key(...)`. It already includes + the leaf's prefix; composing double-prefixes silently (§2). +8. Never use the result without checking it is a `str` — `None` is a legal return (§2). diff --git a/misc/docs/decisions/0007-naming-and-compatibility.md b/misc/docs/decisions/0007-naming-and-compatibility.md index 1c786d2..b462b5c 100644 --- a/misc/docs/decisions/0007-naming-and-compatibility.md +++ b/misc/docs/decisions/0007-naming-and-compatibility.md @@ -109,7 +109,7 @@ These are bug fixes, and preserving them would mean preserving data-misrouting: | `list(store)` returns `[]` on any error | raises | no | | `del endpoint[name]` cascades, unpaginated | refuses non-empty; `force=True` is explicit | no | | `store[k] = 'a str'` accepted | `TypeError` ([ADR-0005](0005-large-object-io.md) §2) | **yes** — shim wraps with `str.encode` + `DeprecationWarning` | -| `store['folder/']` returns a sub-store | removed; use `store.sub()` | **yes** | +| `store['folder/']` returns a sub-store | removed; use `s3dol.sub(store, …)` ([ADR-0011](0011-keyed-capability-surface.md)) | **yes** | | `del store[absent]` silently succeeds | still idempotent ([ADR-0010](0010-bucket-and-bulk-operations.md)) | n/a — unchanged | The last three rows exist because `http_cosmo_prep`'s **currently-passing** tests do all diff --git a/misc/docs/decisions/0010-bucket-and-bulk-operations.md b/misc/docs/decisions/0010-bucket-and-bulk-operations.md index 44f24c9..65be30a 100644 --- a/misc/docs/decisions/0010-bucket-and-bulk-operations.md +++ b/misc/docs/decisions/0010-bucket-and-bulk-operations.md @@ -53,6 +53,17 @@ ADR set exists to remove — but the README must show `on_missing_bucket='create ### 2. `delete_many(keys)` +> **Amended by [ADR-0011](0011-keyed-capability-surface.md) §D4: `delete_many` is a free +> function, `s3dol.delete_many(store, keys)`, not a store method.** The semantics below are +> unchanged; only the surface moves. +> +> Reason: a keyed, destructive, *delegated* method is the exact combination that the `*dol` +> family census found already destroying the wrong data in three sibling packages +> (`cosmodol.CosmosItems.batch`, `cosmodol.CosmosDatabase.delete`, `azuredol.AccountStore.delete`). +> As a method it would be handed the outer, unmapped key by any key wrap and would delete +> objects the caller cannot see; as a free function it resolves the key through the whole +> wrapper chain first. See [dol#83](https://github.com/i2mint/dol/issues/83). + Chunks at **1000** (AWS's cap; moto accepts 1001, so tier 2 cannot catch a missing chunker), parses the `Errors` list out of what is an **HTTP 200** response, and on partial failure raises a single `S3PartialFailure(S3Error)` carrying `.succeeded: list[str]` and diff --git a/misc/docs/decisions/0011-keyed-capability-surface.md b/misc/docs/decisions/0011-keyed-capability-surface.md new file mode 100644 index 0000000..348d0e5 --- /dev/null +++ b/misc/docs/decisions/0011-keyed-capability-surface.md @@ -0,0 +1,418 @@ +# ADR-0011: The keyed capability surface and the unmapped-key problem + +- **Status:** **Proposed** — D1/D2/D3 are under revision; see §"Open fork" below. D4–D9 are settled. +- **Date:** 2026-08-10 +- **Discussion:** [#14](https://github.com/i2mint/s3dol/discussions/14) +- **Amends:** [ADR-0001](0001-layered-architecture.md) (Layer B method table), + [ADR-0006](0006-key-scoping-and-dol-fixes.md) §2 (the escape form), + [ADR-0010](0010-bucket-and-bulk-operations.md) §2 (`delete_many`'s surface) +- **Upstream:** [dol#83](https://github.com/i2mint/dol/issues/83) (this problem), + [dol#18](https://github.com/i2mint/dol/issues/18) (its root) + +## Context + +[ADR-0001](0001-layered-architecture.md) puts the prefix in the leaf, which removes the +key-mapping seam between a capability method and the wire **for the prefix s3dol owns**. It does +not remove the seam in general: a user who wraps an s3dol store with any `dol` key codec still +gets a silently wrong `url_for`. Discussion #14 asked how to fix that properly. + +### The mechanism, precisely + +`dol` wraps by **delegation (has-a)**. Key transforms are applied by the wrapper's +`__getitem__` / `__setitem__` / `__delitem__` / `__contains__` / `__iter__`. Every other method +is handed the **outer, unmapped** key. There are **two** delegation routes, not one: + +| route | when | site | +|---|---|---| +| `Store.__getattr__` | instance-wraps, and `mk_relative_path_store` subclasses | `dol/base.py:742` | +| `DelegatedAttribute.__get__` | class-wraps (`delegate_to` installs one descriptor per attr of `dir(wrapped)`) | `dol/base.py:279`, installed at `dol/base.py:416-480` | + +Both return the method **bound to the leaf**. #14's original framing named only the first; any +fix that covers one and not the other is a silent no-op on half the cases. + +Nothing raises, and capability detection cannot see it: a `@runtime_checkable` Protocol checks +method *presence* only. It is also wrap-dependent — since 3.12 `isinstance` uses +`getattr_static`, so `isinstance(w, SupportsUrlFor)` is `True` for a **class**-wrapped +capability but `False` for an **instance**-wrapped one. Either way it tells you nothing about +whether the key is right. + +### What the family census showed + +Surveyed across ~20 `*dol` packages, this is not a latent tidiness issue. Already shipping: +destructive key-taking delegated methods in `cosmodol` (`CosmosItems.replace`/`batch`, +`CosmosDatabase.delete`, `CosmosAccount.delete`), `azuredol` (`AccountStore.delete`), +`pydrivedol` (`GDStore.upload`, and `GDReader.get_url` which *also* grants anyone/reader +permission on the wrong file), `aiofiledol`, and `sshdol.sync_to` (rsync `--delete` over the +leaf's whole rootdir). And a **confirmed-live** bug in `dol` itself: +`dol.filesys.Files(d).is_valid_key(k)` returns `False` for a key that exists. + +The full census, with per-symbol verdicts, lives on +[dol#83](https://github.com/i2mint/dol/issues/83). + +## Decision + +### D1 — Shrink the keyed surface. This is the primary decision. + +[ADR-0001](0001-layered-architecture.md)'s Layer B table proposed **six** keyed methods +(`url_for`, `info`, `handle`, `sub`, `prefixes`, `delete_many`). `azuredol` — the reference +implementation for our layering — has **~zero**: `ContainerStore` is deliberately method-free, +and the rich per-object surface lives on `BlobHandle` (`azuredol/base.py:233`), which takes its +blob **at construction**, so wrapping the store with a key codec cannot corrupt it. Its only +residual exposures are `ContainerCollection.walk` (`base.py:164`) and `AccountStore.delete` +(`base.py:460`). + +**`azuredol` is not safe because the prefix lives in the leaf. It is safe because it has almost +no seam to get wrong.** Prefix-in-leaf is necessary, not sufficient. That is the finding, and +ADR-0001 previously drew the wrong lesson from the same source. + +So: + +- `url_for` and `info` **move onto `ObjectHandle`** — `ObjectHandle` binds its key at + construction, azuredol-style, so no key ever crosses a delegation seam to reach them. +- `delete_many` leaves the store entirely — see D4. +- **`handle` and `sub` become free functions too**, and Layer B ends up with **zero** key-taking + public methods. That is stronger than this ADR originally proposed, and it is forced by the + finding below rather than chosen for symmetry. + +### D1a — Why zero and not two: a delegated method cannot be made reliably key-correct + +The first draft of this ADR kept `handle(k)` and `sub(prefix)` as methods, hardened by D2's +`inner_most_key(wrapped_self(self), k)`. **Testing that plan refuted it.** + +`wrapped_self` resolves the outer store through a weakref registry keyed by `id(inner)` +(`dol/base.py`, `_wrapper_backrefs`). A `DelegatedAttribute` returns a method bound to the +**leaf**, holding no reference to the wrapper. So in a chained expression the wrapper is a +temporary that is freed by the time the method body runs, its weakref dies, and the registry +entry is **removed by the cleanup callback** — leaving a state indistinguishable from "never +wrapped": + +```python +s = KeyCodecs.prefixed('x/')(BucketReader(data, prefix='logs/')) +s.m_abs('b.txt') # 'logs/x/b.txt' correct +KeyCodecs.prefixed('x/')(BucketReader(data, 'logs/')).m_abs('b.txt') + # 'logs/b.txt' WRONG, silently +``` + +**The wrong answer is a plausible `str`**, so a type check does not catch it. Precisely +*because* ADR-0001 puts the prefix in the leaf, the leaf's own `_id_of_key` still fires and +produces a well-formed key that addresses the wrong object. A leaf with no `_id_of_key` would at +least return `None`. + +Measured, method form vs free function, across wrap shape × wrapper lifetime: + +| form | unwrapped (named/temp) | key-codec + named | key-codec + **temp** | `Pipe` + named | `Pipe` + **temp** | +|---|---|---|---|---|---| +| method via `wrapped_self` | correct | correct | **silently wrong** | correct | **silently wrong** | +| free function | correct | correct | correct | correct | correct | + +**Scope of the failure — stated precisely, because an earlier draft overstated it.** The +precondition is *a user-applied **key** codec* **and** *no live strong reference to the wrapper +at call time*. It is **not** triggered by `s3_store('bucket', prefix='p').handle(k)`: `s3_store` +returns a bare leaf or a **value**-codec wrap, and both are correct as temporaries (verified). +Nor by `filt_iter` alone. The failing shape is +`KeyCodecs.prefixed('x/')(s3_store(...)).handle(k)`. + +"Temporary" is also the wrong word: the predicate is *no live strong reference*. A temporary +caught in a reference cycle silently starts working, and `operator.methodcaller('m', k)(obj)` is +correct where `obj.m(k)` is not — so the failure is **intermittent**, which is worse than +deterministic even though it is rarer. + +**It is detectable, contrary to an earlier draft of this section.** `Store.__init__` probes the +leaf with `hasattr(self.store, "KeysView")`, so a leaf can record that it was ever wrapped and +then refuse when `wrapped_self(self) is self` but that flag is set. Verified: it catches the +instance-wrap, class-wrap and `Pipe` temporary cases and stays silent on a genuinely unwrapped +store. Caveats: it rides on an incidental probe, it does not fire on the unpickle path (which +re-registers via `__setstate__`, bypassing `__init__`), and it false-positives on a leaf that +was wrapped once and is later used bare. A two-line upstream change to +`_register_wrapper_backref` would do it properly. + +So the honest conclusion is narrower than "the method form is impossible": **the method form can +be made loud rather than silently wrong, at the cost of a hack.** Whether that is worth keeping +is the open fork below. + +This does still demote `wrapped_self` generally: it is a **best-effort guardrail with a silent +failure mode**, not "the correct escape" that +[ADR-0006](0006-key-scoping-and-dol-fixes.md) §2 called it. The hole goes upstream (D9). + +### D2 — One key-resolution primitive, and it takes the store as an argument + +```python +from dol.dig import inner_most_key # NOT exported from `dol` — see ADR-0006 §3 + + +def _abs_key(store, k: str) -> str: + """The absolute S3 key for `k` in `store`. Correct at any wrapper depth.""" + _id = inner_most_key(store, k) + if not isinstance(_id, str): + raise KeyNotValid(...) # never let a None reach the wire + return _id +``` + +**`store` is a parameter, not `self`.** The caller's expression holds the store alive for the +duration of the call, `inner_most_key` walks the real `.store` chain, and the `wrapped_self` +weakref registry is never consulted — so D1a's hole cannot occur. Verified correct across +wrapped/unwrapped × named/temporary and a `Pipe` stack, where the method form fails two of +those. + +**But it is not categorically safe, and this ADR must not claim it is.** `inner_most_key` walks +`.store` applying each layer's `_id_of_key`, which breaks when a layer in the chain is **not** a +`dol` `Store`. Verified against a ground-truth oracle: + +| chain | wire key | free function | method form | +|---|---|---|---| +| `KeyCodecs` / `Pipe` / `cached_keys` / `filt_iter` / `wrap_kvs` over the leaf | — | correct | correct (if named) | +| hand-rolled `__getattr__`-passthrough delegator over the leaf | `logs/b.txt` | **`logs/logs/b.txt`** | correct | +| same delegator over a key codec | `logs/x/b.txt` | **`logs/x/x/b.txt`** | correct | +| key codec over a plain `.store`-holding middle layer | `logs/x/b.txt` | **`x/b.txt`** | also wrong | + +Cause: a passthrough `__getattr__` resolves `_id_of_key` to the *leaf's* bound method, so the +walk applies it once at the delegator and again at the leaf; a middle layer with no `_id_of_key` +truncates the walk instead. Every `dol`-shipped wrapper is safe because every `Store` subclass +inherits an identity `_id_of_key` — but `dol/base.py` ships a documented hand-rolled `Delegator` +recipe of exactly the breaking shape. + +So the free function is *more* reliable than the method form, not *reliable*. Any claim of +"correct at any wrapper depth" is false and must not appear in the docstring. + +**It replaces `_id_of_key`. It never composes with it.** Because ADR-0001 puts the prefix in the +leaf, `inner_most_key` walks the whole chain *including the leaf's own `_id_of_key`*, so it +already returns the fully-prefixed absolute key. Composing the two double-prefixes, silently: + +```python +# store scoped to 'logs/', outer key 'a.txt' +inner_most_key(store, k) # 'logs/a.txt' correct +store._id_of_key(inner_most_key(store, k)) # 'logs/logs/a.txt' WRONG +``` + +That instinct — reach for `_id_of_key` — is exactly what an author will have, so the rule is +restated in ADR-0006 §2. + +The `str` check is **mandatory, not defensive**: `inner_most_key` returns `None` — silently, via +`last_element` over an empty generator — when no layer in the chain supplies `_id_of_key`. + +**Two implementation obligations this creates**, because not every capability is a pure +key→address mapping: + +- **`sub(store, prefix)`** must return a store *in the caller's key space*. Resolving the + absolute prefix is not sufficient: a leaf sub-store built from it would speak the leaf's key + space, silently dropping the user's outer codec. On an unwrapped s3dol store, use + `leaf._with(prefix=…)` (cheap, pushes down). On a wrapped store, compose over the **outer** + store with dol's own safe form, `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` + ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §1) — correct, but it loses pushdown, which is + the accepted cost in D8. +- **`prefixes(store)`** returns keys relative to the store, so its results must be mapped + **outward** through the chain (`_key_of_id` per layer, outermost last) — the inverse of + `inner_most_key`, which `dol.dig` does not currently provide. Implement it locally and add a + round-trip property test (`∀ p: abs → rel → abs` is identity). + +### D3 — Free functions are the *only* reliable form, so they are the whole keyed API + +```python +s3dol.handle(store, k) # -> ObjectHandle (then .url(), .info(), ranged reads, …) +s3dol.url_for(store, k) # == s3dol.handle(store, k).url() +s3dol.info(store, k) # == s3dol.handle(store, k).info() +s3dol.sub(store, prefix) # -> a store in the CALLER's key space (see D2) +s3dol.prefixes(store) # -> relative to the caller's key space (see D2) +s3dol.delete_many(store, keys) +``` + +The function resolves the key through the whole wrapper chain once, then acts. This is already +`dol`'s idiom (`dol.content_url`, `get_content`, `put_content`, `add_content` all take the store +first), it composes at any wrapper depth, it is unaffected by whether `dol` wraps by has-a or +is-a, and — per D1a — it is the only form that is correct when the store is a temporary. + +D1a upgrades this from "the safe general form, offered alongside methods" to "the form". There +is no method variant to fall back to, because a method variant would be right most of the time +and silently wrong the rest, which is the worst available option. + +**What remains ergonomic without a keyed method:** `__getitem__` *is* correctly key-mapped by +`dol` at every wrapper depth, so `store[k]`, `store[k] = v`, `del store[k]`, `k in store` and +iteration stay the primary surface and stay correct — which is the Mapping-first promise in +[architecture.md](../architecture.md) goal 1. Only the *capabilities* move out of method +position, and they were always the opt-in part. + +**With one correction to the argument #14 made for it:** `dol.content_url` does *not* currently +resolve through the chain — it does a flat `getattr(store, 'url_for')(key)` +(`dol/content.py:210-214`) and returns a URL for the unmapped key. The *shape* is prior art; the +*resolution* is not. Fixing it upstream is a prerequisite for s3dol to serve as `dol.content`'s +S3 backend, which `dol/content.py`'s own module docstring names as the intended arrangement. + +### D4 — `delete_many` is a free function only + +No method on the store. Destructive **+** key-taking **+** delegated is the exact combination +the census found already destroying the wrong data in three sibling packages. We do not add a +fourth. [ADR-0010](0010-bucket-and-bulk-operations.md) §2's semantics — 1000-key chunking, the +HTTP-200 `Errors` parse, `S3PartialFailure` — are unchanged; only the surface moves. + +### D5 — Option B's guard, without Option B's machinery + +No `_key_methods` registry. Instead, a **reflective conformance test** that enumerates the +public methods of every Layer B class and fails on **any** that takes a key-shaped first +argument. Under D1a the allowlist is *empty*, which makes the test a simple, unarguable +invariant rather than a list someone has to curate. + +#14 observed that a declarative registry fails the same silent way when an author forgets to +declare a method, and that the reflective test is "the kind of guard that actually holds". Since +the test is the part that holds and the registry is the part that can be forgotten, we ship the +test and skip the registry. + +**D5 is not yet writable**, though: a "key-shaped first argument" heuristic misses +`delete(name, force=True)`, `delete_many(keys)` and `batch(operations)` — three of the shapes +this ADR is most worried about. The predicate has to be "does this method take anything that +gets used as a key", which is a judgement, not a signature check. Resolve with the open fork. + +Note also that `dol` already ships a declarative hook of exactly Option B's shape — +`wrap_kvs(ingoing_key_methods=…, outcoming_key_methods=…)` — which is untested (`dol/trans.py` +carries the TODO) and **verified broken for leaf-defined methods** on both wrap paths. Option B +would mean replacing it, not building on it. + +### D6 — Option E (capabilities as parallel Mappings) is deferred to v1.x + +#14 argued E is the most dol-native option because "key transformation happens through the +Mapping protocol the wrapper already handles correctly." **That is not true at dol 0.3.58.** A +wrapper does not re-wrap a Mapping-valued attribute; `store.urls` returns an inner-keyed mapping +under both class- and instance-wrap. E only works if the view itself calls +`inner_most_key(wrapped_self(...))` — the same primitive as D2. + +So E is a *surface* over D2's mechanism, not an alternative mechanism. It buys ergonomics and +collapses three [#12](https://github.com/i2mint/s3dol/issues/12) deferrals into one shape; it +does not buy correctness, and it costs new machinery. Revisit when `dol`'s `.meta` sidecar +design lands, which is the mechanism E actually wants. + +### D7 — Option D (rebind delegated methods to the wrapper) is rejected + +Not primarily for blast radius. `dol/misc/docs/dol_issue18_design.md` surveyed 26 ecosystem +sites and rejected the rebind family on three verified defects, the fatal one being that +rebinding binds `self` to the **innermost** `Wrap` — so under a `Pipe` stack it *does not fix +the case it exists to fix*, and stacked-codec writes gain a partial-transform corruption +surface. Plus statically-undetectable crashes: a leaf method calling `super().__getitem__(k)` +compiles `super(SomeClass, self)` with `self` now a `Wrap` → `TypeError`. + +That document's Phase-1 instructions say verbatim: *do not touch `DelegatedAttribute.__get__`, +the `delegate_to` copy loop, or the signature graft.* We comply. + +**The terminal fix is `dol`'s is-a wrapping** (dol#18 Approach C, committed for dol 0.4/1.0), +which resolves dol#18 and dol#6 together and makes this ADR's machinery redundant. That is why +the selection criterion here was *correct on dol 0.3.x **and** harmlessly redundant under is-a* — +not *permanent*. D2's helper degrades to a no-op; D3's free functions stay correct; D1's smaller +surface stays desirable on its own merits. + +### D8 — Prefix pushdown is closed, not deferred + +ADR-0001 already solves pushdown for the prefix s3dol owns: the leaf passes +`ListObjectsV2(Prefix=self.prefix)`. The residual case is a user stacking `filt_iter.prefixes(…)` +on top, which is **general predicate pushdown** — `dol` has no framework for it, and a +`__iter__(*, prefix_hint=…)` protocol would have exactly one implementer, violating +[ADR-0009](0009-scope-and-deferrals.md)'s own "no new `Protocol` without two implementers" rule. + +Decision: a user-stacked filter **accepts a full scan**, and `s3dol.sub(store, prefix)` is the +documented cheap path. On an unwrapped store `sub` costs zero round-trips and pushes down, so +the fast route exists and is one call away. This is an answer, not a deferral; do not reopen it +without a second implementer. + +### D9 — New upstream finding: `wrapped_self` has a temporary-wrapper hole + +dol#18 shipped `wrapped_self` as the blessed pattern for delegation-wrapped classes. D1a shows +it silently degrades to the raw leaf whenever the wrapper is a temporary, because the delegated +bound method holds no reference to it and the weakref cleanup removes the evidence. Any +`*dol` package that adopted the blessed pattern — `xdol` and `unbox` have — +inherits this. + +This is not a blocker for s3dol (D3 routes around it entirely), but it belongs upstream on +dol#18 with the repro, because the documented remedy for a *No Silent Failures* project +currently has a silent failure. Possible directions for dol, none of them s3dol's to choose: +have `DelegatedAttribute.__get__` return a wrapper-retaining bound method; keep a strong +reference for the duration of the call; or land is-a wrapping, which removes the registry +entirely. + +## Open fork — how the keyed capabilities are actually surfaced + +An adversarial review of the first draft refuted three of its supporting claims (all corrections +are folded in above) and surfaced a fourth option the draft never evaluated, because D6 +conflated *capability as a Mapping-valued **attribute*** (broken — verified) with *capability as +a sibling **store***, which is a different design. + +**Option S — capability stores.** A capability becomes a Layer B `KvReader` over the same key +space whose `__getitem__` returns the capability: + +```python +class BucketHandles(KvReader): # zero key-taking methods + def _id_of_key(self, k): + return self.prefix + k + + def __getitem__(self, k): + return ObjectHandle(self.bucket, self._id_of_key(k)) +``` + +`__getitem__` is the one thing `dol` maps correctly at **every** depth, so this is correct *by +construction*: no `inner_most_key`, no `wrapped_self`, no private `dol.dig` import, no upstream +PR. Verified 5/5 including a temporary under `Pipe`, under `cached_keys`, and under the +hand-rolled delegator **where the free function is silently wrong**. It also restores `[k]` +ergonomics and subsumes three of the capability features currently deferred in ADR-0009. + +Its cost is real: a user who wraps the data store must wrap the sibling in parallel +(`KeyCodecs.prefixed('x/')` applied to both), because `store.handles` as an *attribute* is the +broken form. And it does not cover non-keyed bulk operations (`delete_many`, `prefixes`), which +stay free functions regardless. + +Three further blockers must be resolved with this fork, in any option: + +1. **`EndpointStore.delete(name, force=True)`** — still specified in + [architecture.md](../architecture.md), [ADR-0010](0010-bucket-and-bulk-operations.md) §3 and + [ADR-0007](0007-naming-and-compatibility.md). It is a public, key-taking, destructive, + delegated Layer B method — structurally identical to `azuredol.AccountStore.delete`, which + this ADR cites as a census exhibit. Either D5's empty allowlist fails on day one, or D5's + "key-shaped first argument" heuristic misses it — and the same heuristic misses + `delete_many(keys)`, `cosmodol`'s `batch(operations)` and `sshdol`'s `sync_to(target)`. D5 + needs a real predicate, not a name heuristic. +2. **`dol.SupportsUrlFor` requires a `url_for` *method***, and `dol.content_url` reaches it with + `getattr(store, 'url_for', None)`. Under a zero-method Layer B, `content_url` returns `None` + for every s3dol store forever. That makes the `dol.content` integration a **protocol change** + upstream, not the "small PR" D3 implies. +3. **`url_for` needs `(endpoint, bucket, key)`, and only the key has a resolution primitive.** + `recursive_get_attr(chain, 'bucket')` returns the *first* layer carrying a `bucket` attribute, + so a middle layer with its own can pair a correctly-resolved key with the wrong bucket. Either + add a `_leaf_of` primitive or state the limitation. + +Also pending, independent of the fork: ADR-0011 must be added to `misc/docs/README.md` (which +still teaches the retired `inner_most_key(wrapped_self(self), k)` form), and the superseded +"a method may be added iff it takes a key…" rule survives verbatim in +[ADR-0005](0005-large-object-io.md) §2 and [ADR-0009](0009-scope-and-deferrals.md) §v1.0 scope. + +## Consequences + +**Buys.** Zero keyed seams instead of six, guarded by an invariant with an empty allowlist +rather than by discipline. A resolution primitive verified correct in 6/6 wrap × lifetime +shapes, where the obvious alternative is 2/4 and fails silently. No destructive delegated method +anywhere in the package. The Mapping surface — which `dol` maps correctly — stays the primary +API. Nothing that has to be unwound when `dol` lands is-a wrapping. + +**Costs.** These are real and this ADR does not pretend otherwise. + +- `store.url_for(k)` becomes `s3dol.url_for(store, k)`. That reads worse, and it is a + divergence from v0 that `store.py`'s compat shim + ([ADR-0007](0007-naming-and-compatibility.md)) must absorb — the shim can keep the method on + the legacy class, since a legacy `S3Store` is not something users key-wrap. +- The capability API no longer tab-completes off a store, which is a genuine loss for the + notebook-explorer use case that [state-of-play](../state-of-play.md) §1 names first. Mitigate + in docs: `s3dol.` is the discovery surface, and `__getitem__`/iteration still cover the + common path. +- `sub` and `prefixes` become non-trivial to implement correctly (D2's two obligations), where + as leaf methods they were three lines. +- s3dol depends on `dol.dig.inner_most_key`, which is not public API — a small upstream PR + ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §3). + +**What NOT to do.** + +1. **Do not add a key-taking method to a Layer B class** — not even "just this one", not even + hardened with `wrapped_self`. D1a is why: the hardened form is silently wrong on temporaries + and the failure is undetectable. Add it to `ObjectHandle` (key bound at construction) or ship + a free function. The conformance test enforces an *empty* allowlist; do not add entries. +2. Do not compose `_abs_key` with `_id_of_key`. Re-read D2. +3. Do not treat `wrapped_self` as a correctness mechanism. It is a guardrail with a known + silent failure mode (D1a, D9). +4. Do not rely on `isinstance(store, SupportsUrlFor)` to detect a capability — a + `@runtime_checkable` Protocol checks presence, not correctness, and since 3.12 `isinstance` + uses `getattr_static`, which sees a class-wrapped capability but not an instance-wrapped one. +5. Do not re-propose rebinding delegated methods. See D7 and the upstream evidence. +6. Do not invent a pushdown hint protocol for one implementer. See D8. diff --git a/misc/docs/state-of-play.md b/misc/docs/state-of-play.md index eb48da9..abfee5f 100644 --- a/misc/docs/state-of-play.md +++ b/misc/docs/state-of-play.md @@ -63,12 +63,13 @@ consumer's requirements. | Deferred scope | **#12** — v1.x interfaces + the `s3dol`/`botodol` line | | Live bug found | **#10** — `url_for` emits SigV2 | | Answered | **#5** (multipart / value types), **discussion #6** (extendable KeyError) | -| Upstream blockers | **i2mint/dol#82** (prefix corruption), **i2mint/dol#83** (delegation with unmapped key) | -| Open design questions | **§7** and **§8** of this document | +| Upstream blockers | **i2mint/dol#82** (prefix corruption), **i2mint/dol#83** (delegation with unmapped key), plus `inner_most_key` export + `content_url` resolution (ADR-0006 §3) | +| Resolved design question | **§7** / discussion **#14** → [ADR-0011](decisions/0011-keyed-capability-surface.md) | +| Open design question | **§8** / discussion **#15** — credential and endpoint resolution | **Nothing has been implemented.** The repo still ships v0.1.9 unchanged. -## 3. The ten ADRs, in one paragraph each +## 3. The eleven ADRs, in one paragraph each **[0001 — Four-layer architecture](decisions/0001-layered-architecture.md).** Three layers (`connection` → `base` → `recipes`), mirroring `azuredol` so one adapter in the family reads @@ -128,7 +129,18 @@ restraint. Rule: no new `Protocol` without two implementers. **[0010 — Bucket and bulk operations](decisions/0010-bucket-and-bulk-operations.md).** `on_missing_bucket='assume'` default (no probe, ever); `delete_many` chunking at 1000 with `S3PartialFailure` (not `ExceptionGroup` — 3.10 floor); cascading delete stays explicit and -paginates. +paginates. *Amended by 0011: `delete_many` is a free function, not a store method.* + +**[0011 — Keyed capability surface](decisions/0011-keyed-capability-surface.md).** Resolves §7 / +discussion #14. **Layer B gets zero key-taking public methods**: per-object capabilities live on +`ObjectHandle` (key bound at construction, as `azuredol.BlobHandle` does) and everything else — +`handle`, `sub`, `prefixes`, `url_for`, `info`, `delete_many` — becomes a free function taking +the store first. One primitive, `_abs_key(store, k) = inner_most_key(store, k)`, which +**replaces** `_id_of_key` and must never compose with it, plus a mandatory `str` check. +Conformance test with an empty allowlist. Two findings drive it: **`azuredol` is robust because +it has almost no keyed methods, not because its prefix lives in the leaf** (§D1), and **the +`wrapped_self` escape is silently wrong on temporary wrappers, undetectably** (§D1a) — which is +why free functions are the only form rather than merely the safe one. ## 4. Verified findings — what is actually wrong with v0.1.9 @@ -193,15 +205,20 @@ own rule. Moved to the deferral list. **(g) An absolute `import s3dol < 30 ms` budget.** Arithmetically impossible: `dol` alone is ~61 ms and is a hard dependency of Layer B. Replaced by a delta budget. -## 6. The two open design questions +## 6. The design questions -These are the ones worth discussing before any code is written. Each has a GitHub discussion -with concrete options; this section is the short framing. +- **§7 — Layered transformation and unmapped keys.** Discussion **#14** — **RESOLVED**, see + [ADR-0011](decisions/0011-keyed-capability-surface.md). §7 below is kept as the problem + statement; the answer is in the ADR and summarised at the end of the section. +- **§8 — Credential and endpoint resolution.** Discussion **#15** — **still open**. This is the + one to pick up next. -- **§7 — Layered transformation and unmapped keys.** Discussion: **#14** -- **§8 — Credential and endpoint resolution.** Discussion: **#15** +## 7. ~~Open question 1~~ RESOLVED — layered transformation and the unmapped key -## 7. Open question 1 — layered transformation and the unmapped key +> **Resolved 2026-08-10 in [ADR-0011](decisions/0011-keyed-capability-surface.md)** (discussion +> #14). The framing below stands, with two corrections found while resolving it — see +> §7-resolution at the end. Do not re-litigate the options without reading the ADR: three of the +> five were eliminated by running code, not by argument. ### The problem, precisely @@ -300,7 +317,59 @@ The pushdown half (failure mode 2) is separate and probably needs its own answer protocol (`__iter__(self, *, prefix_hint=…)`) or an explicit `iter_prefix` on the leaf that the wrapper knows to route to. -## 8. Open question 2 — credential and endpoint resolution +### §7-resolution — what was decided, and the two corrections + +Full record: [ADR-0011](decisions/0011-keyed-capability-surface.md). Short form: + +**Decided.** Layer B gets **zero** key-taking public methods. Per-object capabilities move onto +`ObjectHandle` (key bound at construction, as `azuredol.BlobHandle` does); `handle`, `sub`, +`prefixes`, `url_for`, `info` and `delete_many` become **free functions taking the store first** +(Option C). One primitive, `_abs_key(store, k) = inner_most_key(store, k)`, with a mandatory +`str` check. A reflective conformance test with an *empty* allowlist instead of Option B's +registry. Option E deferred; Option D rejected; pushdown closed. + +**Correction 0 — the biggest one, and it was found by testing the plan rather than arguing it.** +The first draft kept `handle(k)`/`sub(prefix)` as methods hardened with +`inner_most_key(wrapped_self(self), k)` — the form ADR-0006 §2 prescribed. That form is +**silently wrong whenever the wrapper is a temporary**: + +```python +s3_store('bucket', prefix='logs/') # named -> correct +KeyCodecs.prefixed('x/')(s3_store(...)).handle(k) # temporary -> WRONG, silently +``` + +A delegated bound method holds no reference to the wrapper, so it is collected before the body +runs; `wrapped_self`'s weakref cleanup then *removes the registry entry*, making it +indistinguishable from "never wrapped". And because the prefix lives in the leaf, the wrong +answer is a plausible `str`, so the type check does not catch it. Measured: free-function form +6/6 correct across wrap × lifetime shapes, method form 2/4. That is what forces "zero methods" +rather than "two". It also demotes `wrapped_self` from *the* escape to a best-effort guardrail, +and adds a new upstream item against dol#18 (ADR-0011 §D9). + +**Correction 1 — the provisional lean above was wrong about Option E.** It claims E works +because key transformation "happens through the Mapping protocol the wrapper already handles +correctly". Verified false at dol 0.3.58: a wrapper does **not** re-wrap a Mapping-valued +attribute — `store.urls` returns an inner-keyed mapping under both class- and instance-wrap. E +needs the same `inner_most_key` call as A and C, so it is a *surface* over one mechanism, not a +third mechanism. That removes its main claimed advantage. + +**Correction 2 — Option C's cited prior art has the bug.** `dol.content_url` resolves with a +flat `getattr(store, 'url_for')(key)` (`dol/content.py:210-214`), so through a key wrap it +returns a URL for the unmapped key. The idiom is prior art; the resolution is not. Fixing it is +now a blocking upstream item (§9 of ADR-0006). + +**Also, two things the framing above got structurally incomplete:** + +- There are **two** delegation routes, not one — `Store.__getattr__` (`dol/base.py:742`) for + instance-wraps and `mk_relative_path_store` subclasses, and `DelegatedAttribute.__get__` + (`dol/base.py:279`) for class-wraps. A fix covering one is a silent no-op on the other. +- **Option D was already rejected upstream** in `dol/misc/docs/dol_issue18_design.md`, with + better reasons than blast radius: rebinding binds to the *innermost* `Wrap`, so under a `Pipe` + stack it does not fix the case it exists to fix. The terminal fix is dol's **is-a wrapping** + (dol#18 Approach C, committed for 0.4/1.0) — which #14's option list did not contain, and + which is why the selection criterion became *correct on 0.3.x AND redundant under is-a*. + +## 8. Open question — credential and endpoint resolution ### The problem @@ -460,6 +529,76 @@ run(True) # 2 of 7 — ['a%0Db','a%20b','a%2520b','a%2Bb','caf%C3%A9','p/x%20y' # moto head_object missing bucket -> Code='NoSuchBucket'; real AWS -> Code='404' ``` +```python +# The temporary-wrapper hole in `wrapped_self` (ADR-0011 §D1a) — the finding that forced +# "zero keyed methods". Needs only dol. +from dol import KeyCodecs, wrapped_self +from dol.base import KvReader +from dol.dig import inner_most_key # NOTE: not exported from `dol` + + +class BR(KvReader): # an s3dol-shaped leaf: it OWNS its prefix, so it has _id_of_key + def __init__(self, d, prefix=""): + self._d = d + self.prefix = f"{prefix.strip('/')}/" if prefix else "" + + def _id_of_key(self, k): + return f"{self.prefix}{k}" + + def _key_of_id(self, i): + return i[len(self.prefix) :] if self.prefix else i + + def __iter__(self): + yield from (self._key_of_id(i) for i in self._d if i.startswith(self.prefix)) + + def __getitem__(self, k): + return self._d[self._id_of_key(k)] + + def m_abs(self, k): # METHOD form — ADR-0006 §2's original prescription + return inner_most_key(wrapped_self(self), k) + + +def f_abs(store, k): # FREE-FUNCTION form — ADR-0011 §D2 + return inner_most_key(store, k) + + +DATA = {"logs/x/b.txt": 2} +mk = lambda: KeyCodecs.prefixed("x/")(BR(DATA, "logs/")) # want 'logs/x/b.txt' + +s = mk() +s.m_abs("b.txt") # 'logs/x/b.txt' correct — wrapper is NAMED, so alive +mk().m_abs("b.txt") # 'logs/b.txt' WRONG — wrapper was a TEMPORARY +# ^ a plausible str, so an isinstance check cannot catch it; +# and `id(leaf) in dol.base._wrapper_backrefs` is False in BOTH the +# collected-temporary and never-wrapped cases, so it is undetectable. + +f_abs(s, "b.txt") # 'logs/x/b.txt' correct +f_abs(mk(), "b.txt") # 'logs/x/b.txt' correct — store is an ARGUMENT, so it stays alive +``` + +```python +# dol.content_url resolves with the OUTER key (ADR-0006 §3 item 6) +from dol import KeyCodecs, content_url + + +class Leaf(dict): + def url_for(self, k): + return f"s3://B/{k}" + + +content_url(KeyCodecs.prefixed("a/")(Leaf)({"a/b": b"v"}), "b") +# -> 's3://B/b' the bytes are at 'a/b' + +# dol.filesys.Files.is_valid_key — confirmed live +import os, tempfile +from dol import Files + +d = tempfile.mkdtemp() +open(os.path.join(d, "a.txt"), "w").write("x") +list(Files(d)) # ['a.txt'] +Files(d).is_valid_key("a.txt") # False <- for a key that exists +``` + `azuredol` is the reference implementation for the layering — read its `base.py` (not its `architecture.md`): normalized prefix in the leaf, `_id_of_key`/`_key_of_id` in the leaf, pushdown via `list_blobs(name_starts_with=…)`, prefix in `__repr__`, sub-stores via @@ -482,10 +621,15 @@ before it can serve as the cross-repo gate ADR-0008 proposes. ## 11. Suggested next steps -1. **Resolve §7 and §8** (discussions #14, #15) — both shape the module boundaries, so they - are worth settling before code. +1. ~~Resolve §7~~ **done** — [ADR-0011](decisions/0011-keyed-capability-surface.md). + **Resolve §8** (discussion #15) — it still shapes the module boundaries, so settle it + before code. 2. **#10 (SigV2)** as a standalone 0.1.x patch. Independent, strictly a fix, immediate value. 3. **`s3dol.diagnose()`** in the same 0.1.x line, so dependents can record what their environment resolves to *before* the resolution order changes. -4. **dol#82 / dol#83** — both block the implementation and benefit every `*dol` adapter. -5. **P0 tier-1 test scaffolding**, then module-by-module per #11. +4. **dol upstreams** — dol#82 / dol#83 benefit every `*dol` adapter; the two ADR-0011 + dependencies (`inner_most_key` export + hardening, `content_url` chain resolution) block the + implementation directly. See [ADR-0006](decisions/0006-key-scoping-and-dol-fixes.md) §3. +5. **P0 tier-1 test scaffolding**, then module-by-module per #11. The reflective + keyed-surface conformance test (ADR-0011 §D5) belongs in P0 — it is cheap and it is what keeps + the §7 decision from decaying. From 88f8cff0db6af8bf4ba9d69c746b3e9ac4d008f4 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:13:07 +0000 Subject: [PATCH 2/2] docs: settle #14 on capability stores (ADR-0011 Accepted) Replaces the WIP draft after an adversarial review refuted three of its claims and surfaced a better option. DECISION. Layer B carries no key-taking public methods. A keyed capability becomes a SIBLING STORE over the same key space -- s3dol.handles(store)[k] / .urls / .info -- keyed through __getitem__, the one thing dol maps correctly at every wrapper depth. Non-keyed operations (sub, prefixes, delete_many, delete_bucket) are free functions. url_for survives as a single guarded method purely to satisfy dol.SupportsUrlFor. This is correct BY CONSTRUCTION: sibling stores call no key-resolution primitive, so they are immune to both holes found below. Verified correct unreferenced, under Pipe, under cached_keys, under a value codec, and under a non-Store passthrough layer. WHY NOT THE ALTERNATIVES (all refuted by running code, not argument): - Hardened METHODS: inner_most_key(wrapped_self(self), k) is silently wrong when nothing references the wrapper -- and because the prefix lives in the leaf, the wrong answer is a plausible str, so a type check cannot catch it. Reproduced on CPython 3.10-3.14. - FREE FUNCTIONS everywhere: not categorically safe either. With a hand-rolled non-Store layer in the chain they return a plausible wrong key in three configurations, two of which the method form gets right. - Option E as an ATTRIBUTE: verified that a wrapper does not re-wrap a Mapping-valued attribute. Adopted as a STORE instead, which is what makes it work. The #14 lean (C+E with B) was right in substance; E's value comes from being a store, not an attribute. CORRECTIONS to the earlier draft, kept visible in the ADR: - "undetectable" was false; a guard is writable, so methods were rejected on cost/benefit, not impossibility - s3_store(...).handle(k) is NOT an instance of the bug; s3_store returns a bare leaf or a value-codec wrap, both correct. Only a user-applied KEY codec triggers it - the census is overwhelmingly LATENT, not shipping-destructive, and 12 of the survey claims were refuted outright Also: EndpointStore.delete becomes s3dol.delete_bucket (last destructive delegated method removed); ADR-0011 added to the docs index; the superseded surface-growth rule marked in 0005 and 0009; 0001/0006/0007/0010 amended. --- misc/docs/README.md | 13 +- misc/docs/architecture.md | 88 ++- .../decisions/0001-layered-architecture.md | 18 +- misc/docs/decisions/0005-large-object-io.md | 8 + .../decisions/0009-scope-and-deferrals.md | 16 +- .../0010-bucket-and-bulk-operations.md | 9 +- .../0011-keyed-capability-surface.md | 554 ++++++++---------- misc/docs/state-of-play.md | 81 ++- 8 files changed, 406 insertions(+), 381 deletions(-) diff --git a/misc/docs/README.md b/misc/docs/README.md index 605f30d..24627f5 100644 --- a/misc/docs/README.md +++ b/misc/docs/README.md @@ -22,6 +22,7 @@ output; don't put prose here that a build step will overwrite.) | [0008](decisions/0008-testing-architecture.md) | Four tiers, shipped fake, exported conformance | writing a test | | [0009](decisions/0009-scope-and-deferrals.md) | v1 scope, deferrals, the `s3dol`/`botodol` line | proposing a feature | | [0010](decisions/0010-bucket-and-bulk-operations.md) | Bucket-existence policy, `delete_many`, cascading delete | touching bucket lifecycle or bulk ops | +| [0011](decisions/0011-keyed-capability-surface.md) | **No key-taking methods; capabilities are sibling stores** | adding any capability, or wondering where `url_for` went | ## The five things most likely to bite you @@ -37,11 +38,13 @@ output; don't put prose here that a build step will overwrite.) `dol` codec anyway, only `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` is safe — and only after normalizing `p` to end in the delimiter. [ADR-0006 §1](decisions/0006-key-scoping-and-dol-fixes.md). -3. **A `dol` wrapper delegates methods with the outer, unmapped key**, so `url_for`, `sub`, - `handle`, `info`, `prefixes` and `delete_many` silently address the wrong object — - `delete_many` *destroys* it. `isinstance(store, SupportsUrlFor)` still says `True`. Use - `inner_most_key(wrapped_self(self), k)`; `inner_most_key(self, k)` returns `None`. - [ADR-0001 §Why the prefix lives in the leaf](decisions/0001-layered-architecture.md). +3. **A `dol` wrapper delegates methods with the outer, unmapped key**, so a keyed method like + `url_for` silently addresses the wrong object. Capability detection can't see it, and + **the obvious escape is also broken**: `inner_most_key(wrapped_self(self), k)` is silently + wrong when nothing holds a reference to the wrapper (`inner_most_key(self, k)` returns + `None`, which at least fails loudly). So s3dol has **no key-taking methods**: capabilities + are sibling stores keyed through `__getitem__` — the one thing `dol` maps correctly at every + depth. [ADR-0011](decisions/0011-keyed-capability-surface.md). 4. **botocore ≥1.36 sends checksums by default**, and several S3-compatible providers either reject them loudly or persist the `aws-chunked` framing *into the object body*. The fix is client config plus `s3transfer>=0.11.2`. diff --git a/misc/docs/architecture.md b/misc/docs/architecture.md index ad16798..d84338c 100644 --- a/misc/docs/architecture.md +++ b/misc/docs/architecture.md @@ -117,29 +117,36 @@ BucketCollection (Collection — __iter__ over object keys) └── BucketReader (+ __getitem__ -> bytes) └── BucketStore (+ __setitem__ / __delitem__) +BucketHandles (KvReader — __getitem__ -> ObjectHandle) ┐ sibling capability +BucketUrls (KvReader — __getitem__ -> presigned URL) │ stores over the SAME +BucketInfo (KvReader — __getitem__ -> ObjectInfo) ┘ key space + EndpointCollection (Collection — __iter__ over bucket names) └── EndpointReader (+ __getitem__ -> BucketReader) └── EndpointStore (+ __setitem__ / __delitem__ for buckets) ``` +The four bucket-level readers share one private base holding `prefix`, connection and +`_id_of_key`/`_key_of_id`, so the key arithmetic exists exactly once. + plus `ObjectHandle` — the escape hatch for one object, which is **not** a Mapping and is where ranged reads, streaming, multipart, presigned URLs and object metadata live. `ObjectHandle` binds its key **at construction**, which is what makes it immune to the delegation trap ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D1, following `azuredol`'s `BlobHandle`). -> **Layer B has no key-taking public methods at all.** The Mapping dunders are the whole keyed -> surface, because `dol` maps *those* correctly at any wrapper depth. Every capability — -> `handle`, `sub`, `prefixes`, `url_for`, `info`, `delete_many` — is a **free function taking -> the store first**. +> **Layer B has no key-taking public methods** (one guarded exception, `url_for` — below). The +> Mapping dunders are the whole keyed surface, because `dol` maps *those* correctly at any +> wrapper depth. A keyed capability is a **sibling store** you index; anything non-keyed is a +> **free function taking the store first**. > > This is not stylistic. A `dol` key wrapper hands any non-dunder method the *outer, unmapped* -> key; and the obvious hardening (`inner_most_key(wrapped_self(self), k)`) is **silently wrong -> when the store is a temporary**, as in `s3_store(...).handle(k)` — the wrapper is collected -> before the method body runs, and the failure is undetectable. Free functions receive the store -> as an argument, so it stays alive and the resolution is reliable: measured 6/6 correct versus -> 2/4 for the method form. See [ADR-0011](decisions/0011-keyed-capability-surface.md) §D1a. A -> reflective conformance test enforces an **empty** allowlist ([ADR-0011] §D5). +> key — and the obvious hardening, `inner_most_key(wrapped_self(self), k)`, is **itself silently +> wrong** when nothing holds a reference to the wrapper, returning a plausible but incorrect key. +> Sibling stores route through `__getitem__` instead, so they are correct *by construction* with +> no key-resolution primitive at all. See +> [ADR-0011](decisions/0011-keyed-capability-surface.md) §D1a/§D2. A reflective conformance test +> enforces the rule (§D5). `EndpointStore`, not `BucketsStore`: naming the *containing* resource (as `azuredol` does with `ContainerStore`/`AccountStore`) avoids shipping `BucketStore` and `BucketsStore` — two of the @@ -156,16 +163,31 @@ yields a working, silently-wrong store. | `__len__()` | **Not implemented.** Raises `TypeError` with guidance. See [0008](decisions/0008-testing-architecture.md) §cost model. | | `__repr__` | bucket, prefix, endpoint host, mode. No secrets, no addresses. | -The capabilities, as **free functions** ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D3): +**Keyed capabilities — sibling stores you index** ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D2): + +| Store | `__getitem__(k)` returns | +|---|---| +| `BucketHandles` — `s3dol.handles(store)` | An `ObjectHandle` bound to `k`. Zero round-trips. | +| `BucketUrls` — `s3dol.urls(store)` | A presigned URL. Zero object requests. | +| `BucketInfo` — `s3dol.info(store)` | One `HeadObject` → `ObjectInfo`. | + +The `s3dol.(store)` accessors derive a sibling from an **unwrapped** store and **raise** on +a wrapped one, naming the remedy: wrap the sibling in parallel with the same codec. s3dol will +not guess a user's codec chain (that is dol#10). + +**Non-keyed operations — free functions** (§D3): | Function | Contract | |---|---| -| `s3dol.handle(store, k)` | An `ObjectHandle` bound to `k`. Zero round-trips. The per-object entry point. | -| `s3dol.sub(store, prefix)` | A store with an extended, normalized prefix, **in the caller's key space**. Zero round-trips on an unwrapped store, where it pushes down; on a wrapped store it composes `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` over the outer store and loses pushdown (§D2, §D8). | -| `s3dol.prefixes(store)` | One `ListObjectsV2(Delimiter='/')` → `CommonPrefixes`, mapped back out to the caller's key space (§D2). | -| `s3dol.url_for(store, k)` | `handle(store, k).url()`. | -| `s3dol.info(store, k)` | `handle(store, k).info()`. | -| `s3dol.delete_many(store, keys)` | See [0010](decisions/0010-bucket-and-bulk-operations.md). | +| `s3dol.sub(store, prefix)` | A store with an extended, normalized prefix, **in the caller's key space**. Zero round-trips on an unwrapped store, where it pushes down; on a wrapped store it composes `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` over the outer store, loses pushdown (§D8), and returns a different type. | +| `s3dol.prefixes(store)` | One `ListObjectsV2(Delimiter='/')` → `CommonPrefixes`, mapped back out to the caller's key space. | +| `s3dol.delete_many(store, keys)` | See [0010](decisions/0010-bucket-and-bulk-operations.md) §2. | +| `s3dol.delete_bucket(endpoint, name, force=True)` | The explicit cascading form. See [0010](decisions/0010-bucket-and-bulk-operations.md) §3. | + +**The one keyed method**, `BucketReader.url_for(k)`: exists only because `dol.SupportsUrlFor` +requires a *method* and `dol.content_url` reaches it by `getattr`. Guarded so it is correct when +unwrapped or when the wrapper is referenced, and **raises** otherwise — never silently wrong. +`s3dol.urls(store)[k]` is the canonical form (§D3b). On `ObjectHandle` (key bound at construction, so no delegation seam — these are methods): @@ -175,14 +197,13 @@ On `ObjectHandle` (key bound at construction, so no delegation seam — these ar | `info()` | One `HeadObject` → size, mtime, etag, content-type, storage class, restore status. | **The rule for what may join the Layer B table** (it is otherwise how a surface grows to -twenty): **nothing that takes a key.** A method may be added iff it takes no key at all. -Anything addressing one object belongs on `ObjectHandle`; anything else belongs in a free -function or a recipe. +twenty): **nothing that takes a key.** Anything addressing one object becomes a sibling +capability store or lives on `ObjectHandle`; anything else is a free function or a recipe. The reason is mechanical, not aesthetic: `dol` hands any non-dunder method the outer, unmapped -key when the store is key-wrapped, and the standard workaround is itself silently wrong on -temporaries. `azuredol` — the reference implementation — has ~zero keyed methods for the same -reason. See [ADR-0011](decisions/0011-keyed-capability-surface.md). +key when the store is key-wrapped, and the standard workaround is itself silently wrong when the +wrapper is unreferenced. `azuredol` — the reference implementation — has ~zero keyed methods for +the same reason. See [ADR-0011](decisions/0011-keyed-capability-surface.md). ### Layer C — `s3dol.recipes` @@ -225,10 +246,11 @@ s3dol/ values.py Filepath / Chunks / Streamable refs; as_fileobj singledispatch writes.py write strategies (simple / transfer / multipart) reads.py read strategies (bytes / stream / ranged / to-file) - base.py Layer B (owns prefix). NO key-taking public methods — ADR-0011 §D1 - capabilities.py the keyed API as free functions: handle / sub / prefixes / - url_for / info / delete_many, plus _abs_key, the one - key-resolution primitive (ADR-0011 §D2/§D3) + base.py Layer B (owns prefix). NO key-taking public methods except the + guarded url_for shim — ADR-0011 §D1/§D3b + capabilities.py sibling stores BucketHandles/BucketUrls/BucketInfo + their + accessors; free functions sub / prefixes / delete_many / + delete_bucket (ADR-0011 §D2/§D3) recipes.py Layer C — s3_store(...), codec facades diagnose.py s3dol.diagnose() — prints resolved endpoint/region/credential SOURCE store.py COMPAT SHIM — legacy S3Store, DeprecationWarning, removed in v2 @@ -240,10 +262,11 @@ s3dol/ `diagnose.py` is small but not optional: it is the step-0 safety mechanism for a change that can silently move a live data target ([ADR-0007](decisions/0007-naming-and-compatibility.md) §5). -**Public API** — `s3_store`, `BucketStore`, `BucketReader`, `EndpointStore`, `ObjectHandle`, -`S3Connection`, `Filepath`/`Chunks`/`Streamable`, the error classes, `diagnose`, and the keyed -free functions `handle` / `sub` / `prefixes` / `url_for` / `info` / `delete_many`. Everything -else is implementation. +**Public API** — `s3_store`, `BucketStore`, `BucketReader`, `BucketHandles`, `BucketUrls`, +`BucketInfo`, `EndpointStore`, `ObjectHandle`, `S3Connection`, +`Filepath`/`Chunks`/`Streamable`, the error classes, `diagnose`, the capability accessors +`handles` / `urls` / `info`, and the free functions `sub` / `prefixes` / `delete_many` / +`delete_bucket`. Everything else is implementation. `store.py` must survive as an importable module: both external dependents do `from s3dol.store import S3Store`, not `from s3dol import S3Store`. @@ -262,7 +285,8 @@ else is implementation. *parameter*; only `on_missing_bucket='raise'` performs I/O, and it says so ([0010](decisions/0010-bucket-and-bulk-operations.md)). - **Cascading deletes as a side effect.** `del endpoint[name]` refuses a non-empty bucket; - `endpoint.delete(name, force=True)` is the explicit form. + `s3dol.delete_bucket(endpoint, name, force=True)` is the explicit form — a free function, not + a method ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D4). - **`__len__` on a bucket store.** Unbounded pagination cost. - **Silent empties.** Anywhere. - **Passing `EncodingType` to a list call.** botocore sets it itself *and* decodes the diff --git a/misc/docs/decisions/0001-layered-architecture.md b/misc/docs/decisions/0001-layered-architecture.md index dc35460..517431b 100644 --- a/misc/docs/decisions/0001-layered-architecture.md +++ b/misc/docs/decisions/0001-layered-architecture.md @@ -115,14 +115,16 @@ over the store cannot corrupt it. `azuredol`'s only residual exposures are So `azuredol` is not safe because of where its prefix lives. It is safe because **it has almost no seam to get wrong.** The table above lists six methods this ADR originally proposed for -Layer B; [ADR-0011](0011-keyed-capability-surface.md) reduces that to **zero**, moving per-object -capabilities onto `ObjectHandle` (key bound at construction) and the rest into free functions. - -Two is not good enough, and the reason is worth knowing before you propose keeping one: the -obvious hardening — `inner_most_key(wrapped_self(self), k)` — is **silently wrong when the -wrapper is a temporary** (`s3_store(...).handle(k)`), because the delegated bound method holds -no reference to the wrapper and the weakref registry entry is removed when it dies. Verified; -see ADR-0011 §D1a. +Layer B; [ADR-0011](0011-keyed-capability-surface.md) reduces that to **zero** (plus one guarded +`url_for` shim for `dol.SupportsUrlFor`), turning keyed capabilities into **sibling stores** you +index — `s3dol.handles(store)[k]` — and the rest into free functions. + +Before proposing that one keyed method be kept: the obvious hardening, +`inner_most_key(wrapped_self(self), k)`, is **itself silently wrong** when nothing holds a +reference to the wrapper, because the delegated bound method holds none and the weakref registry +entry is removed when the wrapper dies. Sibling stores avoid the question entirely by routing +through `__getitem__`, which `dol` maps correctly at every depth. Verified; see ADR-0011 +§D1a/§D2. The "Consequences" claim below that *"every capability method is key-correct by construction"* holds only for an unwrapped store. Read it as: *there is no seam **we** introduce* — a user can diff --git a/misc/docs/decisions/0005-large-object-io.md b/misc/docs/decisions/0005-large-object-io.md index 7c3241e..c398ada 100644 --- a/misc/docs/decisions/0005-large-object-io.md +++ b/misc/docs/decisions/0005-large-object-io.md @@ -27,6 +27,14 @@ pure" was being claimed while six methods were added: > admitted only because the cost difference against a `__delitem__` loop is an order of > magnitude. +> **Superseded by [ADR-0011](0011-keyed-capability-surface.md).** The rule above is now +> stricter and simpler: **a method may be added iff it takes no key at all.** A keyed method is +> handed the *outer, unmapped* key by any `dol` key wrapper, and the standard escape is itself +> silently wrong when nothing references the wrapper — so `info`, `url_for`, `sub`, `handle`, +> `prefixes` and `delete_many` are *not* admissible as methods. Capabilities become sibling +> stores keyed through `__getitem__` (`s3dol.handles(store)[k]`) or free functions. `url_for` +> survives as a single guarded exception, purely to satisfy `dol.SupportsUrlFor`. + For calibration: `azuredol` has exactly one such method (`walk`) and pushes everything per-object onto `BlobHandle`. Six unexplained exceptions is how a surface grows to twenty. diff --git a/misc/docs/decisions/0009-scope-and-deferrals.md b/misc/docs/decisions/0009-scope-and-deferrals.md index 06474a6..ec41af8 100644 --- a/misc/docs/decisions/0009-scope-and-deferrals.md +++ b/misc/docs/decisions/0009-scope-and-deferrals.md @@ -46,11 +46,19 @@ Plus, from the "beyond blobs" analysis, the items that are **free or nearly so** fall out of work already being done: - **`ObjectInfo` from LIST metadata.** Every `ListObjectsV2` response already carries size, - mtime, ETag and storage class, and v0 throws them away. A `store.info(k)` (one HEAD) and a - cheap listing-derived metadata view cost almost nothing and serve nearly every use case. + mtime, ETag and storage class, and v0 throws them away. One HEAD, plus a cheap + listing-derived metadata view, costs almost nothing and serves nearly every use case. - **Prefix tree via `CommonPrefixes`.** `Resp.common_prefixes` has existed in `utility.py` - since 2023 and is **called from nowhere**. `store.prefixes()` is one LIST with a delimiter. -- **A presigned-URL view.** `url_for` already exists; a `Mapping` face over it is trivial. + since 2023 and is **called from nowhere**. One LIST with a delimiter. +- **A presigned-URL view.** The presigning logic already exists; a `Mapping` face over it is + trivial. + +> **Shapes updated by [ADR-0011](0011-keyed-capability-surface.md).** These three are no longer +> `store.info(k)` / `store.prefixes()` / a view *attribute*. They are **sibling stores** — +> `s3dol.info(store)[k]`, `s3dol.urls(store)[k]` — plus the free function +> `s3dol.prefixes(store)`. That is not extra work: ADR-0011 §D2 makes all three instances of one +> mechanism, which is why they move from "nearly free" to actually shared. The ADR-0011 note +> that they were "deferred" is about the *attribute* form only. The multipart **parts** store (`MutableMapping[int, bytes]`, filled then committed) is built as an *internal mechanism* of the write strategy in v1 — it is how `s[k] = v` stays pure — diff --git a/misc/docs/decisions/0010-bucket-and-bulk-operations.md b/misc/docs/decisions/0010-bucket-and-bulk-operations.md index 65be30a..5a71e70 100644 --- a/misc/docs/decisions/0010-bucket-and-bulk-operations.md +++ b/misc/docs/decisions/0010-bucket-and-bulk-operations.md @@ -83,8 +83,15 @@ distinguishes absent ones. ### 3. Cascading bucket deletion stays explicit +> **Amended by [ADR-0011](0011-keyed-capability-surface.md) §D4: the cascading form is +> `s3dol.delete_bucket(endpoint, name, force=True)`, a free function — not +> `endpoint.delete(...)`.** A public, key-taking, destructive, *delegated* method is +> structurally identical to `azuredol.AccountStore.delete`, which ADR-0011 cites as a census +> exhibit. `del endpoint[name]` is unaffected: it is a Mapping dunder, so `dol` maps it +> correctly. + `del endpoint[name]` raises `BucketNotEmpty` if the bucket has objects. -`endpoint.delete(name, force=True)` is the documented cascading form, and it **paginates** — +The cascading form **paginates** — v0's version listed one page and deleted at most 1000 objects before failing on `delete_bucket`, i.e. a partial, non-idempotent destruction. diff --git a/misc/docs/decisions/0011-keyed-capability-surface.md b/misc/docs/decisions/0011-keyed-capability-surface.md index 348d0e5..56f90c9 100644 --- a/misc/docs/decisions/0011-keyed-capability-surface.md +++ b/misc/docs/decisions/0011-keyed-capability-surface.md @@ -1,11 +1,14 @@ # ADR-0011: The keyed capability surface and the unmapped-key problem -- **Status:** **Proposed** — D1/D2/D3 are under revision; see §"Open fork" below. D4–D9 are settled. +- **Status:** Accepted - **Date:** 2026-08-10 - **Discussion:** [#14](https://github.com/i2mint/s3dol/discussions/14) - **Amends:** [ADR-0001](0001-layered-architecture.md) (Layer B method table), + [ADR-0005](0005-large-object-io.md) §2 (the surface-growth rule), [ADR-0006](0006-key-scoping-and-dol-fixes.md) §2 (the escape form), - [ADR-0010](0010-bucket-and-bulk-operations.md) §2 (`delete_many`'s surface) + [ADR-0007](0007-naming-and-compatibility.md) (sub-store migration row), + [ADR-0009](0009-scope-and-deferrals.md) (v1.0 scope: `info`/`prefixes` shapes), + [ADR-0010](0010-bucket-and-bulk-operations.md) §2–§3 (`delete_many`, cascading delete) - **Upstream:** [dol#83](https://github.com/i2mint/dol/issues/83) (this problem), [dol#18](https://github.com/i2mint/dol/issues/18) (its root) @@ -13,7 +16,7 @@ [ADR-0001](0001-layered-architecture.md) puts the prefix in the leaf, which removes the key-mapping seam between a capability method and the wire **for the prefix s3dol owns**. It does -not remove the seam in general: a user who wraps an s3dol store with any `dol` key codec still +not remove the seam in general: a user who wraps an s3dol store with a `dol` **key** codec still gets a silently wrong `url_for`. Discussion #14 asked how to fix that properly. ### The mechanism, precisely @@ -27,31 +30,37 @@ is handed the **outer, unmapped** key. There are **two** delegation routes, not | `Store.__getattr__` | instance-wraps, and `mk_relative_path_store` subclasses | `dol/base.py:742` | | `DelegatedAttribute.__get__` | class-wraps (`delegate_to` installs one descriptor per attr of `dir(wrapped)`) | `dol/base.py:279`, installed at `dol/base.py:416-480` | -Both return the method **bound to the leaf**. #14's original framing named only the first; any -fix that covers one and not the other is a silent no-op on half the cases. +Both return the method **bound to the leaf**. #14's framing named only the first; a fix covering +one and not the other is a silent no-op on half the cases. -Nothing raises, and capability detection cannot see it: a `@runtime_checkable` Protocol checks -method *presence* only. It is also wrap-dependent — since 3.12 `isinstance` uses -`getattr_static`, so `isinstance(w, SupportsUrlFor)` is `True` for a **class**-wrapped -capability but `False` for an **instance**-wrapped one. Either way it tells you nothing about -whether the key is right. +Capability detection cannot see it: a `@runtime_checkable` Protocol checks method *presence* +only. It is also wrap-dependent — since 3.12 `isinstance` uses `getattr_static`, so +`isinstance(w, SupportsUrlFor)` is `True` for a **class**-wrapped capability and `False` for an +**instance**-wrapped one. Either way it says nothing about whether the key is right. ### What the family census showed -Surveyed across ~20 `*dol` packages, this is not a latent tidiness issue. Already shipping: -destructive key-taking delegated methods in `cosmodol` (`CosmosItems.replace`/`batch`, -`CosmosDatabase.delete`, `CosmosAccount.delete`), `azuredol` (`AccountStore.delete`), -`pydrivedol` (`GDStore.upload`, and `GDReader.get_url` which *also* grants anyone/reader -permission on the wrong file), `aiofiledol`, and `sshdol.sync_to` (rsync `--delete` over the -leaf's whole rootdir). And a **confirmed-live** bug in `dol` itself: -`dol.filesys.Files(d).is_valid_key(k)` returns `False` for a key that exists. +13 sibling packages were surveyed and every claim re-verified by source read plus a runnable +repro. **Stated accurately, because an earlier draft of this ADR overstated it:** the defect is +overwhelmingly **latent** — it bites only when a user applies a key codec, and most of these +packages never wrap their own stores. Verified breakdown: `focal` confirmed-live throughout; +`azuredol`, `aiofiledol`, `chromadol`, `sqldol`, `mongodol`, `redisdol` mixed; `cosmodol`, +`pydrivedol`, `sshdol`, `dynamodol`, `hfdol` latent-only; `couchdol` essentially refuted. **12 +survey claims were refuted outright.** -The full census, with per-symbol verdicts, lives on -[dol#83](https://github.com/i2mint/dol/issues/83). +Latent is not harmless — the latent cases include `cosmodol.CosmosItems.replace`, which under a +key codec silently overwrites a *different, real* document in full, and +`pydrivedol.GDReader.get_url`, which returns a URL for the wrong file *and grants +anyone/reader permission on it*. But "already destroying data in production" is not what the +evidence supports, and this ADR does not claim it. Per-package detail and repros go to the +responsible repos, indexed from [dol#83](https://github.com/i2mint/dol/issues/83). + +One case *is* confirmed-live in `dol` itself: `dol.filesys.Files(d).is_valid_key(k)` returns +`False` for a key that exists. ## Decision -### D1 — Shrink the keyed surface. This is the primary decision. +### D1 — Layer B carries no key-taking public methods (one documented exception) [ADR-0001](0001-layered-architecture.md)'s Layer B table proposed **six** keyed methods (`url_for`, `info`, `handle`, `sub`, `prefixes`, `delete_many`). `azuredol` — the reference @@ -65,354 +74,299 @@ residual exposures are `ContainerCollection.walk` (`base.py:164`) and `AccountSt no seam to get wrong.** Prefix-in-leaf is necessary, not sufficient. That is the finding, and ADR-0001 previously drew the wrong lesson from the same source. -So: - -- `url_for` and `info` **move onto `ObjectHandle`** — `ObjectHandle` binds its key at - construction, azuredol-style, so no key ever crosses a delegation seam to reach them. -- `delete_many` leaves the store entirely — see D4. -- **`handle` and `sub` become free functions too**, and Layer B ends up with **zero** key-taking - public methods. That is stronger than this ADR originally proposed, and it is forced by the - finding below rather than chosen for symmetry. +The single exception is **`url_for(k)`, kept solely to satisfy `dol.SupportsUrlFor`** — see D3b. +It is the only entry on D5's allowlist and it is guarded so it is correct-or-loud, never +silently wrong. -### D1a — Why zero and not two: a delegated method cannot be made reliably key-correct +### D1a — Why a hardened *method* is not the answer -The first draft of this ADR kept `handle(k)` and `sub(prefix)` as methods, hardened by D2's -`inner_most_key(wrapped_self(self), k)`. **Testing that plan refuted it.** +The first draft kept `handle(k)`/`sub(prefix)` as methods hardened with +`inner_most_key(wrapped_self(self), k)` — the form ADR-0006 §2 prescribed. Testing that plan +refuted it. -`wrapped_self` resolves the outer store through a weakref registry keyed by `id(inner)` -(`dol/base.py`, `_wrapper_backrefs`). A `DelegatedAttribute` returns a method bound to the -**leaf**, holding no reference to the wrapper. So in a chained expression the wrapper is a -temporary that is freed by the time the method body runs, its weakref dies, and the registry -entry is **removed by the cleanup callback** — leaving a state indistinguishable from "never -wrapped": +`wrapped_self` resolves the outer store through a weakref registry keyed by `id(inner)`. A +`DelegatedAttribute` returns a method bound to the **leaf**, holding no reference to the +wrapper. So when nothing else holds the wrapper, it is freed before the method body runs and the +registry entry is removed by the cleanup callback: ```python s = KeyCodecs.prefixed('x/')(BucketReader(data, prefix='logs/')) -s.m_abs('b.txt') # 'logs/x/b.txt' correct +s.m_abs('b.txt') # 'logs/x/b.txt' correct KeyCodecs.prefixed('x/')(BucketReader(data, 'logs/')).m_abs('b.txt') - # 'logs/b.txt' WRONG, silently + # 'logs/b.txt' WRONG, silently ``` -**The wrong answer is a plausible `str`**, so a type check does not catch it. Precisely -*because* ADR-0001 puts the prefix in the leaf, the leaf's own `_id_of_key` still fires and -produces a well-formed key that addresses the wrong object. A leaf with no `_id_of_key` would at -least return `None`. - -Measured, method form vs free function, across wrap shape × wrapper lifetime: - -| form | unwrapped (named/temp) | key-codec + named | key-codec + **temp** | `Pipe` + named | `Pipe` + **temp** | -|---|---|---|---|---|---| -| method via `wrapped_self` | correct | correct | **silently wrong** | correct | **silently wrong** | -| free function | correct | correct | correct | correct | correct | - -**Scope of the failure — stated precisely, because an earlier draft overstated it.** The -precondition is *a user-applied **key** codec* **and** *no live strong reference to the wrapper -at call time*. It is **not** triggered by `s3_store('bucket', prefix='p').handle(k)`: `s3_store` -returns a bare leaf or a **value**-codec wrap, and both are correct as temporaries (verified). -Nor by `filt_iter` alone. The failing shape is -`KeyCodecs.prefixed('x/')(s3_store(...)).handle(k)`. - -"Temporary" is also the wrong word: the predicate is *no live strong reference*. A temporary -caught in a reference cycle silently starts working, and `operator.methodcaller('m', k)(obj)` is -correct where `obj.m(k)` is not — so the failure is **intermittent**, which is worse than -deterministic even though it is rarer. - -**It is detectable, contrary to an earlier draft of this section.** `Store.__init__` probes the -leaf with `hasattr(self.store, "KeysView")`, so a leaf can record that it was ever wrapped and -then refuse when `wrapped_self(self) is self` but that flag is set. Verified: it catches the -instance-wrap, class-wrap and `Pipe` temporary cases and stays silent on a genuinely unwrapped -store. Caveats: it rides on an incidental probe, it does not fire on the unpickle path (which -re-registers via `__setstate__`, bypassing `__init__`), and it false-positives on a leaf that -was wrapped once and is later used bare. A two-line upstream change to -`_register_wrapper_backref` would do it properly. - -So the honest conclusion is narrower than "the method form is impossible": **the method form can -be made loud rather than silently wrong, at the cost of a hack.** Whether that is worth keeping -is the open fork below. - -This does still demote `wrapped_self` generally: it is a **best-effort guardrail with a silent -failure mode**, not "the correct escape" that -[ADR-0006](0006-key-scoping-and-dol-fixes.md) §2 called it. The hole goes upstream (D9). - -### D2 — One key-resolution primitive, and it takes the store as an argument +The wrong answer is a **plausible `str`** — precisely because ADR-0001 puts the prefix in the +leaf, the leaf's own `_id_of_key` still fires — so a type check cannot catch it. -```python -from dol.dig import inner_most_key # NOT exported from `dol` — see ADR-0006 §3 +**Four corrections to that draft, all from an adversarial review, all re-verified:** + +1. **The predicate is "no live strong reference", not "temporary".** A temporary caught in a + reference cycle silently starts working, and `operator.methodcaller('m', k)(obj)` is correct + where `obj.m(k)` is not. The failure is **intermittent**. +2. **The trigger is narrower than claimed.** `s3_store('bucket', prefix='p').handle(k)` is + **not** an instance of the bug: `s3_store` returns a bare leaf or a **value**-codec wrap, and + both are correct. Nor is `filt_iter` alone. Only a user-applied **key** codec breaks it. +3. **It is detectable.** `Store.__init__` probes the leaf with `hasattr(self.store, "KeysView")`, + so a leaf can record that it was ever wrapped and refuse when `wrapped_self(self) is self` + but that flag is set. Verified against instance-wrap, class-wrap and `Pipe`. So the honest + claim is *"a method can be made loud rather than silently wrong, at the cost of a hack"* — + not *"a method is impossible"*. +4. **Reproduced on CPython 3.10–3.14**; not gc-, version- or bytecode-specific. + +So the method form is rejected on **cost/benefit**, not impossibility: it needs a guard riding +on an incidental `hasattr` probe, which misses the unpickle path and false-positives on a +once-wrapped leaf used bare. D2 gets the same correctness with none of that. +This still demotes `wrapped_self` generally: it is a **best-effort guardrail with a silent +failure mode**, not "the correct escape". The hole goes upstream (D9). + +### D2 — Keyed capabilities are sibling *stores*, keyed through `__getitem__` + +A capability becomes a Layer B `KvReader` over the same key space whose `__getitem__` returns +the capability: -def _abs_key(store, k: str) -> str: - """The absolute S3 key for `k` in `store`. Correct at any wrapper depth.""" - _id = inner_most_key(store, k) - if not isinstance(_id, str): - raise KeyNotValid(...) # never let a None reach the wire - return _id ``` +BucketCollection → BucketReader → BucketStore k -> bytes +BucketHandles k -> ObjectHandle +BucketUrls k -> presigned URL (str) +BucketInfo k -> ObjectInfo +``` + +All four share one private base holding `prefix`, connection and `_id_of_key`/`_key_of_id`, so +the key arithmetic exists once. -**`store` is a parameter, not `self`.** The caller's expression holds the store alive for the -duration of the call, `inner_most_key` walks the real `.store` chain, and the `wrapped_self` -weakref registry is never consulted — so D1a's hole cannot occur. Verified correct across -wrapped/unwrapped × named/temporary and a `Pipe` stack, where the method form fails two of -those. +**`__getitem__` is the one thing `dol` maps correctly at every depth**, so this is correct *by +construction*: no `inner_most_key`, no `wrapped_self`, no private `dol.dig` import, no upstream +PR, no guard. Verified correct with no live reference, under `Pipe`, under `cached_keys`, and +under a hand-rolled non-`Store` passthrough layer — the last being a case where the +free-function form is **silently wrong** (D3a). -**But it is not categorically safe, and this ADR must not claim it is.** `inner_most_key` walks -`.store` applying each layer's `_id_of_key`, which breaks when a layer in the chain is **not** a -`dol` `Store`. Verified against a ground-truth oracle: +It also restores `[k]` ergonomics, and subsumes three capability features +[ADR-0009](0009-scope-and-deferrals.md) currently defers (presigned-URL store, +ObjectInfo-from-LIST, handles) into one mechanism rather than three. -| chain | wire key | free function | method form | -|---|---|---|---| -| `KeyCodecs` / `Pipe` / `cached_keys` / `filt_iter` / `wrap_kvs` over the leaf | — | correct | correct (if named) | -| hand-rolled `__getattr__`-passthrough delegator over the leaf | `logs/b.txt` | **`logs/logs/b.txt`** | correct | -| same delegator over a key codec | `logs/x/b.txt` | **`logs/x/x/b.txt`** | correct | -| key codec over a plain `.store`-holding middle layer | `logs/x/b.txt` | **`x/b.txt`** | also wrong | +**The cost, stated plainly:** a user who wraps the data store must wrap the sibling in parallel. -Cause: a passthrough `__getattr__` resolves `_id_of_key` to the *leaf's* bound method, so the -walk applies it once at the delegator and again at the leaf; a middle layer with no `_id_of_key` -truncates the walk instead. Every `dol`-shipped wrapper is safe because every `Store` subclass -inherits an identity `_id_of_key` — but `dol/base.py` ships a documented hand-rolled `Delegator` -recipe of exactly the breaking shape. +```python +s = s3_store('bucket', prefix='p/') +h = s3_handles('bucket', prefix='p/') +h['f'].url() # correct -So the free function is *more* reliable than the method form, not *reliable*. Any claim of -"correct at any wrapper depth" is false and must not appear in the docstring. +c = KeyCodecs.prefixed('x/') # user-applied key codec +s2, h2 = c(s), c(h) # wrap BOTH +h2['f'] # -> p/x/f correct at any depth, any lifetime +``` + +`s3dol.handles(store)` / `.urls(store)` / `.info(store)` derive a sibling from an **unwrapped** +store and **raise** on a wrapped one, naming the parallel-wrap remedy. That refusal is reliable: +these are free functions holding the store, so `isinstance(store, dol.base.Store)` is a real +test — no weakref, none of D1a's fragility. Re-deriving the user's codec chain onto a sibling is +`dol`'s recursive-wrap problem (dol#10) and is explicitly **not** attempted here. -**It replaces `_id_of_key`. It never composes with it.** Because ADR-0001 puts the prefix in the -leaf, `inner_most_key` walks the whole chain *including the leaf's own `_id_of_key`*, so it -already returns the fully-prefixed absolute key. Composing the two double-prefixes, silently: +### D3 — Free functions for everything not keyed through a store + +Bulk and endpoint-level operations have no Mapping to ride on, so they are free functions: ```python -# store scoped to 'logs/', outer key 'a.txt' -inner_most_key(store, k) # 'logs/a.txt' correct -store._id_of_key(inner_most_key(store, k)) # 'logs/logs/a.txt' WRONG +s3dol.delete_many(store, keys) # D4; see ADR-0010 §2 +s3dol.prefixes(store) # relative to the caller's key space +s3dol.sub(store, prefix) # a store in the CALLER's key space +s3dol.delete_bucket(endpoint, name, force=True) # D4; see ADR-0010 §3 ``` -That instinct — reach for `_id_of_key` — is exactly what an author will have, so the rule is -restated in ADR-0006 §2. +This matches `dol`'s own idiom (`content_url`, `get_content`, `put_content`, `add_content` all +take the store first). -The `str` check is **mandatory, not defensive**: `inner_most_key` returns `None` — silently, via -`last_element` over an empty generator — when no layer in the chain supplies `_id_of_key`. +**`sub` has an obligation:** resolving the absolute prefix is not sufficient, because a leaf +sub-store would speak the leaf's key space and silently drop the user's outer codec. On an +unwrapped store use `leaf._with(prefix=…)` (cheap, pushes down); on a wrapped store compose over +the **outer** store with `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` +([ADR-0006](0006-key-scoping-and-dol-fixes.md) §1) — correct, but it loses pushdown (D8) and +returns a different type than the unwrapped branch. Document both. -**Two implementation obligations this creates**, because not every capability is a pure -key→address mapping: +**`prefixes` has the mirror obligation:** its results must be mapped **outward** through the +chain (`_key_of_id` per layer, outermost last) — the inverse of `inner_most_key`, which +`dol.dig` does not provide. Implement locally with a round-trip property test. -- **`sub(store, prefix)`** must return a store *in the caller's key space*. Resolving the - absolute prefix is not sufficient: a leaf sub-store built from it would speak the leaf's key - space, silently dropping the user's outer codec. On an unwrapped s3dol store, use - `leaf._with(prefix=…)` (cheap, pushes down). On a wrapped store, compose over the **outer** - store with dol's own safe form, `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` - ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §1) — correct, but it loses pushdown, which is - the accepted cost in D8. -- **`prefixes(store)`** returns keys relative to the store, so its results must be mapped - **outward** through the chain (`_key_of_id` per layer, outermost last) — the inverse of - `inner_most_key`, which `dol.dig` does not currently provide. Implement it locally and add a - round-trip property test (`∀ p: abs → rel → abs` is identity). +#### D3a — Free functions are *more* reliable than methods, not reliable -### D3 — Free functions are the *only* reliable form, so they are the whole keyed API +`inner_most_key` walks `.store` applying each layer's `_id_of_key`, which breaks when a layer is +**not** a `dol` `Store`. Verified against a ground-truth oracle: -```python -s3dol.handle(store, k) # -> ObjectHandle (then .url(), .info(), ranged reads, …) -s3dol.url_for(store, k) # == s3dol.handle(store, k).url() -s3dol.info(store, k) # == s3dol.handle(store, k).info() -s3dol.sub(store, prefix) # -> a store in the CALLER's key space (see D2) -s3dol.prefixes(store) # -> relative to the caller's key space (see D2) -s3dol.delete_many(store, keys) -``` +| chain | wire key | free function | method form | +|---|---|---|---| +| `KeyCodecs` / `Pipe` / `cached_keys` / `filt_iter` / `wrap_kvs` | — | correct | correct (if referenced) | +| hand-rolled `__getattr__`-passthrough over the leaf | `logs/b.txt` | **`logs/logs/b.txt`** | correct | +| same passthrough over a key codec | `logs/x/b.txt` | **`logs/x/x/b.txt`** | correct | +| key codec over a plain `.store`-holding middle layer | `logs/x/b.txt` | **`x/b.txt`** | also wrong | + +A passthrough `__getattr__` resolves `_id_of_key` to the *leaf's* bound method, so the walk +applies it at the delegator and again at the leaf; a middle layer without `_id_of_key` truncates +the walk instead. Every `dol`-shipped wrapper is safe (every `Store` inherits an identity +`_id_of_key`) — but `dol/base.py` ships a documented hand-rolled `Delegator` recipe of exactly +the breaking shape. + +**No docstring may claim "correct at any wrapper depth" for a free function.** D2's stores may: +they never call `inner_most_key`. + +#### D3b — `url_for` survives as a method, for the protocol only + +`dol.SupportsUrlFor` requires a `url_for` **method**, and `dol.content_url` reaches it with +`getattr(store, 'url_for', None)`. With no method anywhere, `content_url` would return `None` +for every s3dol store forever — and `dol/content.py`'s module docstring names an `s3dol` store +as its intended S3 backend. + +So `BucketReader.url_for(k)` ships, with the D1a §3 guard: correct when unwrapped or when the +wrapper is referenced, and **raising** (naming `s3dol.urls(store)[k]`) when the wrapper is +unreachable. Verified correct-or-loud in all three cases, and `dol.content_url` returns the +right URL for both an unwrapped and a referenced-wrapped store. -The function resolves the key through the whole wrapper chain once, then acts. This is already -`dol`'s idiom (`dol.content_url`, `get_content`, `put_content`, `add_content` all take the store -first), it composes at any wrapper depth, it is unaffected by whether `dol` wraps by has-a or -is-a, and — per D1a — it is the only form that is correct when the store is a temporary. +`s3dol.urls(store)[k]` (D2) remains the canonical form. `url_for` is a compatibility shim with a +documented limitation, and the real fix is upstream: `content_url` must resolve through the +chain **and** call the innermost `url_for` with the resolved key — resolving without that second +half would double-transform. -D1a upgrades this from "the safe general form, offered alongside methods" to "the form". There -is no method variant to fall back to, because a method variant would be right most of the time -and silently wrong the rest, which is the worst available option. +### D4 — Destructive operations are free functions -**What remains ergonomic without a keyed method:** `__getitem__` *is* correctly key-mapped by -`dol` at every wrapper depth, so `store[k]`, `store[k] = v`, `del store[k]`, `k in store` and -iteration stay the primary surface and stay correct — which is the Mapping-first promise in -[architecture.md](../architecture.md) goal 1. Only the *capabilities* move out of method -position, and they were always the opt-in part. +`delete_many(keys)` and `EndpointStore.delete(name, force=True)` both leave Layer B. -**With one correction to the argument #14 made for it:** `dol.content_url` does *not* currently -resolve through the chain — it does a flat `getattr(store, 'url_for')(key)` -(`dol/content.py:210-214`) and returns a URL for the unmapped key. The *shape* is prior art; the -*resolution* is not. Fixing it upstream is a prerequisite for s3dol to serve as `dol.content`'s -S3 backend, which `dol/content.py`'s own module docstring names as the intended arrangement. +Destructive **+** key-taking **+** delegated is the shape the census found in `cosmodol` +(`CosmosItems.replace`/`batch`, `CosmosDatabase.delete`, `CosmosAccount.delete`), `azuredol` +(`AccountStore.delete`) and `pydrivedol` (`GDStore.upload`). Latent or not, s3dol does not add +another. [ADR-0010](0010-bucket-and-bulk-operations.md)'s semantics are unchanged — 1000-key +chunking, the HTTP-200 `Errors` parse, `S3PartialFailure`, the refusal to cascade implicitly — +only the surface moves. -### D4 — `delete_many` is a free function only +`del endpoint[name]` stays, and still refuses a non-empty bucket: it is a Mapping dunder, so +`dol` maps it correctly. -No method on the store. Destructive **+** key-taking **+** delegated is the exact combination -the census found already destroying the wrong data in three sibling packages. We do not add a -fourth. [ADR-0010](0010-bucket-and-bulk-operations.md) §2's semantics — 1000-key chunking, the -HTTP-200 `Errors` parse, `S3PartialFailure` — are unchanged; only the surface moves. +One honest caveat: a module-level `delete_many(store, keys)` accepts any object as its first +argument, where a method could not be called on a store lacking it. Free-function form fixes the +*unmapped key* problem, not the *wrong target* problem. Validate the first argument. -### D5 — Option B's guard, without Option B's machinery +### D5 — A conformance test, not a registry -No `_key_methods` registry. Instead, a **reflective conformance test** that enumerates the -public methods of every Layer B class and fails on **any** that takes a key-shaped first -argument. Under D1a the allowlist is *empty*, which makes the test a simple, unarguable -invariant rather than a list someone has to curate. +No `_key_methods` registry. A **reflective conformance test** enumerates the public methods of +every Layer B class and fails on any that takes a key — allowlist: `url_for` (D3b), and nothing +else. + +The predicate cannot be "first argument is named like a key": that misses `delete(name, ...)`, +`delete_many(keys)`, `cosmodol`'s `batch(operations)` and `sshdol`'s `sync_to(target)` — several +of the shapes this ADR most cares about. It is "does this method accept anything that reaches +the wire as a key, at any argument position or nested in a structure", which is a judgement. +The test therefore asserts against an **explicit inventory of every public method** and fails on +any *new* one, forcing the judgement at review time rather than pretending a signature check +suffices. #14 observed that a declarative registry fails the same silent way when an author forgets to -declare a method, and that the reflective test is "the kind of guard that actually holds". Since -the test is the part that holds and the registry is the part that can be forgotten, we ship the -test and skip the registry. +declare a method, and that a reflective test is "the kind of guard that actually holds". Note +also that `dol` already ships a hook of Option B's shape — +`wrap_kvs(ingoing_key_methods=…, outcoming_key_methods=…)` — which is untested and verified +broken for leaf-defined methods on both wrap paths (it fails loudly, at least). Option B would +mean replacing it, not building on it. -**D5 is not yet writable**, though: a "key-shaped first argument" heuristic misses -`delete(name, force=True)`, `delete_many(keys)` and `batch(operations)` — three of the shapes -this ADR is most worried about. The predicate has to be "does this method take anything that -gets used as a key", which is a judgement, not a signature check. Resolve with the open fork. +### D6 — Option E: rejected as an *attribute*, adopted as a *store* -Note also that `dol` already ships a declarative hook of exactly Option B's shape — -`wrap_kvs(ingoing_key_methods=…, outcoming_key_methods=…)` — which is untested (`dol/trans.py` -carries the TODO) and **verified broken for leaf-defined methods** on both wrap paths. Option B -would mean replacing it, not building on it. +#14's Option E argued that capabilities-as-Mappings work because "key transformation happens +through the Mapping protocol the wrapper already handles correctly." -### D6 — Option E (capabilities as parallel Mappings) is deferred to v1.x +- **As a Mapping-valued *attribute* (`store.urls`) that is false.** Verified: a wrapper does not + re-wrap such an attribute; `store.urls` returns an inner-keyed mapping under both class- and + instance-wrap. That form needs the same key resolution as everything else. +- **As a sibling *store* it is exactly true**, and that is D2. -#14 argued E is the most dol-native option because "key transformation happens through the -Mapping protocol the wrapper already handles correctly." **That is not true at dol 0.3.58.** A -wrapper does not re-wrap a Mapping-valued attribute; `store.urls` returns an inner-keyed mapping -under both class- and instance-wrap. E only works if the view itself calls -`inner_most_key(wrapped_self(...))` — the same primitive as D2. +The first draft of this ADR conflated the two and deferred Option E wholesale. That was wrong: +the provisional lean in #14 (C + E, with B) was right in substance — the correction is that E's +value comes from being a *store*, not an attribute, and that C's role shrinks to the +non-keyed operations. -So E is a *surface* over D2's mechanism, not an alternative mechanism. It buys ergonomics and -collapses three [#12](https://github.com/i2mint/s3dol/issues/12) deferrals into one shape; it -does not buy correctness, and it costs new machinery. Revisit when `dol`'s `.meta` sidecar -design lands, which is the mechanism E actually wants. +Making `store.urls` work as an attribute would need `dol` to propagate and re-wrap +Mapping-valued attributes — dol#10, and the `.meta` sidecar design in +`dol/misc/docs/dol_content_metadata_bifurcation.md` §2.2, whose stated blocker is precisely +key-transform propagation. Deferred to v1.x; D2 does not depend on it. ### D7 — Option D (rebind delegated methods to the wrapper) is rejected Not primarily for blast radius. `dol/misc/docs/dol_issue18_design.md` surveyed 26 ecosystem sites and rejected the rebind family on three verified defects, the fatal one being that -rebinding binds `self` to the **innermost** `Wrap` — so under a `Pipe` stack it *does not fix -the case it exists to fix*, and stacked-codec writes gain a partial-transform corruption -surface. Plus statically-undetectable crashes: a leaf method calling `super().__getitem__(k)` -compiles `super(SomeClass, self)` with `self` now a `Wrap` → `TypeError`. +rebinding binds `self` to the **innermost** `Wrap` — so under a `Pipe` stack it *does not fix the +case it exists to fix*, and stacked-codec writes gain a partial-transform corruption surface. +Plus statically-undetectable crashes: a leaf method calling `super().__getitem__(k)` compiles +`super(SomeClass, self)` with `self` now a `Wrap` → `TypeError`. -That document's Phase-1 instructions say verbatim: *do not touch `DelegatedAttribute.__get__`, -the `delegate_to` copy loop, or the signature graft.* We comply. +That document's Phase-1 instructions say: *"**Do not touch** `DelegatedAttribute.__get__`, the +`delegate_to` copy loop, or the `base.py:451` signature graft."* We comply. -**The terminal fix is `dol`'s is-a wrapping** (dol#18 Approach C, committed for dol 0.4/1.0), -which resolves dol#18 and dol#6 together and makes this ADR's machinery redundant. That is why +**The terminal fix is `dol`'s is-a wrapping** (dol#18 Approach C), which would resolve dol#18 and +dol#6 together and make D1a moot. Note it is the doc's *recommended* terminal direction but +still an open question for the maintainer (§9 of that doc), and Phases 2–3 have not shipped. So the selection criterion here was *correct on dol 0.3.x **and** harmlessly redundant under is-a* — -not *permanent*. D2's helper degrades to a no-op; D3's free functions stay correct; D1's smaller -surface stays desirable on its own merits. +which D2 satisfies: sibling stores stay correct either way. ### D8 — Prefix pushdown is closed, not deferred ADR-0001 already solves pushdown for the prefix s3dol owns: the leaf passes `ListObjectsV2(Prefix=self.prefix)`. The residual case is a user stacking `filt_iter.prefixes(…)` -on top, which is **general predicate pushdown** — `dol` has no framework for it, and a +on top, which is **general predicate pushdown** — `dol` has no framework for it, and an `__iter__(*, prefix_hint=…)` protocol would have exactly one implementer, violating [ADR-0009](0009-scope-and-deferrals.md)'s own "no new `Protocol` without two implementers" rule. -Decision: a user-stacked filter **accepts a full scan**, and `s3dol.sub(store, prefix)` is the -documented cheap path. On an unwrapped store `sub` costs zero round-trips and pushes down, so -the fast route exists and is one call away. This is an answer, not a deferral; do not reopen it +A user-stacked filter **accepts a full scan**; `s3dol.sub(store, prefix)` is the documented cheap +path and pushes down on an unwrapped store. This is an answer, not a deferral; do not reopen it without a second implementer. -### D9 — New upstream finding: `wrapped_self` has a temporary-wrapper hole - -dol#18 shipped `wrapped_self` as the blessed pattern for delegation-wrapped classes. D1a shows -it silently degrades to the raw leaf whenever the wrapper is a temporary, because the delegated -bound method holds no reference to it and the weakref cleanup removes the evidence. Any -`*dol` package that adopted the blessed pattern — `xdol` and `unbox` have — -inherits this. - -This is not a blocker for s3dol (D3 routes around it entirely), but it belongs upstream on -dol#18 with the repro, because the documented remedy for a *No Silent Failures* project -currently has a silent failure. Possible directions for dol, none of them s3dol's to choose: -have `DelegatedAttribute.__get__` return a wrapper-retaining bound method; keep a strong -reference for the duration of the call; or land is-a wrapping, which removes the registry -entirely. - -## Open fork — how the keyed capabilities are actually surfaced +### D9 — Upstream findings -An adversarial review of the first draft refuted three of its supporting claims (all corrections -are folded in above) and surfaced a fourth option the draft never evaluated, because D6 -conflated *capability as a Mapping-valued **attribute*** (broken — verified) with *capability as -a sibling **store***, which is a different design. +Blocking for s3dol ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §3): -**Option S — capability stores.** A capability becomes a Layer B `KvReader` over the same key -space whose `__getitem__` returns the capability: +1. **Export `dol.dig.inner_most_key`** and harden `store_trans_path` (raise instead of returning + `None`; fix `dol/dig.py:41` hardcoding `unravel_key`). Needed by D3's free functions. +2. **Fix `dol.content_url`** (`dol/content.py:210-214`) — see D3b for the two-part fix. -```python -class BucketHandles(KvReader): # zero key-taking methods - def _id_of_key(self, k): - return self.prefix + k - - def __getitem__(self, k): - return ObjectHandle(self.bucket, self._id_of_key(k)) -``` +Non-blocking, reported not fixed by us: -`__getitem__` is the one thing `dol` maps correctly at **every** depth, so this is correct *by -construction*: no `inner_most_key`, no `wrapped_self`, no private `dol.dig` import, no upstream -PR. Verified 5/5 including a temporary under `Pipe`, under `cached_keys`, and under the -hand-rolled delegator **where the free function is silently wrong**. It also restores `[k]` -ergonomics and subsumes three of the capability features currently deferred in ADR-0009. - -Its cost is real: a user who wraps the data store must wrap the sibling in parallel -(`KeyCodecs.prefixed('x/')` applied to both), because `store.handles` as an *attribute* is the -broken form. And it does not cover non-keyed bulk operations (`delete_many`, `prefixes`), which -stay free functions regardless. - -Three further blockers must be resolved with this fork, in any option: - -1. **`EndpointStore.delete(name, force=True)`** — still specified in - [architecture.md](../architecture.md), [ADR-0010](0010-bucket-and-bulk-operations.md) §3 and - [ADR-0007](0007-naming-and-compatibility.md). It is a public, key-taking, destructive, - delegated Layer B method — structurally identical to `azuredol.AccountStore.delete`, which - this ADR cites as a census exhibit. Either D5's empty allowlist fails on day one, or D5's - "key-shaped first argument" heuristic misses it — and the same heuristic misses - `delete_many(keys)`, `cosmodol`'s `batch(operations)` and `sshdol`'s `sync_to(target)`. D5 - needs a real predicate, not a name heuristic. -2. **`dol.SupportsUrlFor` requires a `url_for` *method***, and `dol.content_url` reaches it with - `getattr(store, 'url_for', None)`. Under a zero-method Layer B, `content_url` returns `None` - for every s3dol store forever. That makes the `dol.content` integration a **protocol change** - upstream, not the "small PR" D3 implies. -3. **`url_for` needs `(endpoint, bucket, key)`, and only the key has a resolution primitive.** - `recursive_get_attr(chain, 'bucket')` returns the *first* layer carrying a `bucket` attribute, - so a middle layer with its own can pair a correctly-resolved key with the wrong bucket. Either - add a `_leaf_of` primitive or state the limitation. - -Also pending, independent of the fork: ADR-0011 must be added to `misc/docs/README.md` (which -still teaches the retired `inner_most_key(wrapped_self(self), k)` form), and the superseded -"a method may be added iff it takes a key…" rule survives verbatim in -[ADR-0005](0005-large-object-io.md) §2 and [ADR-0009](0009-scope-and-deferrals.md) §v1.0 scope. +3. **`wrapped_self` has a lost-reference hole** (D1a). dol ships it as the *blessed* pattern and + `xdol`, `unbox` and `lexis` have adopted it, so they inherit a silent failure mode. dol's own + test suite does not cover it — all of `dol/tests/base_test.py` binds the wrapper to a name. +4. **`dol.filesys.is_valid_key`/`validate_key`** (`dol/filesys.py:422,425`) — confirmed-live, and + the best regression sentinel for any future delegation fix. `dol/paths.py:1199-1206` already + carries the hand-rolled fix for the same shape. +5. **`dol#83`'s own "Ask"** requests the two things this ADR rejects (bless `wrapped_self`; ship + a declarative key-method helper). It needs correcting with D1a and D7. ## Consequences -**Buys.** Zero keyed seams instead of six, guarded by an invariant with an empty allowlist -rather than by discipline. A resolution primitive verified correct in 6/6 wrap × lifetime -shapes, where the obvious alternative is 2/4 and fails silently. No destructive delegated method -anywhere in the package. The Mapping surface — which `dol` maps correctly — stays the primary -API. Nothing that has to be unwound when `dol` lands is-a wrapping. - -**Costs.** These are real and this ADR does not pretend otherwise. - -- `store.url_for(k)` becomes `s3dol.url_for(store, k)`. That reads worse, and it is a - divergence from v0 that `store.py`'s compat shim - ([ADR-0007](0007-naming-and-compatibility.md)) must absorb — the shim can keep the method on - the legacy class, since a legacy `S3Store` is not something users key-wrap. -- The capability API no longer tab-completes off a store, which is a genuine loss for the - notebook-explorer use case that [state-of-play](../state-of-play.md) §1 names first. Mitigate - in docs: `s3dol.` is the discovery surface, and `__getitem__`/iteration still cover the - common path. -- `sub` and `prefixes` become non-trivial to implement correctly (D2's two obligations), where - as leaf methods they were three lines. -- s3dol depends on `dol.dig.inner_most_key`, which is not public API — a small upstream PR - ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §3). +**Buys.** The keyed surface is correct *by construction* rather than by a resolution primitive: +D2's stores never call `inner_most_key`, so they are immune to both D1a's lost-reference hole and +D3a's non-`Store`-layer hole — the only form in this ADR that is. `[k]` ergonomics survive. Three +ADR-0009 deferrals collapse into one mechanism. No destructive delegated method. Nothing to +unwind when `dol` lands is-a wrapping. + +**Costs.** + +- **Parallel wrapping.** Wrapping the data store does not wrap its siblings, and s3dol will not + guess the chain. `s3dol.handles(store)` raises rather than silently returning an unwrapped + sibling, so the cost is visible — but it is a real ergonomic tax on the one case (user-applied + key codec) this whole ADR is about. +- **More classes.** Four Layer B readers where there was one, plus factories. +- `sub` and `prefixes` become non-trivial (D3's two obligations) where as leaf methods they were + three lines, and `sub` returns different types on the wrapped and unwrapped branches. +- `url_for` survives as a guarded shim, which is one exception to an otherwise clean rule, and + the guard rides on an incidental `dol` implementation detail. +- s3dol depends on `dol.dig.inner_most_key`, not currently public API. **What NOT to do.** -1. **Do not add a key-taking method to a Layer B class** — not even "just this one", not even - hardened with `wrapped_self`. D1a is why: the hardened form is silently wrong on temporaries - and the failure is undetectable. Add it to `ObjectHandle` (key bound at construction) or ship - a free function. The conformance test enforces an *empty* allowlist; do not add entries. -2. Do not compose `_abs_key` with `_id_of_key`. Re-read D2. -3. Do not treat `wrapped_self` as a correctness mechanism. It is a guardrail with a known - silent failure mode (D1a, D9). -4. Do not rely on `isinstance(store, SupportsUrlFor)` to detect a capability — a - `@runtime_checkable` Protocol checks presence, not correctness, and since 3.12 `isinstance` - uses `getattr_static`, which sees a class-wrapped capability but not an instance-wrapped one. -5. Do not re-propose rebinding delegated methods. See D7 and the upstream evidence. -6. Do not invent a pushdown hint protocol for one implementer. See D8. +1. **Do not add a key-taking method to a Layer B class** — not even hardened with `wrapped_self` + (D1a). Add a sibling capability store (D2), put it on `ObjectHandle` (key bound at + construction), or ship a free function (D3). The conformance test allows `url_for` and + nothing else. +2. Do not compose `inner_most_key(store, k)` with `_id_of_key` — it already includes the leaf's + prefix, and composing double-prefixes silently. +3. Do not claim a free function is correct at any wrapper depth (D3a). +4. Do not treat `wrapped_self` as a correctness mechanism (D1a, D9). +5. Do not rely on `isinstance(store, SupportsUrlFor)` to detect a capability. +6. Do not re-propose rebinding delegated methods (D7). +7. Do not invent a pushdown hint protocol for one implementer (D8). +8. Do not attempt to re-derive a user's codec chain onto a sibling store — that is dol#10. diff --git a/misc/docs/state-of-play.md b/misc/docs/state-of-play.md index abfee5f..bf0cbd6 100644 --- a/misc/docs/state-of-play.md +++ b/misc/docs/state-of-play.md @@ -109,8 +109,10 @@ join the Mapping surface at all. **[0006 — Prefix normalization, key validity, dol traps](decisions/0006-key-scoping-and-dol-fixes.md).** Prefix normalization is mandatory and comes first. The dol corruption table. The delegation -trap and the `inner_most_key(wrapped_self(self), k)` form. The `EncodingType` prohibition. The -reduced dol upstream list. +trap — accurate on the *mechanism*, but its prescribed remedy +(`inner_most_key(wrapped_self(self), k)`) is **retired** by ADR-0011 §D1a; take the remedy from +there. The `EncodingType` prohibition. The dol upstream list, now including the +`inner_most_key` export and the `content_url` fix. **[0007 — Naming and compatibility](decisions/0007-naming-and-compatibility.md).** New names; `s3dol.store.S3Store` as a deprecated shim removed in v2, doubling as the fix-delivery @@ -132,15 +134,16 @@ restraint. Rule: no new `Protocol` without two implementers. paginates. *Amended by 0011: `delete_many` is a free function, not a store method.* **[0011 — Keyed capability surface](decisions/0011-keyed-capability-surface.md).** Resolves §7 / -discussion #14. **Layer B gets zero key-taking public methods**: per-object capabilities live on -`ObjectHandle` (key bound at construction, as `azuredol.BlobHandle` does) and everything else — -`handle`, `sub`, `prefixes`, `url_for`, `info`, `delete_many` — becomes a free function taking -the store first. One primitive, `_abs_key(store, k) = inner_most_key(store, k)`, which -**replaces** `_id_of_key` and must never compose with it, plus a mandatory `str` check. -Conformance test with an empty allowlist. Two findings drive it: **`azuredol` is robust because -it has almost no keyed methods, not because its prefix lives in the leaf** (§D1), and **the -`wrapped_self` escape is silently wrong on temporary wrappers, undetectably** (§D1a) — which is -why free functions are the only form rather than merely the safe one. +discussion #14. **Layer B gets no key-taking public methods.** A keyed capability becomes a +**sibling store** keyed through `__getitem__` (`s3dol.handles(store)[k]`, `.urls`, `.info`) — +correct *by construction*, since `__getitem__` is the one thing `dol` maps correctly at every +depth, so no key-resolution primitive is involved at all. Non-keyed operations are free +functions; `url_for` survives as one guarded method for `dol.SupportsUrlFor`. Three findings +drive it: **`azuredol` is robust because it has almost no keyed methods, not because its prefix +lives in the leaf** (§D1); **the `wrapped_self` escape is silently wrong when the wrapper is +unreferenced** (§D1a); and **free functions are not categorically safe either** — they break on +non-`Store` layers (§D3a). Also carries the corrected census: the family defect is +overwhelmingly *latent*, and 12 survey claims were refuted. ## 4. Verified findings — what is actually wrong with v0.1.9 @@ -321,30 +324,46 @@ wrapper knows to route to. Full record: [ADR-0011](decisions/0011-keyed-capability-surface.md). Short form: -**Decided.** Layer B gets **zero** key-taking public methods. Per-object capabilities move onto -`ObjectHandle` (key bound at construction, as `azuredol.BlobHandle` does); `handle`, `sub`, -`prefixes`, `url_for`, `info` and `delete_many` become **free functions taking the store first** -(Option C). One primitive, `_abs_key(store, k) = inner_most_key(store, k)`, with a mandatory -`str` check. A reflective conformance test with an *empty* allowlist instead of Option B's -registry. Option E deferred; Option D rejected; pushdown closed. +**Decided.** Layer B gets **no key-taking public methods**. A keyed capability becomes a +**sibling store** over the same key space — `s3dol.handles(store)[k]`, `s3dol.urls(store)[k]`, +`s3dol.info(store)[k]` — keyed through `__getitem__`, which is the one thing `dol` maps +correctly at every wrapper depth. Non-keyed operations (`sub`, `prefixes`, `delete_many`, +`delete_bucket`) are free functions taking the store first. `url_for` survives as a single +guarded method purely to satisfy `dol.SupportsUrlFor`. Option D rejected; pushdown closed; +Option E rejected as an *attribute* and adopted as a *store*. -**Correction 0 — the biggest one, and it was found by testing the plan rather than arguing it.** -The first draft kept `handle(k)`/`sub(prefix)` as methods hardened with -`inner_most_key(wrapped_self(self), k)` — the form ADR-0006 §2 prescribed. That form is -**silently wrong whenever the wrapper is a temporary**: +This is close to the provisional lean in #14 (C + E, with B) — refined by evidence: E's value +comes from being a **store**, not an attribute; C shrinks to the non-keyed operations; B +contributes only its guard. + +**Correction 0 — two design drafts were refuted by testing rather than argument.** + +*Draft 1* kept `handle(k)`/`sub(prefix)` as methods hardened with +`inner_most_key(wrapped_self(self), k)` — the form ADR-0006 §2 prescribed. That form is silently +wrong when **nothing holds a reference to the wrapper**, because a delegated bound method holds +none and `wrapped_self`'s weakref cleanup then removes the registry entry: ```python -s3_store('bucket', prefix='logs/') # named -> correct -KeyCodecs.prefixed('x/')(s3_store(...)).handle(k) # temporary -> WRONG, silently +s = KeyCodecs.prefixed('x/')(s3_store(...)); s.handle(k) # correct +KeyCodecs.prefixed('x/')(s3_store(...)).handle(k) # WRONG, silently ``` -A delegated bound method holds no reference to the wrapper, so it is collected before the body -runs; `wrapped_self`'s weakref cleanup then *removes the registry entry*, making it -indistinguishable from "never wrapped". And because the prefix lives in the leaf, the wrong -answer is a plausible `str`, so the type check does not catch it. Measured: free-function form -6/6 correct across wrap × lifetime shapes, method form 2/4. That is what forces "zero methods" -rather than "two". It also demotes `wrapped_self` from *the* escape to a best-effort guardrail, -and adds a new upstream item against dol#18 (ADR-0011 §D9). +Because the prefix lives in the leaf, the wrong answer is a plausible `str`, so a type check +cannot catch it. + +*Draft 2* concluded from that that **free functions** were the only reliable form. An adversarial +review refuted three of its supporting claims, all re-verified: + +- **"Undetectable" was false** — a guard is writable (`Store.__init__` probes the leaf with + `hasattr(store, 'KeysView')`, so a leaf can record that it was ever wrapped). +- **Free functions are not categorically safe** — with a hand-rolled non-`Store` layer in the + chain they return a plausible wrong key in three configurations, two of which the *method* + form gets right. All `dol`-shipped wrappers are fine. +- **`s3_store(...).handle(k)` is not an instance of the bug at all** — `s3_store` returns a bare + leaf or a *value*-codec wrap, both correct. Only a user-applied **key** codec triggers it. + +Sibling stores need no key-resolution primitive, so they are immune to both holes. That is what +settled it. **Correction 1 — the provisional lean above was wrong about Option E.** It claims E works because key transformation "happens through the Mapping protocol the wrapper already handles @@ -356,7 +375,7 @@ third mechanism. That removes its main claimed advantage. **Correction 2 — Option C's cited prior art has the bug.** `dol.content_url` resolves with a flat `getattr(store, 'url_for')(key)` (`dol/content.py:210-214`), so through a key wrap it returns a URL for the unmapped key. The idiom is prior art; the resolution is not. Fixing it is -now a blocking upstream item (§9 of ADR-0006). +now a blocking upstream item ([ADR-0006](decisions/0006-key-scoping-and-dol-fixes.md) §3). **Also, two things the framing above got structurally incomplete:**