Repository navigation
fix(dataloader): bound split cache filenames without legacy aliasing (#261) - #262
Conversation
…tity Keep normal cache filenames and request parameters unchanged. Bound transformed filename components with identity digests and a versioned suffix that cannot alias legacy integer-page cache names. Add offline real-cache and legacy-upgrade regressions for issue #261.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a5d176ec2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parts = (_slug(config), split, str(offset), str(length)) | ||
| components = tuple(_rows_cache_component(part) for part in parts) |
There was a problem hiding this comment.
When a config's slug exceeds 48 characters, this passes the already-lossy _slug(config) into _rows_cache_component, so distinct configs that normalize to the same slug—such as "a/" + "x" * 1000 and "a:" + "x" * 1000—produce the same .v2.json cache path. After one config is fetched, requesting the other silently returns rows from the first; use the raw config to compute the digest when bounding a long config while preserving the legacy slug only for unchanged short names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6445210: when the config component has to be bounded, the digest is now taken over the raw config (_rows_cache_component(slug, identity=config)), not the lossy slug. Short config slugs keep their legacy .json names. New regression test_long_configs_that_slug_identically_keep_distinct_entries ("a/"+"x"*1000 vs "a:"+"x"*1000) fails on 9a5d176 and passes now; test_dataloader*.py 51 passed locally.
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Review
Verdict: LGTM (Closes #261)
Faithful fix for the unbounded / traversal-prone split component in _rows_payload. Ordinary short filenames stay byte-identical (components == parts → .json), so existing cache hits are preserved; anything that must be sanitised or truncated gets a ~{sha256[:32]} identity suffix and a .v2.json basename so a pre-upgrade literal cannot alias the new name.
Checked locally on head 9a5d176:
pytest tests/test_dataloader_cache_names.py→ 23 passedpytest tests/test_dataloader.py tests/test_dataloader_cache_names.py→ 50 passed- Confirmed the new regressions fail when
dataloader.pyis swapped back tomain's implementation (aliasing / nested paths / unbounded names)
Hosted CI is fully green (tests 3.9–3.13 + docs build; deploy skipped). Ready to merge from our side.
(Posted as COMMENT because GitHub blocks APPROVE from the author account.)
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 6445210
Verdict: LGTM — Codex's P2 (long configs that slug identically shared one .v2.json page) is fixed: the bounded config component is now digested from the raw config, while short config slugs keep their legacy .json names.
- New
test_long_configs_that_slug_identically_keep_distinct_entries("a/"+"x"*1000vs"a:"+"x"*1000) fails on 9a5d176, passes on 6445210;test_dataloader.py+test_dataloader_cache_names.py51 passed locally. - Hosted CI on 6445210: build + tests 3.9–3.13 green (deploy skipped).
No unresolved blocking feedback. Ready to merge.
(Posted as COMMENT — author account cannot APPROVE.)
Resolve CHANGELOG.md conflict with recently merged entries.
…ilenames # Conflicts: # CHANGELOG.md
Resolve CHANGELOG.md: keep #261 dataloader cache filenames above newly merged main entries.
…ilenames # Conflicts: # CHANGELOG.md
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 6c5f523: still LGTM. This push only merges current main (1049ab5, after #250/#260/#279 landed); the sole conflict was CHANGELOG.md, resolved by keeping both entries, and the branch diff against main is unchanged apart from that. Full local suite passed and CI is green. Mergeable, no open blocking feedback.
Closes #261.
Summary
Validation on this exact head
Published tree:
7436ba152d43ff5aa70834762c57ec81f8e986cb, based onf3d8b848845e248af8673c931f6d5326fa1e5f65.Failed to create NETLINK_ROUTE socket: Operation not permitted, plustests/test_porous_molecules::test_the_pdf_is_actually_written, whose Chromium/PDF failure also reproduced on the base.Hosted validation
Exact head
9a5d176ec20ca87ff55688b87d0c181ce9ef10b4is terminal green:Independent final review and all applicable hosted checks have passed. The local limitations above remain disclosed. No test gates or workflow settings were weakened; formal repository review and merge protections still apply.