Skip to content

Fix: bare store's absolute keys broke once the instance was wrapped; pin dol>=0.3.50 - #3

Merged
thorwhalen merged 1 commit into
masterfrom
fix/bare-store-keys-after-instance-wrap
Sep 22, 2026
Merged

thorwhalen merged 1 commit into
masterfrom
fix/bare-store-keys-after-instance-wrap

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Post-merge refute review of #2.

Defect 1, regression. #2 resolves every aget/asetitem key through inner_most_key(wrapped_self(self), k). wrapped_self climbs from the store to any live wrapper, and it cannot tell a call that came through the wrapper from a direct call on the inner store. So after w = wrap_kvs(bare, id_of_key=..., key_of_id=...), the bare store's own bare.aget(absolute_path) (which worked before #2) runs through id_of_key a second time and raises FileNotFoundError: <rootdir><rootdir>name. asetitem has the same problem: it writes to the wrong path. Reproduced before fixing.

Fix: a key the leaf store already accepts (is_valid_key, i.e. a path under its rootdir) is used verbatim. Relative keys never match an absolute rootdir pattern, so #2's RelPath fix is unaffected (its 4 tests still pass).

Defect 2, packaging. wrapped_self first shipped in dol 0.3.50, but the dependency was an unpinned dol. An environment with an older dol that upgrades aiofiledol gets ImportError at import aiofiledol. Pinned dol>=0.3.50.

Tests: two new tests (read and write through the bare store after an instance-wrap). Both fail on master and pass here. 6 tests + doctests pass.

Residual (not fixed): a wrapper that maps absolute keys to different absolute keys under the same rootdir would now be bypassed. I judged that much rarer than using the bare store directly.

Self-reviewed only (the dispatching run disallowed sub-agents).

🤖 Generated with Claude Code

#2 resolves every aget/asetitem key through wrapped_self's wrapper chain, but
wrapped_self cannot tell a call reached through a wrapper from a direct call
on the inner store. After wrap_kvs(bare, ...), bare.aget(abs_path) went
through the wrapper's id_of_key again and read rootdir+rootdir+name. Keys the
store already accepts are now taken verbatim. Also pin dol>=0.3.50, the first
release with wrapped_self (an older installed dol made import fail).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thorwhalen
thorwhalen merged commit d11723b into master Sep 22, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the fix/bare-store-keys-after-instance-wrap branch September 22, 2026 14:13
@thorwhalen

Copy link
Copy Markdown
Member Author

Post-merge refute review: design concern (not changed in code).

The is_valid_key short-circuit also fires for class-wrapped stores, where the leaf is built by the wrapper and cannot be called directly, so the ambiguity it resolves cannot arise. The usual dol idiom of adding an extension to absolute keys regressed from #2's behaviour:

J = wrap_kvs(AioFileBytesReader, id_of_key=lambda k: k + ".json", key_of_id=lambda i: i[:-5])
s = J(root, max_levels=0)          # root holds both 'a' and 'a.json'
asyncio.run(s.aget(root + "a"))    # b'PLAIN' (reads root/a); with #2 alone: root/a.json

This is the "absolute keys mapped to other absolute keys under the same rootdir" residual the PR body mentions. It covers every suffix/extension wrapper over absolute keys, which is probably more common than calling the bare instance after instance-wrapping it.

From inside aget all three call paths look the same (bound to the leaf, and wrapped_self returns the same wrapper). A heuristic in aiofiledol would depend on dol internals, such as StoreWrap.__init__.__wrapped__, so I did not land one. Options:

  1. In dol: have wrapped_self (or a sibling) report whether the wrapper constructed its inner (class-wrap: store_wrap's StoreWrap) or received it (instance-wrap). aiofiledol would then short-circuit only for instance-wraps.
  2. In aiofiledol: prefer the chain-resolved key when it is itself valid, and fall back to the raw key only when the resolved one is not. This fixes the extension case and Fix: bare store's absolute keys broke once the instance was wrapped; pin dol>=0.3.50 #3's max_levels=0 case, but not Fix: bare store's absolute keys broke once the instance was wrapped; pin dol>=0.3.50 #3's case under max_levels=None, where rootdir + rootdir + name still matches rootdir.*.

Recommend (1). There are no fleet dependents, so the impact is limited.

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