diff --git a/.claude/board/ISSUES.md b/.claude/board/ISSUES.md index ab6d146a..8a3c94c1 100644 --- a/.claude/board/ISSUES.md +++ b/.claude/board/ISSUES.md @@ -26,7 +26,7 @@ file must be mirrored by hand. Both module docs say so; neither can enforce it, and nothing fails when they diverge. That is the live risk this entry exists to keep visible. -## ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES (2026-08-17) — OPEN, partially-mitigated +## ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES (2026-08-17) — RESOLVED (follow-up PR) `copy::hydrate_dir` and `file::hydrate_file` carry near-identical staging → fetch → verify → publish-by-rename bodies. PR #958 unified only the @@ -50,6 +50,44 @@ the merge worth doing deliberately, not as diff-noise inside a hardening PR. Cheap to do now precisely BECAUSE consumers are still zero — that window closes the moment q2 or OGAR wires this in. +> **⊘ RESOLVED (follow-up PR, `crates/lance-graph-hydrate/src/publish.rs`).** +> Landed while the zero-consumer window was still open, per this entry's own +> reasoning. Extracted `publish::publish_by_rename` — the PUBLISH half only +> (pre-rename re-check, rename, cleanup-on-error, rename-race remap); the +> FETCH half (list+get many objects vs stream+hash one object) stayed +> per-caller, genuinely different in shape. `copy::hydrate_dir` and +> `file::hydrate_file` now route ALL staging removal (not only the publish +> tail — the fetch-error and empty/checksum-reject paths too) through the +> same `publish::remove_staging`, so the "cleanup ladder" half of this +> entry's finding is closed as completely as the "rename-race remap" half. +> +> **The asymmetry this entry named (dir fails loudly ENOTEMPTY; file +> silently clobbers) is preserved, not flattened** — `publish_by_rename` +> now runs BOTH defenses (a pre-rename re-check AND a post-rename remap) for +> BOTH callers, which is strictly a narrowing of each window, never a +> weakening: the pre-check was file's real defense (its danger case never +> manifests as a rename error to remap) and is now also narrowing dir's +> window further; the post-rename remap was dir's real defense and is inert +> defense-in-depth for file. See `publish.rs`'s own module doc for the +> full reasoning, so it isn't restated in a second place — which was the +> point. +> +> Four new falsifiers in `publish.rs` prove the merge, including two that +> could not be written before this seam existed (the pre-check narrowing +> `hydrate_dir`'s window is a NEW guarantee, not merely a refactor of an old +> one): `publish_by_rename_directory_race_is_reported_as_already_published_ +> and_cleans_up` and its file-kind twin construct the exact race (a +> competing publisher already occupies the destination when this caller +> reaches its own publish attempt) and assert BOTH that the loser is +> reported `AlreadyPublished` (not a raw I/O error) AND that the winner's +> content survives untouched — a two-sided proof, not just "it doesn't +> crash". All 4 pre-existing `hydrate_dir`/`hydrate_file` tests pass +> unchanged (the merge is behavior-preserving at every previously-tested +> path); `lib.rs`'s own "does NOT merge" deferral paragraph is replaced with +> a "follow-up landed" pointer to this entry, so a future reader of the +> crate doc sees the resolution without needing to cross-reference here +> first. + ## ISS-HYDRATE-NAME-COLLIDES-WITH-TWO-EXISTING-WORKSPACE-MEANINGS (2026-08-17) — OPEN, cosmetic, no-correctness-impact Two names in `lance-graph-hydrate` already mean something else in this diff --git a/.claude/board/LATEST_STATE.md b/.claude/board/LATEST_STATE.md index 57f732f3..8e27987a 100644 --- a/.claude/board/LATEST_STATE.md +++ b/.claude/board/LATEST_STATE.md @@ -1,3 +1,13 @@ +## 2026-08-17 — branch `claude/hydrate-staging-merge` — `lance-graph-hydrate`: the deferred staging/publish merge lands (closes `ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES`) + +### Current Contract Inventory — new private module (`crates/lance-graph-hydrate/src/publish.rs`) + +- **`publish::publish_by_rename(staging, publish_path, StagingKind) -> Result<(), PublishError>`** — the PUBLISH half of `copy::hydrate_dir` and `file::hydrate_file`'s near-identical bodies, extracted per the `crate/lance-graph-hydrate`-hardening council's own named follow-up (`.claude/board/ISSUES.md` `ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES`, filed 2026-08-17, resolved same day once the zero-consumer window it was priced against was confirmed still open). The FETCH half (list+get many objects vs stream+hash one object) stayed per-caller — genuinely different in shape, merging it would obscure more than clarify. +- **A real improvement, 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` had only the post-rename remap (no pre-check — its window was wider than it needed to be); `hydrate_file` had only the pre-check (its real defense, since POSIX file-onto-file rename SILENTLY CLOBBERS rather than erroring, so a post-rename remap can't see the danger case). Unifying to BOTH strictly narrows each window; it weakens neither. +- **`publish::remove_staging(staging, StagingKind) -> io::Result<()>`** — also absorbed the fetch-error and empty/checksum-reject cleanup call sites in both callers (not only the publish tail), so the ISS's "own cleanup ladder" half is closed as completely as the "own rename-race remap" half. Callers choose swallow (`let _ = ...`) vs propagate (`...?`) per call site — the Ok-path-must-not-lie behavior from the prior council pass (C7) is preserved exactly, not weakened by the extraction. +- **Gates:** 4 new falsifiers in `publish.rs` (publish-a-dir, publish-a-file, and — the two that could not exist before this seam — a directory-race and a file-race test, each constructing a competing publisher already at the destination and asserting BOTH `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. +- **Verification status — same honest caveat as the prior two PRs on this crate**: not locally compiled in this session's container (disk-constrained; see the 2026-08-17 entries below for the full history). Real verification is this repo's CI (`rust-test.yml` et al.), watched per the standing PR-ownership protocol. + ## 2026-08-17 — branch `claude/lance-graph-hydrate-hardening-council` — 5+3 council hardens `lance-graph-hydrate` (merged PR #957); corrects 3 claims made in the entry below **Council record:** `.claude/plans/hydrate-crate-hardening-council-v1.md` — diff --git a/crates/lance-graph-hydrate/src/copy.rs b/crates/lance-graph-hydrate/src/copy.rs index 2587aac2..feda29f6 100644 --- a/crates/lance-graph-hydrate/src/copy.rs +++ b/crates/lance-graph-hydrate/src/copy.rs @@ -16,10 +16,13 @@ //! `E-A-REPEATABLE-TRANSFER-IS-NOT-IDEMPOTENCE-OVER-A-MULTI-FILE-DIRECTORY-1`, //! cited rather than restated as new): objects land in a private sibling //! staging directory first, then ONE atomic directory rename publishes the -//! complete artifact. A caller observing the publish path therefore either -//! sees nothing (not yet hydrated) or the complete artifact (hydrated) — -//! never a partial one. This is a filesystem-atomicity boundary, deliberately -//! NOT a lock/lease protocol. +//! complete artifact — via `crate::publish::publish_by_rename`, shared +//! with [`crate::file::hydrate_file`] (see that module's doc for the merge +//! and why the TOCTOU-narrowing details live there now, not restated here). +//! A caller observing the publish path therefore either sees nothing (not +//! yet hydrated) or the complete artifact (hydrated) — never a partial one. +//! This is a filesystem-atomicity boundary, deliberately NOT a lock/lease +//! protocol. //! //! **The idempotency boundary has TWO conditions, not one** (doctrine §4a): //! (a) a pinned source version and (b) an empty/uncontested destination. @@ -30,6 +33,7 @@ //! snapshot or an in-flux one. (For the single-object case, //! [`crate::file::hydrate_file`]'s checksum pin IS condition (a).) +use crate::publish::{publish_by_rename, remove_staging, PublishError, StagingKind}; use crate::staging::staging_suffix; use futures::TryStreamExt; use object_store::{path::Path as ObjPath, ObjectStore}; @@ -90,7 +94,7 @@ pub async fn hydrate_dir( let report = match fetch_all(store, remote_root, &staging).await { Ok(r) => r, Err(e) => { - let _ = tokio::fs::remove_dir_all(&staging).await; + let _ = remove_staging(&staging, StagingKind::Dir).await; return Err(e); } }; @@ -100,28 +104,17 @@ pub async fn hydrate_dir( // `Ok` return promises "leaves NOTHING at publish_dir", and silently // discarding this error would let that promise be false (a council // finding, 2026-08-17). - tokio::fs::remove_dir_all(&staging).await?; + remove_staging(&staging, StagingKind::Dir).await?; return Ok(report); } - if let Err(e) = tokio::fs::rename(&staging, publish_dir).await { - // A concurrent hydrate_dir call may have published between our entry - // check and this rename. `rename` onto an existing non-empty - // directory fails (ENOTEMPTY) — remap that specific race to the - // documented `AlreadyPublished` contract instead of leaking it as an - // opaque `Io` error. This NARROWS the TOCTOU window (checked at - // entry AND at rename) — it does not CLOSE it: a publish landing in - // the instant between this re-check and returning is still - // (vanishingly) possible. No lock is taken (doctrine: filesystem- - // atomicity boundary, not a coordination protocol). - let _ = tokio::fs::remove_dir_all(&staging).await; - return if publish_dir.exists() { + match publish_by_rename(&staging, publish_dir, StagingKind::Dir).await { + Ok(()) => Ok(report), + Err(PublishError::AlreadyPublished) => { Err(HydrateError::AlreadyPublished(publish_dir.to_path_buf())) - } else { - Err(e.into()) - }; + } + Err(PublishError::Io(e)) => Err(e.into()), } - Ok(report) } async fn fetch_all( diff --git a/crates/lance-graph-hydrate/src/file.rs b/crates/lance-graph-hydrate/src/file.rs index 8c04195b..526aa3b3 100644 --- a/crates/lance-graph-hydrate/src/file.rs +++ b/crates/lance-graph-hydrate/src/file.rs @@ -1,8 +1,11 @@ //! Single-file hydration: checksum-pinned download with the same //! hydrate-aside/publish-by-rename discipline as [`crate::copy::hydrate_dir`] -//! (see that module's doc for the mechanism's doctrine citation and -//! provenance), generalized from q2's `osm_slab_hydrate.rs::download_verified` -//! (the `.part` + atomic-rename sidecar pattern for one artifact rather than a +//! — shared via `crate::publish::publish_by_rename`, see that module's +//! doc for the mechanism's doctrine citation, provenance, and why this +//! function's pre-rename re-check (its real defense against POSIX's +//! file-onto-file silent-clobber rename semantics) now lives there — +//! generalized from q2's `osm_slab_hydrate.rs::download_verified` (the +//! `.part` + atomic-rename sidecar pattern for one artifact rather than a //! whole Lance directory — e.g. a `SHA256SUMS` manifest, a single baked //! `.soa`/`.chains`/`.books` sidecar). //! @@ -11,6 +14,7 @@ //! [`crate::copy::hydrate_dir`] has no equivalent and names condition (a) as //! the caller's responsibility instead. +use crate::publish::{publish_by_rename, remove_staging, PublishError, StagingKind}; use crate::staging::staging_suffix; use futures::TryStreamExt; use object_store::{path::Path as ObjPath, ObjectStore}; @@ -82,41 +86,27 @@ pub async fn hydrate_file( let hasher = match fetch_result { Ok(h) => h, Err(e) => { - let _ = tokio::fs::remove_file(&part_path).await; + let _ = remove_staging(&part_path, StagingKind::File).await; return Err(e); } }; let actual = hex_lower(&hasher.finalize()); if !actual.eq_ignore_ascii_case(expected_sha256_hex) { - let _ = tokio::fs::remove_file(&part_path).await; + let _ = remove_staging(&part_path, StagingKind::File).await; return Err(HydrateFileError::ChecksumMismatch { expected: expected_sha256_hex.to_string(), actual, }); } - // Re-assert immediately before the rename: unlike `hydrate_dir`'s - // dir-onto-dir rename (which fails loudly, ENOTEMPTY, on a race), a - // file-onto-file `rename` on POSIX SILENTLY CLOBBERS an existing - // destination rather than failing — so the danger case is exactly the - // one where the rename would otherwise SUCCEED, not where it errors. - // This narrows (does not eliminate) the TOCTOU window between the entry - // check and here; see `copy::hydrate_dir`'s doc for the same caveat - // stated once (a council found the entry-only check here left this race - // completely unguarded, 2026-08-17). - if publish_path.exists() { - let _ = tokio::fs::remove_file(&part_path).await; - return Err(HydrateFileError::AlreadyPublished( + match publish_by_rename(&part_path, publish_path, StagingKind::File).await { + Ok(()) => Ok(()), + Err(PublishError::AlreadyPublished) => Err(HydrateFileError::AlreadyPublished( publish_path.to_path_buf(), - )); - } - - if let Err(e) = tokio::fs::rename(&part_path, publish_path).await { - let _ = tokio::fs::remove_file(&part_path).await; - return Err(e.into()); + )), + Err(PublishError::Io(e)) => Err(e.into()), } - Ok(()) } fn hex_lower(bytes: &[u8]) -> String { diff --git a/crates/lance-graph-hydrate/src/lib.rs b/crates/lance-graph-hydrate/src/lib.rs index 700fd2f7..dd272bcd 100644 --- a/crates/lance-graph-hydrate/src/lib.rs +++ b/crates/lance-graph-hydrate/src/lib.rs @@ -64,13 +64,16 @@ //! policy from `.claude/plans/idle-flush-dataset-eviction-v1.md` — that plan //! is still a PROPOSAL. This crate ships the mechanisms the policy would //! call (hydrate, dirty-check, flush-gate, release); the scheduling policy -//! is deliberately out of scope for v1. It also does not merge -//! [`copy::hydrate_dir`] and [`file::hydrate_file`]'s near-identical -//! staging/publish bodies into one shared primitive beyond the shared nonce -//! helper (`staging::staging_suffix`) — a 5+3 hardening council on this -//! crate (2026-08-17) named that merge as worth considering but explicitly -//! deferred it to keep this PR's diff to the uniqueness fix it actually -//! needed; tracked as a named follow-up, not silently dropped. +//! is deliberately out of scope for v1. +//! +//! **Follow-up landed:** [`copy::hydrate_dir`] and [`file::hydrate_file`]'s +//! staging/publish bodies were merged into `publish::publish_by_rename` — +//! a 5+3 hardening council on this crate (2026-08-17) named the duplication +//! as `ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES` +//! (`.claude/board/ISSUES.md`) and deferred it deliberately, since it was +//! cheap only while this crate had zero consumers; that window was still +//! open, so this follow-up closes it. See the `publish` module's doc for +//! what merged and what stayed per-caller. pub mod copy; pub mod dirty; @@ -78,6 +81,7 @@ pub mod env; pub mod file; pub mod lifecycle; pub mod marker; +mod publish; pub mod release; mod staging; diff --git a/crates/lance-graph-hydrate/src/publish.rs b/crates/lance-graph-hydrate/src/publish.rs new file mode 100644 index 00000000..9b66540f --- /dev/null +++ b/crates/lance-graph-hydrate/src/publish.rs @@ -0,0 +1,197 @@ +//! The shared hydrate-aside/publish-by-rename TAIL for [`crate::copy::hydrate_dir`] +//! and [`crate::file::hydrate_file`]. +//! +//! Extracted during a follow-up to the 2026-08-17 5+3 hardening council, +//! which found (and deliberately deferred, `ISS-HYDRATE-DIR-AND-FILE- +//! DUPLICATE-THEIR-STAGING-BODIES`, `.claude/board/ISSUES.md`) that both +//! functions carried near-identical staging → publish bodies: each with its +//! own TOCTOU window, its own cleanup ladder, and its own rename-race remap. +//! That duplication was the audit surface at risk — a fix to any of the +//! three had to be applied twice or would silently drift. This module is +//! that fix, applied once. +//! +//! **What is shared and what stays per-caller.** The FETCH half (list+get +//! many objects for a directory vs stream+hash one object for a file) is +//! genuinely different in shape and stays in `copy.rs`/`file.rs` — merging +//! it would obscure more than it clarifies. The PUBLISH half — re-check the +//! destination immediately before the rename, attempt the rename, and on +//! failure decide whether that failure IS the doctrine's idempotency- +//! boundary condition (a concurrent publisher won the race) or a genuine +//! I/O error — is identical in intent between the two callers and is what +//! lives here. +//! +//! **The two rename semantics this unifies to, uniformly (a real +//! improvement, not just deduplication).** Directory-onto-directory rename +//! on POSIX fails loudly (`ENOTEMPTY`) when the destination is non-empty; +//! file-onto-file rename SILENTLY CLOBBERS an existing destination instead +//! of failing. Before this module, `hydrate_dir` only remapped the loud +//! failure AFTER attempting the rename (no pre-check), and `hydrate_file` +//! only guarded with a pre-check (its danger case never manifests as a +//! rename error to remap). [`publish_by_rename`] does BOTH for both +//! callers: a pre-rename re-check (the file case's real defense, now also +//! narrowing the directory case's window further) and a post-rename remap +//! on failure (the directory case's real defense, harmless as a no-op +//! safety net for the file case). This strictly narrows both windows; it +//! weakens neither. +//! +//! Still a filesystem-atomicity boundary, deliberately NOT a coordination +//! protocol — no lock, no lease (doctrine `.claude/knowledge/ +//! s3-hydration-lifecycle.md` §4a). The residual race (a publish landing in +//! the instant between the pre-check and the OS rename call itself) is +//! narrowed, not closed, exactly as the doctrine's own framing describes it. + +use std::path::Path as FsPath; + +/// Which removal (and therefore which rename failure mode) applies to a +/// given publish attempt. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum StagingKind { + /// A staging directory (`hydrate_dir`): removed via `remove_dir_all`; + /// a rename onto a non-empty destination fails loudly (`ENOTEMPTY`). + Dir, + /// A staging file (`hydrate_file`): removed via `remove_file`; a + /// rename onto an existing destination SILENTLY CLOBBERS it on POSIX + /// rather than failing — the pre-rename re-check is this case's real + /// defense, not the post-rename remap. + File, +} + +/// Best-effort removal is the caller's choice, not this function's — some +/// callers must swallow a cleanup failure (the primary error already +/// dominates) and at least one must propagate it (an `Ok` return promising +/// "leaves nothing behind" must not be true only optimistically). This +/// returns the real `io::Result` so both are expressible: `let _ = +/// remove_staging(..).await;` to swallow, `remove_staging(..).await?;` to +/// propagate. +pub(crate) async fn remove_staging(staging: &FsPath, kind: StagingKind) -> std::io::Result<()> { + match kind { + StagingKind::Dir => tokio::fs::remove_dir_all(staging).await, + StagingKind::File => tokio::fs::remove_file(staging).await, + } +} + +/// Why [`publish_by_rename`] failed. Both callers map this onto their own +/// error enum's `AlreadyPublished(PathBuf)` / `Io(io::Error)` variants — +/// deliberately not a shared error type, since each caller's variant +/// already carries the path (this function doesn't need to duplicate it). +#[derive(Debug)] +pub(crate) enum PublishError { + /// `publish_path` was occupied — either at the pre-rename re-check, or + /// the rename itself failed in a way consistent with a concurrent + /// publisher having won the race (directory case: `ENOTEMPTY`; file + /// case: caught by the pre-check, so this arm is defense-in-depth + /// there). `staging` has already been removed (best-effort). + AlreadyPublished, + /// A genuine I/O error unrelated to the destination being occupied. + /// `staging` has already been removed (best-effort). + Io(std::io::Error), +} + +/// The shared publish tail: re-check `publish_path` immediately before the +/// rename, rename `staging` onto it, and on any failure clean up `staging` +/// and classify the failure per [`PublishError`]. See the module doc for +/// why both the pre-check and the post-rename remap run for both +/// [`StagingKind`]s. +pub(crate) async fn publish_by_rename( + staging: &FsPath, + publish_path: &FsPath, + kind: StagingKind, +) -> Result<(), PublishError> { + if publish_path.exists() { + let _ = remove_staging(staging, kind).await; + return Err(PublishError::AlreadyPublished); + } + if let Err(e) = tokio::fs::rename(staging, publish_path).await { + let _ = remove_staging(staging, kind).await; + return Err(if publish_path.exists() { + PublishError::AlreadyPublished + } else { + PublishError::Io(e) + }); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn publish_by_rename_publishes_a_directory() { + let local_tmp = tempfile::tempdir().expect("local tempdir"); + let staging = local_tmp.path().join(".staging-dir"); + std::fs::create_dir_all(&staging).expect("staging dir"); + std::fs::write(staging.join("data.lance"), b"row-bytes").expect("staged file"); + let publish_dir = local_tmp.path().join("hydrated.lance"); + + publish_by_rename(&staging, &publish_dir, StagingKind::Dir) + .await + .expect("publish"); + + assert_eq!( + std::fs::read(publish_dir.join("data.lance")).expect("published file"), + b"row-bytes" + ); + assert!(!staging.exists(), "staging dir must be gone after publish"); + } + + #[tokio::test] + async fn publish_by_rename_publishes_a_file() { + let local_tmp = tempfile::tempdir().expect("local tempdir"); + let staging = local_tmp.path().join("x.part"); + std::fs::write(&staging, b"payload").expect("staged file"); + let publish_path = local_tmp.path().join("x.soa"); + + publish_by_rename(&staging, &publish_path, StagingKind::File) + .await + .expect("publish"); + + assert_eq!(std::fs::read(&publish_path).expect("published file"), b"payload"); + assert!(!staging.exists(), "staging file must be gone after publish"); + } + + #[tokio::test] + async fn publish_by_rename_directory_race_is_reported_as_already_published_and_cleans_up() { + // Simulates the exact race the pre-check narrows: a competing + // publisher lands at `publish_dir` between this caller's fetch + // completing and its own publish attempt. + let local_tmp = tempfile::tempdir().expect("local tempdir"); + let staging = local_tmp.path().join(".staging-dir"); + std::fs::create_dir_all(&staging).expect("staging dir"); + std::fs::write(staging.join("data.lance"), b"loser").expect("staged file"); + let publish_dir = local_tmp.path().join("hydrated.lance"); + std::fs::create_dir_all(&publish_dir).expect("winner already published"); + std::fs::write(publish_dir.join("data.lance"), b"winner").expect("winner content"); + + let err = publish_by_rename(&staging, &publish_dir, StagingKind::Dir) + .await + .expect_err("must detect the race"); + assert!(matches!(err, PublishError::AlreadyPublished)); + assert!(!staging.exists(), "loser's staging dir must be cleaned up"); + assert_eq!( + std::fs::read(publish_dir.join("data.lance")).expect("winner survives"), + b"winner", + "the winner's published content must be untouched" + ); + } + + #[tokio::test] + async fn publish_by_rename_file_race_is_reported_as_already_published_and_cleans_up() { + let local_tmp = tempfile::tempdir().expect("local tempdir"); + let staging = local_tmp.path().join("x.part"); + std::fs::write(&staging, b"loser").expect("staged file"); + let publish_path = local_tmp.path().join("x.soa"); + std::fs::write(&publish_path, b"winner").expect("winner already published"); + + let err = publish_by_rename(&staging, &publish_path, StagingKind::File) + .await + .expect_err("must detect the race"); + assert!(matches!(err, PublishError::AlreadyPublished)); + assert!(!staging.exists(), "loser's staging file must be cleaned up"); + assert_eq!( + std::fs::read(&publish_path).expect("winner survives"), + b"winner", + "the winner's published content must be untouched" + ); + } +}