Skip to content

Commit 704e245

Browse files
RajivTSmeta-codesync[bot]
authored andcommitted
Add get_with_version for atomic content+version reads
Summary: Adds `ConfigHandle::get_with_version(&self) -> (Arc<T>, Option<ConfigVersionInfo>)`, an accessor that returns the config contents together with the version metadata (`version` string + `mod_time`) they were parsed from, read atomically from one snapshot. **The skew problem.** Today a `RegisteredConfigEntity`'s deserialized contents live in a tokio watch channel while `version`/`mod_time` live in a separate `RwLock<CachedConfigEntity>`. `refresh()` sends the new contents to the channel *before* committing the new version to the lock, so calling `get()` and then reading the version separately can pair new contents with the old version (or vice versa) for one refresh cycle. **The fix.** `CachedConfigEntity` now also carries the deserialized `Arc<T>`, committed in the same write-lock critical section as `version`/`mod_time`; `get_with_version` reads all three under a single read-lock acquisition, so the returned pair always corresponds. `get()` semantics and the watch-channel behavior are unchanged (the extra `Arc` in the lock is just another reference to the same allocation). **The Fixed contract.** Handles backed by static configs (`from_json`, `default`, `From<T>`) have no version, so the version half is `Option`-typed and they return `(contents, None)` — never a sentinel string. `ConfigVersionInfo` is `#[non_exhaustive]` so future fields (e.g. mutation id) can be added without breaking callers. Also adds `TestSource::insert_config_with_version` so tests can exercise non-empty versions (`insert_config` previously hardcoded `version: String::new()` and now delegates). **Consumer.** This is Diff A1 of the Task 32 stack (Mononoke: decouple startup from legacy tier blobs; design doc: configerator `source/scm/mononoke/docs/plans/2026-08-17-task32-blob-decouple-design-final.md`). Mononoke's per-repo config provenance (Diff B) will call this accessor at parse time and log the config version to scuba. Reviewed By: YousefSalama Differential Revision: D116321758 fbshipit-source-id: fab12e34e2f2da92da257d62b1a5fbf749685e9d
1 parent 8445bcd commit 704e245

5 files changed

Lines changed: 196 additions & 6 deletions

File tree

shed/cached_config/src/handle.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,24 @@ use serde::de::DeserializeOwned;
1717
use serde_json::from_str;
1818
use tokio::sync::watch::Receiver;
1919

20+
use crate::ModificationTime;
2021
use crate::refreshable_entities::RegisteredConfigEntity;
2122

23+
/// Versioning metadata for a config snapshot, as reported by the config
24+
/// source (e.g. configerator's Entity version). For configerator-backed
25+
/// sources the version uniquely identifies the config snapshot; other
26+
/// `Source` impls (e.g. `TestSource` without explicit versions) may reuse
27+
/// versions across changing contents — pair with `mod_time` when identity
28+
/// matters.
29+
#[derive(Clone, Debug, PartialEq, Eq)]
30+
#[non_exhaustive]
31+
pub struct ConfigVersionInfo {
32+
/// Version of the config as reported by the config source
33+
pub version: String,
34+
/// Modification time of the config, e.g. file modification time
35+
pub mod_time: ModificationTime,
36+
}
37+
2238
/// A configuration handle, with self-refresh and wait-on-update if obtained
2339
/// from a `ConfigStore`. If your type `T` implements `Default`, then this
2440
/// will implement `Default` using a fixed config matching `T`'s default
@@ -80,6 +96,22 @@ where
8096
}
8197
}
8298

99+
/// Fetch the current version of the config referred to by this handle
100+
/// together with its versioning metadata. Both halves of the returned
101+
/// tuple are read atomically from the same snapshot, so the version info
102+
/// always corresponds to the returned contents, even if the config is
103+
/// concurrently refreshed. Fixed (static) configs, e.g. obtained via
104+
/// `from_json` or `default`, have no version and return `None`.
105+
pub fn get_with_version(&self) -> (Arc<T>, Option<ConfigVersionInfo>) {
106+
match &self.inner {
107+
ConfigHandleImpl::Registered(handle) => {
108+
let (contents, version_info) = handle.get_with_version();
109+
(contents, Some(version_info))
110+
}
111+
ConfigHandleImpl::Fixed(contents) => (contents.clone(), None),
112+
}
113+
}
114+
83115
/// Method that returns a config update watcher that observes changes
84116
/// that are applied to the underlying config. Requesting a config watcher
85117
/// for static config (e.g. sourced via static JSON file) results in an error.

shed/cached_config/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ use bytes::Bytes;
3434
use chrono::NaiveDateTime;
3535
pub use handle::ConfigHandle;
3636
pub use handle::ConfigUpdateWatcher;
37+
pub use handle::ConfigVersionInfo;
3738
pub use store::ConfigStore;
3839
pub use test_source::TestSource;
3940

shed/cached_config/src/refreshable_entities.rs

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use tokio::sync::watch::channel;
2020

2121
use crate::Entity;
2222
use crate::ModificationTime;
23+
use crate::handle::ConfigVersionInfo;
2324

2425
// Type-erasure trick. I don't actually care about T for RegisteredConfigEntity,
2526
/// so hide it via a trait object
@@ -30,16 +31,21 @@ pub(crate) trait Refreshable {
3031

3132
/// The type contained in a `ConfigHandle` when it's obtained from a `ConfigStore`
3233
pub(crate) struct RegisteredConfigEntity<T> {
33-
contents: RwLock<CachedConfigEntity>,
34+
contents: RwLock<CachedConfigEntity<T>>,
3435
path: String,
3536
deserializer: fn(Bytes) -> Result<T>,
3637
update_sender: RwLock<Sender<Arc<T>>>,
3738
update_receiver: RwLock<Receiver<Arc<T>>>,
3839
}
3940

40-
struct CachedConfigEntity {
41+
/// A single config snapshot: the deserialized contents together with the
42+
/// version metadata they were parsed from. All fields are committed in the
43+
/// same write-lock critical section during `refresh`, so readers holding the
44+
/// read lock always observe contents and version that correspond.
45+
struct CachedConfigEntity<T> {
4146
mod_time: ModificationTime,
4247
version: String,
48+
contents: Arc<T>,
4349
}
4450

4551
impl<T> RegisteredConfigEntity<T>
@@ -57,10 +63,14 @@ where
5763
contents,
5864
} = entity;
5965
let contents = Arc::new(deserializer(contents.unwrap_or_else(Bytes::new))?);
60-
let (update_sender, update_receiver) = channel(contents);
66+
let (update_sender, update_receiver) = channel(contents.clone());
6167

6268
Ok(Self {
63-
contents: RwLock::new(CachedConfigEntity { mod_time, version }),
69+
contents: RwLock::new(CachedConfigEntity {
70+
mod_time,
71+
version,
72+
contents,
73+
}),
6474
path,
6575
deserializer,
6676
update_sender: RwLock::new(update_sender),
@@ -76,6 +86,21 @@ where
7686
.clone()
7787
}
7888

89+
/// Get the current contents together with the version metadata they were
90+
/// parsed from. Both are read under a single read-lock acquisition, so
91+
/// they are guaranteed to correspond to the same config snapshot even if
92+
/// a refresh is in flight.
93+
pub(crate) fn get_with_version(&self) -> (Arc<T>, ConfigVersionInfo) {
94+
let locked = self.contents.read().expect("lock poisoned");
95+
(
96+
locked.contents.clone(),
97+
ConfigVersionInfo {
98+
version: locked.version.clone(),
99+
mod_time: locked.mod_time.clone(),
100+
},
101+
)
102+
}
103+
79104
pub(crate) fn update_receiver(&self) -> Receiver<Arc<T>> {
80105
self.update_receiver.read().expect("lock poisoned").clone()
81106
}
@@ -98,7 +123,11 @@ where
98123
if has_changed {
99124
let contents = Arc::new((self.deserializer)(entity.contents.unwrap_or_default())?);
100125
let update_sender = self.update_sender.write().expect("lock poisoned");
101-
if update_sender.send(contents).is_err() {
126+
// Deliberate ordering: the watch channel is updated before the
127+
// snapshot lock below is committed, so watchers/get() can briefly
128+
// see newer contents than get_with_version(). Each accessor family
129+
// is self-consistent; do not "fix" the order.
130+
if update_sender.send(contents.clone()).is_err() {
102131
bail!(
103132
"No subscriber for config updates at path {}",
104133
self.get_path()
@@ -109,6 +138,7 @@ where
109138
*locked = CachedConfigEntity {
110139
mod_time: entity.mod_time,
111140
version: entity.version,
141+
contents,
112142
};
113143
Ok(true)
114144
}

shed/cached_config/src/test_source.rs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,25 @@ impl TestSource {
5555

5656
/// Insert config value into the `TestSource`, overwriting existing one
5757
pub fn insert_config(&self, key: &str, contents: &str, mod_time: ModificationTime) {
58+
self.insert_config_with_version(key, contents, mod_time, "");
59+
}
60+
61+
/// Insert config value with an explicit version into the `TestSource`,
62+
/// overwriting existing one
63+
pub fn insert_config_with_version(
64+
&self,
65+
key: &str,
66+
contents: &str,
67+
mod_time: ModificationTime,
68+
version: &str,
69+
) {
5870
let mut map = self.path_to_config.lock().expect("poisoned lock");
5971
map.insert(
6072
key.to_owned(),
6173
Entity {
6274
contents: Some(Bytes::copy_from_slice(contents.as_bytes())),
6375
mod_time,
64-
version: String::new(),
76+
version: version.to_owned(),
6577
},
6678
);
6779
}

shed/cached_config/src/tests.rs

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,121 @@ fn test_config_store() {
109109
assert_eq!(*raw_handle.get(), r#"{ "value": 11 }"#);
110110
}
111111

112+
#[test]
113+
fn test_get_with_version_registered() {
114+
let test_source = {
115+
let test_source = TestSource::new();
116+
test_source.insert_config_with_version(
117+
"some1",
118+
r#"{ "value": 1 }"#,
119+
ModificationTime::UnixTimestamp(1),
120+
"v1",
121+
);
122+
Arc::new(test_source)
123+
};
124+
125+
// No poll interval: refreshes happen only via force_update_configs, so the
126+
// test is deterministic.
127+
let store = ConfigStore::new(test_source.clone(), None::<Duration>, None);
128+
129+
let handle = get_test_handle(&store, "some1").expect("Failed to get handle");
130+
131+
let (contents, info) = handle.get_with_version();
132+
assert_eq!(
133+
*contents,
134+
TestConfig { value: 1 },
135+
"contents should match the initially inserted config"
136+
);
137+
let info = info.expect("registered handles should always report version info");
138+
assert_eq!(
139+
info.version, "v1",
140+
"version should match the inserted config's version"
141+
);
142+
assert_eq!(
143+
info.mod_time,
144+
ModificationTime::UnixTimestamp(1),
145+
"mod_time should match the inserted config's mod_time"
146+
);
147+
148+
// Update the config and refresh; contents and version from ONE call must
149+
// correspond to the same snapshot.
150+
test_source.insert_config_with_version(
151+
"some1",
152+
r#"{ "value": 2 }"#,
153+
ModificationTime::UnixTimestamp(2),
154+
"v2",
155+
);
156+
test_source.insert_to_refresh("some1".to_owned());
157+
store.force_update_configs();
158+
159+
let (contents, info) = handle.get_with_version();
160+
let info = info.expect("registered handles should always report version info");
161+
assert_eq!(
162+
*contents,
163+
TestConfig { value: 2 },
164+
"contents should reflect the refreshed config"
165+
);
166+
assert_eq!(
167+
info.version, "v2",
168+
"version must correspond to the contents returned by the same call"
169+
);
170+
assert_eq!(
171+
info.mod_time,
172+
ModificationTime::UnixTimestamp(2),
173+
"mod_time must correspond to the contents returned by the same call"
174+
);
175+
176+
// get() must be unaffected by the new accessor
177+
assert_eq!(
178+
*handle.get(),
179+
TestConfig { value: 2 },
180+
"get() should still return the refreshed config"
181+
);
182+
}
183+
184+
#[test]
185+
fn test_get_with_version_legacy_insert_config() {
186+
let test_source = {
187+
let test_source = TestSource::new();
188+
test_source.insert_config(
189+
"legacy",
190+
r#"{ "value": 3 }"#,
191+
ModificationTime::UnixTimestamp(1),
192+
);
193+
Arc::new(test_source)
194+
};
195+
let store = ConfigStore::new(test_source, None::<Duration>, None);
196+
197+
let handle = get_test_handle(&store, "legacy").expect("Failed to get handle");
198+
let (contents, info) = handle.get_with_version();
199+
assert_eq!(
200+
*contents,
201+
TestConfig { value: 3 },
202+
"contents should match the inserted config"
203+
);
204+
let info = info.expect("registered handles should always report version info");
205+
assert_eq!(
206+
info.version, "",
207+
"legacy insert_config must keep reporting an empty version for backward compatibility"
208+
);
209+
}
210+
211+
#[test]
212+
fn test_get_with_version_fixed() {
213+
let handle = ConfigHandle::<TestConfig>::from_json(r#"{ "value": 44 }"#)
214+
.expect("failed to deserialize json");
215+
let (contents, info) = handle.get_with_version();
216+
assert_eq!(
217+
*contents,
218+
TestConfig { value: 44 },
219+
"contents should match the fixed JSON config"
220+
);
221+
assert!(
222+
info.is_none(),
223+
"fixed (from_json/default) handles must return None version info, not a sentinel"
224+
);
225+
}
226+
112227
#[test]
113228
fn test_config_handle_from_json() {
114229
let result = ConfigHandle::<TestConfig>::from_json(r#"{ "value": 44 }"#)

0 commit comments

Comments
 (0)