From 2990c6f18238fc6be842cbae801823048d658e7c Mon Sep 17 00:00:00 2001 From: Takashi Yamashina Date: Fri, 14 Aug 2026 13:48:58 +0900 Subject: [PATCH 1/2] fix: document restore's ReplicationRecord asymmetry, consolidate lane-stem derivation, complete the unsupported-scheme error message Addresses #619's remaining items: - `taguru restore` never writes ReplicationRecord unlike hydrate's boot-time restore; documented the asymmetry and why on both sides. - Extracted `naming::lane_stem` as the single suffix-stripping authority, shared by `lane_metric_labels` and the replica tailer (previously duplicated inline in src/replica.rs). - `open_store`'s unsupported-scheme error now covers object_store's full scheme surface: the s3a/adl/abfs/abfss aliases, Azure's and S3's host-based https:// detection, and the deliberately-unsupported memory:// and generic https:// cases. Item 2 (legacy restore fallback removal) was already resolved via #638. Claude-Session: https://claude.ai/code/session_01KGdCCEPLGcimWQtAcAAXqZ --- src/replica.rs | 5 +---- src/ship.rs | 2 +- src/ship/config.rs | 9 ++++++++- src/ship/naming.rs | 40 +++++++++++++++++++++++++++++----------- src/ship/restore.rs | 13 +++++++++++++ 5 files changed, 52 insertions(+), 17 deletions(-) diff --git a/src/replica.rs b/src/replica.rs index 9d9de7b6..0544a395 100644 --- a/src/replica.rs +++ b/src/replica.rs @@ -510,10 +510,7 @@ impl Tailer { // behind. for (lane_name, lane) in &manifest.lanes { let (context, lane_label) = ship::lane_metric_labels(lane_name); - let stem = lane_name - .strip_suffix(".passages.wal.jsonl") - .or_else(|| lane_name.strip_suffix(".wal.jsonl")) - .unwrap_or(lane_name); + let stem = ship::lane_stem(lane_name); if failed.contains(stem) { self.state .metrics() diff --git a/src/ship.rs b/src/ship.rs index a5781ee2..c932ced8 100644 --- a/src/ship.rs +++ b/src/ship.rs @@ -153,7 +153,7 @@ use naming::{ pub(crate) use naming::{ HEARTBEAT_MARKER, Manifest, ManifestFile, ManifestLane, REPLICATION_RECORD, RETIRED_MARKER, ReplicationRecord, TAKEOVER_GRACE, complete_key, fence_key, gen_root, lane_metric_labels, - read_replication_record, segment_name, write_replication_record, + lane_stem, read_replication_record, segment_name, write_replication_record, }; pub(crate) use progress::{FileSig, ShipProgress}; use progress::{LaneState, ShippedFile}; diff --git a/src/ship/config.rs b/src/ship/config.rs index a10e329b..e38b7f11 100644 --- a/src/ship/config.rs +++ b/src/ship/config.rs @@ -153,7 +153,14 @@ pub(crate) fn open_store(url: &str) -> io::Result<(Arc, StorePa return Err(io::Error::new( io::ErrorKind::InvalidInput, format!( - "{url}: unsupported replication scheme — use s3://, gs://, az://, or file://" + "{url}: unsupported replication scheme — use s3://, gs://, az://, or \ + file:// (also accepted: s3a://, adl://, abfs://, abfss://, and — per \ + object_store's own host-based detection — https:// URLs shaped like a \ + cloud provider's native endpoint, e.g. \ + https://{{account}}.blob.core.windows.net/... for Azure or \ + https://s3.{{region}}.amazonaws.com/... for S3; object_store also \ + recognizes memory:// and other https:// hosts, but taguru does not \ + support shipping to them)" ), )); } diff --git a/src/ship/naming.rs b/src/ship/naming.rs index 267fa28a..d1891a2c 100644 --- a/src/ship/naming.rs +++ b/src/ship/naming.rs @@ -30,6 +30,12 @@ pub(crate) const TAKEOVER_GRACE: Duration = Duration::from_secs(300); /// it hydrated from (the cache-mode marker `crate::hydrate` keys on). /// Never shipped (see [`classify`]): it describes the local replica of /// the relationship, not the data. +/// +/// Only `crate::hydrate`'s boot-time restore writes this. The +/// `taguru restore` CLI (`restore::run`) never does — it materializes +/// an independent directory with no promise of ever booting against +/// this bucket again, so there is no cache relationship to record +/// (see `restore::run`'s own doc for the full reasoning). pub(crate) const REPLICATION_RECORD: &str = ".taguru.replication"; /// What [`REPLICATION_RECORD`] holds. @@ -218,21 +224,33 @@ pub(super) fn parent_snapshot_of(lane_name: &str) -> Option { .map(|stem| format!("{stem}.ctx")) } +/// The stem a lane's file name derives from — the passage suffix +/// checked first because both lanes end in `.wal.jsonl`, same as +/// [`parent_snapshot_of`]. Shared by [`lane_metric_labels`] (which +/// decodes the stem further into a display name) and the replica +/// tailer (which needs the raw stem itself, to key its per-poll +/// `failed` set against `ensure_context`'s own stem-keyed errors), so +/// the two can never drift on which suffixes count (issue #619). +pub(crate) fn lane_stem(lane_name: &str) -> &str { + lane_name + .strip_suffix(".passages.wal.jsonl") + .or_else(|| lane_name.strip_suffix(".wal.jsonl")) + .unwrap_or(lane_name) +} + /// The per-lane label pair the lag metric carries: the context's /// decoded name where the stem decodes (it always should — these files /// were written by the server), plus which lane. The replica's lag /// rows reuse it so the two vocabularies cannot drift. pub(crate) fn lane_metric_labels(lane_name: &str) -> (String, &'static str) { - if let Some(stem) = lane_name.strip_suffix(".passages.wal.jsonl") { - ( - crate::registry::name_from_stem(stem).unwrap_or_else(|| stem.to_string()), - "passages", - ) + let kind = if lane_name.ends_with(".passages.wal.jsonl") { + "passages" } else { - let stem = lane_name.strip_suffix(".wal.jsonl").unwrap_or(lane_name); - ( - crate::registry::name_from_stem(stem).unwrap_or_else(|| stem.to_string()), - "graph", - ) - } + "graph" + }; + let stem = lane_stem(lane_name); + ( + crate::registry::name_from_stem(stem).unwrap_or_else(|| stem.to_string()), + kind, + ) } diff --git a/src/ship/restore.rs b/src/ship/restore.rs index 6d46be7c..1f7aff72 100644 --- a/src/ship/restore.rs +++ b/src/ship/restore.rs @@ -32,6 +32,19 @@ mix two histories. Verify the result with: taguru inspect DIR /// — bad/missing credentials, a rejected cloud config, an /// inaccessible local path) · 2 usage error (a malformed URL, an /// unrecognized scheme, or a bad flag). +/// +/// Unlike `crate::hydrate`'s boot-time restore, this never writes +/// [`ReplicationRecord`] into `out`: `taguru restore` hands back an +/// independent directory the caller owns, with no promise the writer +/// will ever point at this bucket again, so there is no lineage for a +/// record to describe. `hydrate` writes one because it materializes +/// the SAME data directory `serve` is about to run against — the +/// record is how a later boot tells "this disk is a cache of the +/// bucket" from "this disk is independent truth" (see +/// [`ReplicationRecord`]'s own doc). A directory this CLI restores +/// stays plain local truth from the moment it lands; if the caller +/// wants it to behave as a cache instead, that is a `serve --replica` +/// boot against it, not this command. pub(crate) fn run(args: &[String]) -> i32 { let usage = |message: &str| crate::config::subcommand_usage_error("restore", message); let mut out: Option = None; From e2cda23d7bdb5d5407f76a5f70f5de8d0d1250b5 Mon Sep 17 00:00:00 2001 From: Takashi Yamashina Date: Fri, 14 Aug 2026 14:46:50 +0900 Subject: [PATCH 2/2] fix: correct the ReplicationRecord writer doc and complete the unsupported-scheme guidance CodeRabbit review on #641: - ReplicationRecord's doc said only hydrate's boot-time restore writes the record, but the replica tailer also writes it after a successful generation switch (src/replica.rs:537). Corrected. - open_store's unsupported-scheme message was missing the azure:// alias (object_store maps it to MicrosoftAzure same as az://) and didn't call out plain http:// as a recognized-but-unsupported scheme. Added both, plus a regression test covering memory://, http://, and non-cloud https:// rejections. Claude-Session: https://claude.ai/code/session_01KGdCCEPLGcimWQtAcAAXqZ --- src/ship/config.rs | 34 +++++++++++++++++++++++++++++----- src/ship/naming.rs | 6 ++++-- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/ship/config.rs b/src/ship/config.rs index e38b7f11..31bfce8d 100644 --- a/src/ship/config.rs +++ b/src/ship/config.rs @@ -154,13 +154,13 @@ pub(crate) fn open_store(url: &str) -> io::Result<(Arc, StorePa io::ErrorKind::InvalidInput, format!( "{url}: unsupported replication scheme — use s3://, gs://, az://, or \ - file:// (also accepted: s3a://, adl://, abfs://, abfss://, and — per \ - object_store's own host-based detection — https:// URLs shaped like a \ - cloud provider's native endpoint, e.g. \ + file:// (also accepted: s3a://, adl://, azure://, abfs://, abfss://, \ + and — per object_store's own host-based detection — https:// URLs \ + shaped like a cloud provider's native endpoint, e.g. \ https://{{account}}.blob.core.windows.net/... for Azure or \ https://s3.{{region}}.amazonaws.com/... for S3; object_store also \ - recognizes memory:// and other https:// hosts, but taguru does not \ - support shipping to them)" + recognizes memory://, plain http://, and other https:// hosts, but \ + taguru does not support shipping to them)" ), )); } @@ -179,6 +179,30 @@ mod tests { assert_eq!(error.kind(), io::ErrorKind::InvalidInput, "{error}"); } + /// `object_store` itself recognizes `memory://` and generic + /// `http(s)://` hosts (`ObjectStoreScheme::Memory`/`Http`) — these + /// are not merely unrecognized like `ftp://`, they are schemes + /// `open_store`'s `match` deliberately falls through on. The + /// rejection must still be `InvalidInput`, and the message must + /// name both that they exist and that they are unsupported here, + /// not just recite the three cloud schemes as if these were never + /// considered. + #[test] + fn a_recognized_but_unsupported_scheme_names_itself_in_the_message() { + for url in [ + "memory:///", + "http://example.com/path", + "https://example.com/path", + ] { + let error = open_store(url).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::InvalidInput, "{error}"); + let message = error.to_string(); + assert!(message.contains(url), "{message}"); + assert!(message.contains("memory://"), "{message}"); + assert!(message.contains("http://"), "{message}"); + } + } + /// A syntactically fine `file://` URL naming a directory that does /// not exist is also a usage mistake, not the store refusing to /// open: `NotFound`. diff --git a/src/ship/naming.rs b/src/ship/naming.rs index d1891a2c..8f3d81cb 100644 --- a/src/ship/naming.rs +++ b/src/ship/naming.rs @@ -31,8 +31,10 @@ pub(crate) const TAKEOVER_GRACE: Duration = Duration::from_secs(300); /// Never shipped (see [`classify`]): it describes the local replica of /// the relationship, not the data. /// -/// Only `crate::hydrate`'s boot-time restore writes this. The -/// `taguru restore` CLI (`restore::run`) never does — it materializes +/// Both `crate::hydrate`'s boot-time restore and the replica tailer +/// (`replica::Tailer::poll_once`, after a successful generation +/// switch) write this. The `taguru restore` CLI (`restore::run`) +/// never does — it materializes /// an independent directory with no promise of ever booting against /// this bucket again, so there is no cache relationship to record /// (see `restore::run`'s own doc for the full reasoning).