Skip to content

fix: two delegation bugs in dol's own code (content_url, filesys key validation) - #85

Merged
thorwhalen merged 3 commits into
masterfrom
claude/fix-delegation-bugs-content-and-filesys
Aug 10, 2026
Merged

fix: two delegation bugs in dol's own code (content_url, filesys key validation)#85
thorwhalen merged 3 commits into
masterfrom
claude/fix-delegation-bugs-content-and-filesys

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Two live bugs in dol itself, both instances of #83's root cause: a wrapper delegates a
non-dunder method to an inner layer bound to that layer, so it receives the outer, unmapped
key.

1. content_url returns a URL for the wrong object

>>> class Served(dict):
...     def url_for(self, key): return f'https://cdn/{key}'
>>> wrapped = KeyCodecs.prefixed('a/')(Served)({'a/k': b'v'})
>>> wrapped['k']
b'v'                          # reads a/k
>>> content_url(wrapped, 'k')
'https://cdn/k'               # WRONG -- addresses k, not a/k

content_url did a flat getattr(store, 'url_for')(key). It now walks the .store chain to
the layer that actually defines url_for, applying each outer layer's _id_of_key on the
way and stopping there — because that layer applies its own.

Stopping matters. Resolving all the way would double-apply the provider's own transform, which
breaks exactly the backend dol/content.py's docstring names — an s3dol store, which owns a
prefix:

content_url(KeyCodecs.prefixed('x/')(Prefixed('logs/')), 'f')
# 'https://s3/logs/x/f'   — not 'https://s3/logs/logs/x/f'

Uses inspect.getattr_static to distinguish a real url_for from a class-wrap's forwarding
DelegatedAttribute — without it the walk stops at the wrapper and never maps the key.

None of the existing tests caught this: they are all identity-keyed.

2. Files(d).is_valid_key(k) is False for a key that exists

>>> s = Files(d)
>>> list(s)
['a.txt']
>>> s.is_valid_key('a.txt')
False                         # WRONG -- and validate_key() raises on it

Files is mk_relative_path_store(FileBytesPersister), so is_valid_key/validate_key were
reached through Store.__getattr__ with the relative key, while the leaf matched them
against a pattern built from the absolute path. This breaks the invariant
all(s.is_valid_key(k) for k in s).

mk_relative_path_store now maps the key before delegating — the same thing its
with_key_validation=True branch (dol/paths.py:1199-1206) already did by hand for
_id_of_key.

focal inherits this via LocalBinaryStore(Files).

Verification

  • Full suite: 513 passed, 3 skipped.
  • 5 new tests: class-wrap, instance-wrap, Pipe, the no-double-apply case, keyless wraps
    unchanged, and the all(is_valid_key(k) for k in s) invariant.
  • Dependents gate byte-for-byte identical to HEAD — xdol 4, pdfdol 23+1 skipped, chromadol
    2, focal 1, dol_cookbook 1, ftpdol 1. mongodol/sqldol/hfdol/dropboxdol/sshdol fail on absent
    backend SDKs both before and after.

Part of #83. Independent of #84 (different modules); either can merge first.

…validation)

Both are instances of the same root cause (#83): a wrapper delegates a non-dunder
method to an inner layer bound to that layer, so it receives the OUTER, unmapped key.

1. `content_url(store, ref_or_key)` did a flat `getattr(store, 'url_for')(key)`.
   On a wrapped store that returns the method bound to an inner layer, so the backend
   got the outer key and returned a URL for a different object than `store[key]` reads:

       wrapped = KeyCodecs.prefixed('a/')(Served)({'a/k': b'v'})
       wrapped['k']              # b'v'                  -> reads a/k
       content_url(wrapped, 'k') # 'https://cdn/k'       -> WRONG, addresses k

   It now walks the .store chain to the layer that actually defines `url_for`, applying
   each OUTER layer's `_id_of_key` on the way and stopping there -- because that layer
   applies its own. Resolving all the way would double-apply the provider's transform,
   which matters for a backend that owns a prefix (an s3dol store, the case
   `dol/content.py`'s docstring names). Uses `inspect.getattr_static` to tell a real
   `url_for` from a class-wrap's forwarding `DelegatedAttribute`.

   None of the existing tests caught this: they are all identity-keyed.

2. `Files(d).is_valid_key(k)` returned **False** for a key that demonstrably exists,
   breaking the invariant `all(s.is_valid_key(k) for k in s)`. `Files` is
   `mk_relative_path_store(FileBytesPersister)`, so `is_valid_key`/`validate_key` were
   reached through `Store.__getattr__` with the RELATIVE key while the leaf matched them
   against a pattern built from the absolute path. `mk_relative_path_store` now maps the
   key first -- the same thing its `with_key_validation=True` branch already did by hand.

Tests: 4 new cases (class-wrap, instance-wrap, Pipe, no-double-apply, keyless wraps
unchanged) plus a filesys case asserting the `all(is_valid_key(k) for k in s)` invariant.

Verified: full suite 513 passed / 3 skipped. Dependents gate byte-for-byte identical to
HEAD (xdol 4, pdfdol 23+1, chromadol 2, focal 1, dol_cookbook 1, ftpdol 1); the rest fail
on absent backend SDKs both before and after.

(2) is also what `focal` inherits via `LocalBinaryStore(Files)`.
This was referenced Aug 10, 2026
Three defects found by an independent refutation pass, all in the new
`_url_for_provider_and_key`, plus its cost. None were caught by the tests.

1. UNBOUNDED WALK. `while layer is not None` had no cycle detection and no depth
   bound, so `content_url(MagicMock(), k)` hung forever -- every `.store` on a mock
   mints a fresh child -- as did a store whose `.store` returns itself. Master was
   bounded by Python's recursion limit; this was not. Any downstream test passing a
   mock store to `content_url` would have wedged CI. Now bounded by an id-based
   `seen` set (cycles) AND a depth cap (chains that never repeat and never end).

2. STATIC LOOKUP BROKE DUCK TYPING. A store providing `url_for` via `__getattr__`
   (proxies, lazy-client wrappers) is invisible to a static lookup, so the walk found
   no provider and `content_url` returned None -- which callers read as "no URL
   exists". That is exactly the silent-wrong-answer failure mode this PR set is
   against. Now falls back to plain `getattr(store, 'url_for')` with the original key
   when the walk finds nothing, making the behaviour a strict superset of master's.

3. COLLISION WITH `with_key_validation=True`. That option redefines `_id_of_key` to
   RAISE `KeyError` on an invalid key, and the new `is_valid_key`/`validate_key`
   wrappers called it -- so the predicate raised for precisely the input it exists to
   answer "no" for, and `validate_key`'s exception type changed from the leaf's to
   `KeyError`. The wrappers now use `PrefixRelativizationMixin._id_of_key`, the
   unvalidated mapping.

Also: `content_url` was 14.5x slower (3.4us -> 49.5us). Two causes, both fixed --
`import inspect` + `from dol.base import ...` ran on every call (hoisted to module
level), and `getattr_static` was called even for unwrapped stores. Checking `.store`
before the static lookup restores the unwrapped fast path (3.2us, master parity), and
replacing `getattr_static` with a hand-rolled MRO scan makes the wrapped path ~7x
cheaper. The scan agrees with `getattr_static` on every shape that matters (instance
attribute, plain method, __slots__, class-wrap, instance-wrap, __getattr__-provided),
and anything it misses falls through to the plain-getattr fallback, so a miss costs
correctness nothing.

Documented the `DelegatedAttribute` duplication (`dol.base` vs the unused `dol.util`
copy) that the isinstance check depends on.

Tests: 3 new cases -- pathological chains terminate, a dynamically-provided `url_for`
is still found, and `with_key_validation=True` coexists with the wrappers.
516 passed / 3 skipped.
@thorwhalen
thorwhalen merged commit 76abae0 into master Aug 10, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the claude/fix-delegation-bugs-content-and-filesys branch August 10, 2026 16:53
thorwhalen added a commit that referenced this pull request Aug 10, 2026
Discussion #86 captured this, but a discussion is easy to lose and hard to cite from
code review. misc/docs/ is where dol's durable design records live, so this lands
next to dol_issue18_design.md (its inverse) and dol_issue10_design.md.

Records, with the running-code evidence for each:

- TWO delegation routes, not one -- Store.__getattr__ AND
  DelegatedAttribute.__get__. A fix covering one is a silent no-op on the other,
  which is why several past fixes had to be applied twice.
- The 13-package census, stated accurately: the defect is overwhelmingly LATENT
  (it bites only when a user applies a key codec), and 12 survey claims were
  refuted outright. An earlier downstream draft claimed the family was actively
  destroying data; it is not, and the record says so.
- Options A-F with what each actually costs:
  A wrapped_self  -- has its own silent hole: degrades to the raw leaf when nothing
                     holds a strong reference to the wrapper, and where the leaf owns
                     a prefix the wrong answer is a plausible str. CPython 3.10-3.14.
                     Detectable via the KeysView probe, so it can be made loud.
  B declarative   -- dol already ships this (ingoing_key_methods), untested and
                     broken for leaf-defined methods. The reflective test is the part
                     that holds, not the registry.
  C free funcs    -- break on a non-Store layer, where the method form is right.
                     dol's own instance (content_url) had the bug until #85.
  D rebind        -- rejected in dol_issue18_design.md with evidence; binds to the
                     INNERMOST wrap, so it misses the Pipe case it exists to fix.
  E parallel maps -- broken as an ATTRIBUTE (a wrapper does not re-wrap one),
                     correct by construction as a sibling STORE keyed through
                     __getitem__. The only option needing no resolution primitive.
  F is-a          -- the terminal fix; dissolves #83, #18 and #6 together.

Section 5 is the explicit carry-forward list for a future redesign: the has-a/is-a
choice is one decision not three; a wrapper must be able to express "this method
takes a key"; the two routes must be unified; key mapping needs an inverse
(inward->outward has no supported helper); weakref backrefs are the wrong substrate
for correctness; non-Store layers exist in the wild; and "how many keyed methods does
this force adapters to write?" is a design metric worth tracking (azuredol has ~0 and
is clean).

Indexed from CLAUDE.md and dol_misc_docs_guide.md. All file:line citations verified
against the current source.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant