From 269417240402e2d717d51cce2f2c7fd8929018b4 Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:08:00 +1000 Subject: [PATCH 1/4] fix(services/cloudflare-kv): report the stored etag for listed directories The lister handed every directory entry a fresh build_tmp_path_of string as its etag. That string is random, so the same unchanged directory came back with a different etag on every list call, and never with the etag stat reads out of the very same record -- which is also the value stat compares if_match and if_none_match against. Taking an etag from a listing and feeding it to stat_with(path).if_match(etag) therefore failed with ConditionNotMatch every time. The stored value was already in hand. CfKvListKey carries the record's CfKvMetadata, and the file branch three lines below was already reading metadata.etag from it; only the directory branch threw it away. Both branches now report what the service stored. One directory entry has no record behind it: the placeholder the non-recursive branch synthesises from the keys underneath when the listed path is not itself a key. That one now reports no etag rather than a fabricated one. Those two lines were the only places in the repository that invented an etag for a directory. azfile's lister attaches dir.properties.etag, the value the server returned, and every other service leaves the field unset -- so both halves of this change follow existing practice. Etag generation is untouched: the writer and create_dir still mint the value with build_tmp_path_of, and because that value is persisted with the record it stays stable for the life of the entry. build_entry_for_item never used self, so it moves out of the impl and becomes directly testable. Three unit tests. Two fail against the previous expression -- one pins the stored value, the other only asserts that two list calls agree, so it holds whatever scheme the service picks for the etag. The file-branch test passes either way, so the first two are not trivially red. The synthesised placeholder is not unit-tested: reaching it needs a live lister and a page context, so that line rests on the argument above rather than on a test. --- core/services/cloudflare-kv/src/lister.rs | 120 ++++++++++++++++------ 1 file changed, 86 insertions(+), 34 deletions(-) diff --git a/core/services/cloudflare-kv/src/lister.rs b/core/services/cloudflare-kv/src/lister.rs index 87744db7043c..5078d385f9aa 100644 --- a/core/services/cloudflare-kv/src/lister.rs +++ b/core/services/cloudflare-kv/src/lister.rs @@ -50,6 +50,41 @@ fn relative_to_root(root: &str, name: &str) -> String { .to_string() } +/// Build the listed entry for a record the KV API returned. +/// +/// Both branches report the `etag` this service stored alongside the record. A directory used to +/// be handed a freshly generated `build_tmp_path_of` string instead: that value is random, so it +/// differed on every `list` call and never matched what `stat` reads back from the very same +/// record, which is also the value `stat` compares `if_match`/`if_none_match` against. +fn build_entry_for_item(item: &CfKvListKey, root: &str) -> Result { + let metadata = item.metadata.clone(); + let mut name = item.name.clone(); + + if metadata.is_dir && !name.ends_with('/') { + name += "/"; + } + + let mut name = relative_to_root(root, &name); + + // If it is the root directory, it needs to be processed as / + if name.is_empty() { + name = "/".to_string(); + } + + let entry_metadata = if name.ends_with('/') { + Metadata::new(EntryMode::DIR) + .with_etag(metadata.etag) + .with_content_length(0) + } else { + Metadata::new(EntryMode::FILE) + .with_etag(metadata.etag) + .with_content_length(metadata.content_length as u64) + .with_last_modified(metadata.last_modified.parse::()?) + }; + + Ok(oio::Entry::new(&name, entry_metadata)) +} + impl CloudflareKvLister { pub fn new( core: Arc, @@ -68,35 +103,6 @@ impl CloudflareKvLister { } } - fn build_entry_for_item(&self, item: &CfKvListKey, root: &str) -> Result { - let metadata = item.metadata.clone(); - let mut name = item.name.clone(); - - if metadata.is_dir && !name.ends_with('/') { - name += "/"; - } - - let mut name = relative_to_root(root, &name); - - // If it is the root directory, it needs to be processed as / - if name.is_empty() { - name = "/".to_string(); - } - - let entry_metadata = if name.ends_with('/') { - Metadata::new(EntryMode::DIR) - .with_etag(build_tmp_path_of(&name)) - .with_content_length(0) - } else { - Metadata::new(EntryMode::FILE) - .with_etag(metadata.etag) - .with_content_length(metadata.content_length as u64) - .with_last_modified(metadata.last_modified.parse::()?) - }; - - Ok(oio::Entry::new(&name, entry_metadata)) - } - fn handle_non_recursive_file_list( &self, ctx: &mut oio::PageContext, @@ -104,15 +110,15 @@ impl CloudflareKvLister { root: &str, ) -> Result<()> { if let Some(item) = result.iter().find(|item| item.name == self.path) { - let entry = self.build_entry_for_item(item, root)?; + let entry = build_entry_for_item(item, root)?; ctx.entries.push_back(entry); } else if !result.is_empty() { let path_name = relative_to_root(root, &self.path); let entry = oio::Entry::new( &format!("{path_name}/"), - Metadata::new(EntryMode::DIR) - .with_etag(build_tmp_path_of(&path_name)) - .with_content_length(0), + // This directory is inferred from the keys under it rather than read back from a + // record of its own, so there is no stored etag to report for it. + Metadata::new(EntryMode::DIR).with_content_length(0), ); ctx.entries.push_back(entry); } @@ -179,7 +185,7 @@ impl oio::PageList for CloudflareKvLister { } } - let entry = self.build_entry_for_item(&item, &root)?; + let entry = build_entry_for_item(&item, &root)?; ctx.entries.push_back(entry); } } @@ -191,6 +197,52 @@ impl oio::PageList for CloudflareKvLister { #[cfg(test)] mod tests { use super::*; + use crate::model::CfKvMetadata; + + fn list_key(name: &str, etag: &str, is_dir: bool) -> CfKvListKey { + CfKvListKey { + name: name.to_string(), + metadata: CfKvMetadata { + etag: etag.to_string(), + last_modified: "2024-01-01T00:00:00Z".to_string(), + content_length: 7, + is_dir, + }, + } + } + + #[test] + fn dir_entry_reports_the_stored_etag() { + let item = list_key("data/sub/", "dir-etag-1", true); + + let entry = build_entry_for_item(&item, "/data/").expect("entry"); + + assert_eq!(entry.path(), "sub/"); + assert_eq!(entry.metadata().etag(), Some("dir-etag-1")); + } + + #[test] + fn dir_entry_etag_is_the_same_on_every_list_call() { + // The generated value was random, so an unchanged directory reported a different etag + // each time it was listed -- and never the one `stat` reads back from the same record. + let item = list_key("data/sub/", "dir-etag-1", true); + + let first = build_entry_for_item(&item, "/data/").expect("first"); + let second = build_entry_for_item(&item, "/data/").expect("second"); + + assert_eq!(first.metadata().etag(), second.metadata().etag()); + } + + #[test] + fn file_entry_reports_the_stored_etag() { + let item = list_key("data/file.txt", "file-etag-1", false); + + let entry = build_entry_for_item(&item, "/data/").expect("entry"); + + assert_eq!(entry.path(), "file.txt"); + assert_eq!(entry.metadata().etag(), Some("file-etag-1")); + assert_eq!(entry.metadata().content_length(), 7); + } #[test] fn relative_to_root_strips_only_the_prefix() { From 29c77fd7a5d14f1e154efaddb7cd657faf66081b Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:52:34 +1000 Subject: [PATCH 2/4] Keep build_entry_for_item a method and drop the added tests Matching the shape asked for on #8068, #8069 and #8071 rather than waiting to be asked again. The function goes back to being a method as it was, and the three tests added with it go. The change is now the two etag lines: a listed directory reports the etag stored with its record, and the synthesised directory placeholder reports none. The relative_to_root tests already in this file are from #8048 and are untouched. --- core/services/cloudflare-kv/src/lister.rs | 114 ++++++---------------- 1 file changed, 31 insertions(+), 83 deletions(-) diff --git a/core/services/cloudflare-kv/src/lister.rs b/core/services/cloudflare-kv/src/lister.rs index 5078d385f9aa..c16a0b913123 100644 --- a/core/services/cloudflare-kv/src/lister.rs +++ b/core/services/cloudflare-kv/src/lister.rs @@ -50,41 +50,6 @@ fn relative_to_root(root: &str, name: &str) -> String { .to_string() } -/// Build the listed entry for a record the KV API returned. -/// -/// Both branches report the `etag` this service stored alongside the record. A directory used to -/// be handed a freshly generated `build_tmp_path_of` string instead: that value is random, so it -/// differed on every `list` call and never matched what `stat` reads back from the very same -/// record, which is also the value `stat` compares `if_match`/`if_none_match` against. -fn build_entry_for_item(item: &CfKvListKey, root: &str) -> Result { - let metadata = item.metadata.clone(); - let mut name = item.name.clone(); - - if metadata.is_dir && !name.ends_with('/') { - name += "/"; - } - - let mut name = relative_to_root(root, &name); - - // If it is the root directory, it needs to be processed as / - if name.is_empty() { - name = "/".to_string(); - } - - let entry_metadata = if name.ends_with('/') { - Metadata::new(EntryMode::DIR) - .with_etag(metadata.etag) - .with_content_length(0) - } else { - Metadata::new(EntryMode::FILE) - .with_etag(metadata.etag) - .with_content_length(metadata.content_length as u64) - .with_last_modified(metadata.last_modified.parse::()?) - }; - - Ok(oio::Entry::new(&name, entry_metadata)) -} - impl CloudflareKvLister { pub fn new( core: Arc, @@ -103,6 +68,35 @@ impl CloudflareKvLister { } } + fn build_entry_for_item(&self, item: &CfKvListKey, root: &str) -> Result { + let metadata = item.metadata.clone(); + let mut name = item.name.clone(); + + if metadata.is_dir && !name.ends_with('/') { + name += "/"; + } + + let mut name = relative_to_root(root, &name); + + // If it is the root directory, it needs to be processed as / + if name.is_empty() { + name = "/".to_string(); + } + + let entry_metadata = if name.ends_with('/') { + Metadata::new(EntryMode::DIR) + .with_etag(metadata.etag) + .with_content_length(0) + } else { + Metadata::new(EntryMode::FILE) + .with_etag(metadata.etag) + .with_content_length(metadata.content_length as u64) + .with_last_modified(metadata.last_modified.parse::()?) + }; + + Ok(oio::Entry::new(&name, entry_metadata)) + } + fn handle_non_recursive_file_list( &self, ctx: &mut oio::PageContext, @@ -110,7 +104,7 @@ impl CloudflareKvLister { root: &str, ) -> Result<()> { if let Some(item) = result.iter().find(|item| item.name == self.path) { - let entry = build_entry_for_item(item, root)?; + let entry = self.build_entry_for_item(item, root)?; ctx.entries.push_back(entry); } else if !result.is_empty() { let path_name = relative_to_root(root, &self.path); @@ -185,7 +179,7 @@ impl oio::PageList for CloudflareKvLister { } } - let entry = build_entry_for_item(&item, &root)?; + let entry = self.build_entry_for_item(&item, &root)?; ctx.entries.push_back(entry); } } @@ -197,52 +191,6 @@ impl oio::PageList for CloudflareKvLister { #[cfg(test)] mod tests { use super::*; - use crate::model::CfKvMetadata; - - fn list_key(name: &str, etag: &str, is_dir: bool) -> CfKvListKey { - CfKvListKey { - name: name.to_string(), - metadata: CfKvMetadata { - etag: etag.to_string(), - last_modified: "2024-01-01T00:00:00Z".to_string(), - content_length: 7, - is_dir, - }, - } - } - - #[test] - fn dir_entry_reports_the_stored_etag() { - let item = list_key("data/sub/", "dir-etag-1", true); - - let entry = build_entry_for_item(&item, "/data/").expect("entry"); - - assert_eq!(entry.path(), "sub/"); - assert_eq!(entry.metadata().etag(), Some("dir-etag-1")); - } - - #[test] - fn dir_entry_etag_is_the_same_on_every_list_call() { - // The generated value was random, so an unchanged directory reported a different etag - // each time it was listed -- and never the one `stat` reads back from the same record. - let item = list_key("data/sub/", "dir-etag-1", true); - - let first = build_entry_for_item(&item, "/data/").expect("first"); - let second = build_entry_for_item(&item, "/data/").expect("second"); - - assert_eq!(first.metadata().etag(), second.metadata().etag()); - } - - #[test] - fn file_entry_reports_the_stored_etag() { - let item = list_key("data/file.txt", "file-etag-1", false); - - let entry = build_entry_for_item(&item, "/data/").expect("entry"); - - assert_eq!(entry.path(), "file.txt"); - assert_eq!(entry.metadata().etag(), Some("file-etag-1")); - assert_eq!(entry.metadata().content_length(), 7); - } #[test] fn relative_to_root_strips_only_the_prefix() { From 6edf1ec7f6eb29c9ab8eb1ea7627ab0ba0ef99ad Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:36:24 +1000 Subject: [PATCH 3/4] Add a wire-response test for the listed directory etag Review request from @erickguan. The test feeds a Cloudflare `list keys` response through CfKvListResponse and asserts the entries built from it, so the deserialization is exercised rather than a hand-built struct. It pins the thing this change actually depends on: the metadata blob on a directory key carries the same CfKvMetadata a file's does, etag included, because this service wrote it through `set`. Both the directory and the file entry are asserted. Restoring the old expression fails this test and nothing else. --- core/services/cloudflare-kv/src/lister.rs | 63 +++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/core/services/cloudflare-kv/src/lister.rs b/core/services/cloudflare-kv/src/lister.rs index c16a0b913123..9726514f7bbf 100644 --- a/core/services/cloudflare-kv/src/lister.rs +++ b/core/services/cloudflare-kv/src/lister.rs @@ -192,6 +192,69 @@ impl oio::PageList for CloudflareKvLister { mod tests { use super::*; + #[test] + fn a_listed_directory_reports_the_etag_the_wire_response_carries() { + // A `list keys` response as the Cloudflare API returns it. The metadata blob is what + // this service itself wrote through `set`, so a directory key carries the same + // CfKvMetadata a file does -- including the etag `stat` later compares if_match against. + let body = r#"{ + "errors": [], + "messages": [], + "success": true, + "result": [ + { + "name": "data/sub/", + "metadata": { + "etag": "sub/.AvaaBbxz", + "last_modified": "2024-01-01T00:00:00Z", + "content_length": 0, + "is_dir": true + } + }, + { + "name": "data/a.txt", + "metadata": { + "etag": "a.txt.xHzwzn53", + "last_modified": "2024-01-01T00:00:00Z", + "content_length": 7, + "is_dir": false + } + } + ], + "result_info": { "cursor": "" } + }"#; + + let resp: CfKvListResponse = serde_json::from_str(body).expect("response must parse"); + let items = resp.result.expect("result must be present"); + + let lister = CloudflareKvLister::new( + Arc::new(CloudflareKvCore { + api_token: "token".to_string(), + account_id: "account".to_string(), + namespace_id: "namespace".to_string(), + expiration_ttl: None, + info: ServiceInfo::new(crate::CLOUDFLARE_KV_SCHEME, "/data/", "namespace"), + capability: Capability::default(), + }), + OperationContext::default(), + "/", + false, + None, + ); + + let dir = lister + .build_entry_for_item(&items[0], "/data/") + .expect("directory entry"); + assert_eq!(dir.path(), "sub/"); + assert_eq!(dir.metadata().etag(), Some("sub/.AvaaBbxz")); + + let file = lister + .build_entry_for_item(&items[1], "/data/") + .expect("file entry"); + assert_eq!(file.path(), "a.txt"); + assert_eq!(file.metadata().etag(), Some("a.txt.xHzwzn53")); + } + #[test] fn relative_to_root_strips_only_the_prefix() { // The root repeated as an inner segment must survive; only the leading copy goes. From 5ad0aba7664d5527ac21f244b1de88cadfc6c665 Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:50:12 +1000 Subject: [PATCH 4/4] Say plainly that the fixture is reconstructed, not captured --- core/services/cloudflare-kv/src/lister.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/services/cloudflare-kv/src/lister.rs b/core/services/cloudflare-kv/src/lister.rs index 9726514f7bbf..0dab292ff586 100644 --- a/core/services/cloudflare-kv/src/lister.rs +++ b/core/services/cloudflare-kv/src/lister.rs @@ -194,9 +194,11 @@ mod tests { #[test] fn a_listed_directory_reports_the_etag_the_wire_response_carries() { - // A `list keys` response as the Cloudflare API returns it. The metadata blob is what - // this service itself wrote through `set`, so a directory key carries the same - // CfKvMetadata a file does -- including the etag `stat` later compares if_match against. + // Reconstructed, not captured: the envelope is the subset of the `list keys` response + // that CfKvListResponse reads, and the metadata blob is CfKvMetadata as serde emits it, + // since `set` sends `serde_json::to_string(&metadata)` as the metadata form part. The + // etags are shaped the way `build_tmp_path_of` mints them at write time. The point it + // pins is that a directory key carries the same CfKvMetadata a file does. let body = r#"{ "errors": [], "messages": [],