Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions crates/superposition_provider/src/data_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,18 @@ pub trait SuperpositionDataSource: Send + Sync {
.await
}

/// Fetch the full resolved configuration only if the source changed.
///
/// The default implementation delegates to `fetch_config`; data sources
/// with cheaper change detection can override this without changing normal
/// refresh behavior.
async fn fetch_config_if_modified(
&self,
if_modified_since: Option<DateTime<Utc>>,
) -> Result<FetchResponse<ConfigData>> {
self.fetch_config(if_modified_since).await
}

/// Fetch a resolved configuration filtered by the given context and key prefixes.
async fn fetch_filtered_config(
&self,
Expand Down
198 changes: 188 additions & 10 deletions crates/superposition_provider/src/data_source/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,36 @@ impl FileDataSource {
watcher: Mutex::new(None),
})
}
}

#[async_trait]
impl SuperpositionDataSource for FileDataSource {
async fn fetch_filtered_config(
async fn last_modified_at(&self) -> Result<DateTime<Utc>> {
let metadata = tokio::fs::metadata(&self.file_path).await.map_err(|e| {
SuperpositionError::ConfigError(format!(
"Failed to read metadata for config file {:?}: {}",
self.file_path, e
))
})?;

metadata.modified().map(DateTime::<Utc>::from).map_err(|e| {
SuperpositionError::ConfigError(format!(
"Failed to read modified time for config file {:?}: {}",
self.file_path, e
))
})
}

fn is_not_modified(
last_modified_at: DateTime<Utc>,
if_modified_since: Option<DateTime<Utc>>,
) -> bool {
if_modified_since.is_some_and(|modified_since| last_modified_at <= modified_since)
}

async fn read_config(
&self,
context: Option<Map<String, Value>>,
prefix_filter: Option<Vec<String>>,
if_modified_since: Option<DateTime<Utc>>,
fetched_at: DateTime<Utc>,
) -> Result<FetchResponse<ConfigData>> {
if if_modified_since.is_some() {
log::debug!("FileDataSource: ignoring if_modified_since, always reading fresh from file");
}
let now = Utc::now();
let content = tokio::fs::read_to_string(&self.file_path)
.await
.map_err(|e| {
Expand Down Expand Up @@ -89,9 +105,38 @@ impl SuperpositionDataSource for FileDataSource {

Ok(FetchResponse::Data(ConfigData {
data: config,
fetched_at: now,
fetched_at,
}))
}
}

#[async_trait]
impl SuperpositionDataSource for FileDataSource {
async fn fetch_filtered_config(
&self,
context: Option<Map<String, Value>>,
prefix_filter: Option<Vec<String>>,
if_modified_since: Option<DateTime<Utc>>,
) -> Result<FetchResponse<ConfigData>> {
if if_modified_since.is_some() {
log::debug!("FileDataSource: ignoring if_modified_since, always reading fresh from file");
}

self.read_config(context, prefix_filter, Utc::now()).await
}
Comment on lines +115 to +126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Timestamp inconsistency: fetch_filtered_config uses Utc::now() while fetch_config_if_modified uses file mtime.

Lines 125 and 138 assign different timestamps for the same logical fetch event. If a caller mixes fetch_config (which delegates to fetch_filtered_config) and fetch_config_if_modified, the fetched_at values won't be comparable, breaking change detection.

Scenario:

  1. refresh()fetch_configfetched_at = Utc::now() = T₁
  2. File is modified before T₁ (e.g., at T₀ < T₁), so file mtime = T₀
  3. refresh_if_changed(Some(T₁)) → checks if T₀ ≤ T₁ → returns NotModified (incorrect, file has changed content)

Solution: Both methods should use the same timestamp source. Since fetch_config_if_modified relies on file mtime for change detection, fetch_filtered_config should also use last_modified_at() as the fetched_at value.

🔧 Proposed fix
 async fn fetch_filtered_config(
     &self,
     context: Option<Map<String, Value>>,
     prefix_filter: Option<Vec<String>>,
     if_modified_since: Option<DateTime<Utc>>,
 ) -> Result<FetchResponse<ConfigData>> {
     if if_modified_since.is_some() {
         log::debug!("FileDataSource: ignoring if_modified_since, always reading fresh from file");
     }
 
-    self.read_config(context, prefix_filter, Utc::now()).await
+    let fetched_at = self.last_modified_at().await?;
+    self.read_config(context, prefix_filter, fetched_at).await
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async fn fetch_filtered_config(
&self,
context: Option<Map<String, Value>>,
prefix_filter: Option<Vec<String>>,
if_modified_since: Option<DateTime<Utc>>,
) -> Result<FetchResponse<ConfigData>> {
if if_modified_since.is_some() {
log::debug!("FileDataSource: ignoring if_modified_since, always reading fresh from file");
}
self.read_config(context, prefix_filter, Utc::now()).await
}
async fn fetch_filtered_config(
&self,
context: Option<Map<String, Value>>,
prefix_filter: Option<Vec<String>>,
if_modified_since: Option<DateTime<Utc>>,
) -> Result<FetchResponse<ConfigData>> {
if if_modified_since.is_some() {
log::debug!("FileDataSource: ignoring if_modified_since, always reading fresh from file");
}
let fetched_at = self.last_modified_at().await?;
self.read_config(context, prefix_filter, fetched_at).await
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/superposition_provider/src/data_source/file.rs` around lines 115 -
126, The fetch_filtered_config method uses Utc::now() as the timestamp passed to
read_config, but fetch_config_if_modified uses the file's actual mtime, causing
inconsistent fetched_at values. Replace the Utc::now() call in the read_config
invocation within fetch_filtered_config with a call to last_modified_at() to
ensure both methods use the same timestamp source (the file's actual
modification time) for consistent change detection across callers that mix these
two fetch methods.


async fn fetch_config_if_modified(
&self,
if_modified_since: Option<DateTime<Utc>>,
) -> Result<FetchResponse<ConfigData>> {
let last_modified_at = self.last_modified_at().await?;
if Self::is_not_modified(last_modified_at, if_modified_since) {
log::debug!("FileDataSource: config file not modified");
return Ok(FetchResponse::NotModified);
}

self.read_config(None, None, last_modified_at).await
}

async fn fetch_active_experiments(
&self,
Expand Down Expand Up @@ -197,3 +242,136 @@ impl SuperpositionDataSource for FileDataSource {
Ok(())
}
}

#[cfg(test)]
mod tests {
use std::path::{Path, PathBuf};

use tokio::time::{sleep, Duration};

use super::*;

fn config_content(timeout: i64) -> String {
format!(
r#"[default-configs]
timeout = {{ value = {timeout}, schema = {{ type = "integer" }} }}

[dimensions]
os = {{ position = 1, schema = {{ type = "string" }} }}

[[overrides]]
_context_ = {{ os = "linux" }}
timeout = 45
"#
)
}

fn temp_config_path() -> PathBuf {
std::env::temp_dir().join(format!(
"superposition-provider-file-source-{}.toml",
uuid::Uuid::new_v4()
))
}

async fn write_config(path: &Path, timeout: i64) {
tokio::fs::write(path, config_content(timeout))
.await
.unwrap();
}

async fn rewrite_config_after(
source: &FileDataSource,
path: &Path,
timeout: i64,
previous_modified_at: DateTime<Utc>,
) {
for _ in 0..100 {
sleep(Duration::from_millis(20)).await;
write_config(path, timeout).await;

if source.last_modified_at().await.unwrap() > previous_modified_at {
return;
}
}

panic!("config file modified time did not advance");
}

#[tokio::test]
async fn fetch_config_keeps_reading_fresh_when_if_modified_since_is_present() {
let path = temp_config_path();
write_config(&path, 30).await;

let source = FileDataSource::new(path.clone()).unwrap();
let first_fetch = source.fetch_config(None).await.unwrap();
let fetched_at = match first_fetch {
FetchResponse::Data(data) => {
assert_eq!(data.data.default_configs.get("timeout"), Some(&30.into()));
data.fetched_at
}
FetchResponse::NotModified => panic!("initial fetch should return data"),
};

let second_fetch = source.fetch_config(Some(fetched_at)).await.unwrap();
match second_fetch {
FetchResponse::Data(data) => {
assert_eq!(data.data.default_configs.get("timeout"), Some(&30.into()));
}
FetchResponse::NotModified => {
panic!("normal fetch should keep reading fresh")
}
}

let _ = tokio::fs::remove_file(path).await;
}

#[tokio::test]
async fn fetch_config_if_modified_returns_not_modified_when_file_has_not_changed() {
let path = temp_config_path();
write_config(&path, 30).await;

let source = FileDataSource::new(path.clone()).unwrap();
let first_fetch = source.fetch_config(None).await.unwrap();
let fetched_at = match first_fetch {
FetchResponse::Data(data) => data.fetched_at,
FetchResponse::NotModified => panic!("initial fetch should return data"),
};

let second_fetch = source
.fetch_config_if_modified(Some(fetched_at))
.await
.unwrap();
assert!(second_fetch.is_not_modified());

let _ = tokio::fs::remove_file(path).await;
}

#[tokio::test]
async fn fetch_config_if_modified_returns_data_after_file_changes() {
let path = temp_config_path();
write_config(&path, 30).await;

let source = FileDataSource::new(path.clone()).unwrap();
let first_fetch = source.fetch_config(None).await.unwrap();
let fetched_at = match first_fetch {
FetchResponse::Data(data) => data.fetched_at,
FetchResponse::NotModified => panic!("initial fetch should return data"),
};

rewrite_config_after(&source, &path, 60, fetched_at).await;

let changed_fetch = source
.fetch_config_if_modified(Some(fetched_at))
.await
.unwrap();
match changed_fetch {
FetchResponse::Data(data) => {
assert_eq!(data.data.default_configs.get("timeout"), Some(&60.into()));
assert!(data.fetched_at > fetched_at);
}
FetchResponse::NotModified => panic!("changed file should return data"),
}

let _ = tokio::fs::remove_file(path).await;
}
}
Loading
Loading