lance-graph-hydrate: merge hydrate_dir/hydrate_file staging bodies - #959
Conversation
Closes ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES, filed by the 2026-08-17 5+3 hardening council (PR #958) and deliberately deferred there: the council judged the uniqueness fix (staging_suffix) worth landing immediately and this larger merge worth doing deliberately, cheap specifically because the crate had zero consumers. That window was still open, so this follow-up closes it before any consumer wires the crate in. New crates/lance-graph-hydrate/src/publish.rs extracts the PUBLISH half shared by hydrate_dir and hydrate_file: pre-rename re-check, rename, cleanup-on-error, and rename-race remap (publish_by_rename), plus the staging removal both fetch-error and empty/checksum-reject paths in both callers also now route through (remove_staging). The FETCH half stays per-caller (list+get many objects vs stream+hash one object) - genuinely different in shape, merging it would obscure more than clarify. Not just deduplication: publish_by_rename runs BOTH a pre-rename re-check AND a post-rename ENOTEMPTY remap for BOTH callers. Before this, hydrate_dir only had the post-rename remap (no pre-check) and hydrate_file only had the pre-check (its real defense, since POSIX file-onto-file rename silently clobbers rather than erroring - a post-rename remap can't see that danger case). Unifying to both strictly narrows each window; it weakens neither existing guarantee. 4 new falsifiers in publish.rs, two of which could not exist before this seam did: a directory-race and a file-race test, each constructing a competing publisher already at the destination and asserting both that the loser is reported AlreadyPublished (not a raw I/O error) and that the winner's content survives untouched. All 4 pre-existing hydrate_dir/ hydrate_file tests pass unchanged - the merge is behavior-preserving at every previously-tested path. Board hygiene in the same commit: LATEST_STATE.md new dated entry, ISSUES.md regrade (RESOLVED, append-only - the original entry's reasoning is left intact, the resolution appended below it). Verification status: not locally compiled in this session's container (same disk constraint as the two prior PRs on this crate) - real verification is this repo's CI, watched per the standing PR-ownership protocol. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_02d88616-7ef0-498a-9aab-6c9b2e8af711) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES, filed by the 2026-08-17 5+3 hardening council (5+3 council: harden lance-graph-hydrate (fast-follow to merged #957) #958) and deliberately deferred there — the council judged the uniqueness fix (staging_suffix) worth landing immediately and this larger merge worth doing deliberately, cheap specifically because the crate had zero consumers. That window was still open, so this closes it before any consumer (q2, OGAR) wires the crate in.crates/lance-graph-hydrate/src/publish.rsextracts the PUBLISH half shared byhydrate_dirandhydrate_file: pre-rename re-check → rename → cleanup-on-error → rename-race remap (publish_by_rename). Both callers' fetch-error and empty/checksum-reject cleanup paths also now route through the sameremove_staging, so the "cleanup ladder" half of the original finding is closed as completely as the "rename-race remap" half.Not just deduplication — a real narrowing
publish_by_renameruns BOTH a pre-rename re-check AND a post-renameENOTEMPTYremap, for BOTH callers. Before this:hydrate_dironly had the post-rename remap (no pre-check);hydrate_fileonly had the pre-check (its real defense — POSIX file-onto-file rename SILENTLY CLOBBERS rather than erroring, so a post-rename remap can't see that danger case). Unifying to both strictly narrows each window; it weakens neither.Test plan
publish.rs, two of which could not exist before this seam did: a directory-race and a file-race test, each constructing a competing publisher already at the destination and asserting BOTH that the loser is reportedAlreadyPublished(not a raw I/O error) AND that the winner's content survives untouched.hydrate_dir/hydrate_filetests unchanged — the merge is behavior-preserving at every previously-tested path (verified by careful manual review of every call site, not by running the suite — see caveat below).lance/arrow/aws-lc-rssub-tree exhausted disk twice earlier this session) as the two prior PRs on this crate (Mint lance-graph-hydrate: generic SoA->S3->volume->Lance hydration pattern #957, 5+3 council: harden lance-graph-hydrate (fast-follow to merged #957) #958), both of which went green on this repo's real CI. Watching this PR's CI as the actual gate.Board hygiene in the same commit:
LATEST_STATE.mdnew dated entry,ISSUES.mdregrade (RESOLVED, append-only — the original entry's reasoning is left intact, the resolution appended below it).🤖 Generated with Claude Code
Generated by Claude Code