Handle no-record cars and per-copy trash listing
Empty API responses (200 + empty body) mean "known car, no times
submitted yet" on the records site — typically newly released prize
cars. Previously each such car burned 3 retries with backoff and then
aborted the entire fetch; 27 cars in the current DB hit this.
- Add AppError::EmptyResponse { url } as a distinct, deterministic
failure class; get_file fails immediately instead of retrying
- fetch_car downgrades EmptyResponse to a skip with an info log, so
fetch completes; the next fetch picks the cars up automatically
once records exist
- parse_car_times treats empty (or whitespace-only) timing files
like missing files, eliminating "EOF while parsing a value at line
1 column 0" parse errors from stale empty files
trash.csv Config column now lists only the copies to delete: for a
car whose one tune is a final build not best on any track (e.g.
owned 332 next to a best-on-213-tracks 323), only the dead tune is
shown instead of joining all owned configs. Categories 1 and 3 still
list all configs since the whole car goes.
Tests: empty response makes exactly 1 request and yields the
EmptyResponse variant; empty file parses Ok; partial-copy trash
shows only the deletable config.
This commit is contained in:
parent
293a8d7707
commit
7ac889cbff
6 changed files with 148 additions and 45 deletions
|
|
@ -32,6 +32,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||||
### Removed
|
### Removed
|
||||||
- `plans/code-review-v2.md` (completed action items)
|
- `plans/code-review-v2.md` (completed action items)
|
||||||
|
|
||||||
|
### Fixed
|
||||||
|
- `trash.csv` Config column now lists only the copies to delete: for cars where just one tune is useless (e.g. owned 332 + best-everywhere 323) only the dead tune is shown instead of all owned configs
|
||||||
|
- Cars the records site knows but has no times for (typically new prize cars, returned as `200` with an empty body) no longer abort `fetch` after 3 retries — they are skipped with an info log
|
||||||
|
- Empty timing files are now treated like missing files instead of producing `EOF while parsing a value` parse errors
|
||||||
|
|
||||||
## [0.3.0] - 2026-05-07
|
## [0.3.0] - 2026-05-07
|
||||||
|
|
||||||
### Added
|
### Added
|
||||||
|
|
|
||||||
24
src/http.rs
24
src/http.rs
|
|
@ -21,10 +21,13 @@ fn retry_delay(attempt: usize) -> std::time::Duration {
|
||||||
/// Writes to a temporary file first, then renames on success, so a failed
|
/// Writes to a temporary file first, then renames on success, so a failed
|
||||||
/// download never deletes the existing file.
|
/// download never deletes the existing file.
|
||||||
///
|
///
|
||||||
/// Retries up to `MAX_FETCH_RETRIES` times on transient failures (server errors,
|
/// Retries up to `MAX_FETCH_RETRIES` times on transient failures (server errors
|
||||||
/// connection errors, or empty responses). Permanent HTTP errors (4xx) fail immediately.
|
/// or connection errors). Permanent conditions fail immediately: client HTTP
|
||||||
|
/// errors (4xx) and empty bodies ([`AppError::EmptyResponse`] — the records API
|
||||||
|
/// reports "no data submitted yet" as a 200 with an empty body, which no amount
|
||||||
|
/// of retrying can change).
|
||||||
pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppError> {
|
pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppError> {
|
||||||
/// Maximum number of retries when the server returns an empty response.
|
/// Maximum number of retries on transient failures (5xx, connection errors).
|
||||||
const MAX_FETCH_RETRIES: usize = 3;
|
const MAX_FETCH_RETRIES: usize = 3;
|
||||||
|
|
||||||
// Parse the URL once so malformed URLs are caught before any network I/O.
|
// Parse the URL once so malformed URLs are caught before any network I/O.
|
||||||
|
|
@ -58,18 +61,9 @@ pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppEr
|
||||||
match response.text().await {
|
match response.text().await {
|
||||||
Ok(content) => {
|
Ok(content) => {
|
||||||
if content.is_empty() {
|
if content.is_empty() {
|
||||||
if attempt < MAX_FETCH_RETRIES {
|
// "Known resource, no data yet" — deterministic, so
|
||||||
log::warn!(
|
// fail immediately instead of burning retries.
|
||||||
"empty response from {file_url} (attempt {}/{MAX_FETCH_RETRIES}), retrying...",
|
return Err(AppError::EmptyResponse { url: file_url.to_string() });
|
||||||
attempt + 1,
|
|
||||||
);
|
|
||||||
tokio::time::sleep(retry_delay(attempt)).await;
|
|
||||||
attempt += 1;
|
|
||||||
continue;
|
|
||||||
}
|
|
||||||
return Err(AppError::Network(format!(
|
|
||||||
"Empty response from {file_url} after {MAX_FETCH_RETRIES} retries"
|
|
||||||
)));
|
|
||||||
}
|
}
|
||||||
fs::write(&tmp_path, &content).map_err(AppError::Io)?;
|
fs::write(&tmp_path, &content).map_err(AppError::Io)?;
|
||||||
return fs::rename(&tmp_path, local_file_path).map_err(AppError::Io);
|
return fs::rename(&tmp_path, local_file_path).map_err(AppError::Io);
|
||||||
|
|
|
||||||
|
|
@ -359,6 +359,11 @@ pub fn plan_trash_rules(
|
||||||
/// categories 1 and 2 because they belong in `improve.csv` instead (unless
|
/// categories 1 and 2 because they belong in `improve.csv` instead (unless
|
||||||
/// caught by category 3). Prize cars and RQ ≥ 80 are always excluded.
|
/// caught by category 3). Prize cars and RQ ≥ 80 are always excluded.
|
||||||
///
|
///
|
||||||
|
/// The Config column lists only the copies to delete: for category 2 just the
|
||||||
|
/// final builds that are not best on any track (a best-on-213-tracks 323 next
|
||||||
|
/// to a useless 332 shows only `332`); for categories 1 and 3 all owned
|
||||||
|
/// configs (the whole car goes).
|
||||||
|
///
|
||||||
/// `improve` holds the precomputed improve entries (see
|
/// `improve` holds the precomputed improve entries (see
|
||||||
/// [`collect_improve_entries`]) so callers can share one computation with
|
/// [`collect_improve_entries`]) so callers can share one computation with
|
||||||
/// [`plan_trash_rules`].
|
/// [`plan_trash_rules`].
|
||||||
|
|
@ -393,31 +398,41 @@ pub fn write_trash_csv(
|
||||||
|
|
||||||
let trash: Vec<TrashEntry> = owned
|
let trash: Vec<TrashEntry> = owned
|
||||||
.iter()
|
.iter()
|
||||||
.filter(|guid| {
|
|
||||||
if low_value_guids.contains(guid.as_str()) {
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
let Some(best_configs) = counts.guid_to_rid.get(*guid) else {
|
|
||||||
return true;
|
|
||||||
};
|
|
||||||
if let Some(configs) = counts.owned_configs.get(*guid) {
|
|
||||||
// Any copy with a final build not matching any best config → trash
|
|
||||||
if configs.iter().any(|actual_config| {
|
|
||||||
is_final_build(actual_config)
|
|
||||||
&& !best_configs.iter().any(|(_, nc)| nc == actual_config)
|
|
||||||
}) {
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
false
|
|
||||||
})
|
|
||||||
.filter_map(|guid| {
|
.filter_map(|guid| {
|
||||||
let car = counts.car_map.get(guid)?;
|
let car = counts.car_map.get(guid)?;
|
||||||
if car.prize || car.rq >= TRASH_MIN_RQ {
|
if car.prize || car.rq >= TRASH_MIN_RQ {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
let config =
|
// Categories 1 (not best anywhere) and 3 (low-value fodder) trash
|
||||||
counts.owned_configs.get(guid).map(|configs| configs.join("/")).unwrap_or_default();
|
// the whole car — every owned copy goes. Category 2 trashes only
|
||||||
|
// the copies whose final build is not best on any track; valuable
|
||||||
|
// copies of the same car (e.g. a best 323 next to a useless 332)
|
||||||
|
// stay and must not appear in the Config column.
|
||||||
|
let whole_car =
|
||||||
|
low_value_guids.contains(guid.as_str()) || !counts.guid_to_rid.contains_key(guid);
|
||||||
|
let config = if whole_car {
|
||||||
|
counts.owned_configs.get(guid).map(|configs| configs.join("/")).unwrap_or_default()
|
||||||
|
} else {
|
||||||
|
let best_configs = &counts.guid_to_rid[guid];
|
||||||
|
counts
|
||||||
|
.owned_configs
|
||||||
|
.get(guid)
|
||||||
|
.map(|configs| {
|
||||||
|
configs
|
||||||
|
.iter()
|
||||||
|
.filter(|actual_config| {
|
||||||
|
is_final_build(actual_config)
|
||||||
|
&& !best_configs.iter().any(|(_, nc)| nc == *actual_config)
|
||||||
|
})
|
||||||
|
.cloned()
|
||||||
|
.collect::<Vec<_>>()
|
||||||
|
})
|
||||||
|
.unwrap_or_default()
|
||||||
|
.join("/")
|
||||||
|
};
|
||||||
|
if !whole_car && config.is_empty() {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
Some(TrashEntry { car, config })
|
Some(TrashEntry { car, config })
|
||||||
})
|
})
|
||||||
.collect();
|
.collect();
|
||||||
|
|
@ -1395,6 +1410,43 @@ mod tests {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_write_trash_csv_partial_copies_shows_only_deletable_config() {
|
||||||
|
let ctx = AnalysisContext::new();
|
||||||
|
|
||||||
|
// Car_A is best on 3 tracks with 323; player owns a 332 copy too.
|
||||||
|
let mut rec_323 = make_record("Car_A", 10, 90.0, "guid_a");
|
||||||
|
rec_323.config = "323".to_string();
|
||||||
|
rec_323.normalized_config = "323".to_string();
|
||||||
|
|
||||||
|
for i in 0..3 {
|
||||||
|
ctx.best_cars
|
||||||
|
.insert((format!("Track{i}"), 10, "Dry".to_string()), vec![rec_323.clone()]);
|
||||||
|
}
|
||||||
|
|
||||||
|
let car_list = vec![make_car("Car_A", 10, "guid_a")];
|
||||||
|
|
||||||
|
let mut owned_configs = HashMap::new();
|
||||||
|
owned_configs.insert("guid_a".to_string(), vec!["332".to_string(), "323".to_string()]);
|
||||||
|
|
||||||
|
let counts = compute_car_counts(&ctx, car_list, owned_configs, HashMap::new());
|
||||||
|
|
||||||
|
let owned: HashSet<String> = vec!["guid_a".to_string()].into_iter().collect();
|
||||||
|
|
||||||
|
let dir = tempfile::tempdir().unwrap();
|
||||||
|
let output_dir = dir.path().to_str().unwrap();
|
||||||
|
write_trash_csv(output_dir, &counts, &owned, None, &[]).unwrap();
|
||||||
|
|
||||||
|
let content = fs::read_to_string(dir.path().join("trash.csv")).unwrap();
|
||||||
|
let lines: Vec<&str> = content.lines().collect();
|
||||||
|
|
||||||
|
assert_eq!(lines.len(), 2, "Car_A should be trash via its 332 copy, got: {lines:?}");
|
||||||
|
assert_eq!(
|
||||||
|
lines[1], "10,TestBrand,TestModel,2020,332",
|
||||||
|
"Config column must list only the deletable 332 copy, not 332/323"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// =======================================================================
|
// =======================================================================
|
||||||
// compare_records tests
|
// compare_records tests
|
||||||
// =======================================================================
|
// =======================================================================
|
||||||
|
|
|
||||||
|
|
@ -97,6 +97,12 @@ pub fn parse_car_times(
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// An empty file means "no timing data" (the API reports this as an empty
|
||||||
|
// body) — same as a missing file, not a parse error.
|
||||||
|
if file_content.trim().is_empty() {
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
|
||||||
// Parse JSON data
|
// Parse JSON data
|
||||||
let json_data: Value = match serde_json::from_str(&file_content) {
|
let json_data: Value = match serde_json::from_str(&file_content) {
|
||||||
Ok(v) => v,
|
Ok(v) => v,
|
||||||
|
|
@ -399,12 +405,23 @@ pub async fn fetch_times(cars_db_path: &str, times_dir: &str) -> Result<(), AppE
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Downloads timing data for a single car from the remote API.
|
/// Downloads timing data for a single car from the remote API.
|
||||||
|
///
|
||||||
|
/// An empty response means the records site knows the car but no times have
|
||||||
|
/// been submitted yet (typical for freshly released prize cars) — treated as
|
||||||
|
/// a skip, not a failure, so the overall fetch is not aborted.
|
||||||
async fn fetch_car(rid: &str, times_dir: &str) -> Result<(), AppError> {
|
async fn fetch_car(rid: &str, times_dir: &str) -> Result<(), AppError> {
|
||||||
let file_name = rid.to_owned() + ".json";
|
let file_name = rid.to_owned() + ".json";
|
||||||
let url = format!("{CAR_TIMING_API_BASE}{rid}");
|
let url = format!("{CAR_TIMING_API_BASE}{rid}");
|
||||||
let local_file_path = format!("{times_dir}/{file_name}");
|
let local_file_path = format!("{times_dir}/{file_name}");
|
||||||
log::info!("Fetching {url}.");
|
log::info!("Fetching {url}.");
|
||||||
top_drives::get_file(&url, &local_file_path).await
|
match top_drives::get_file(&url, &local_file_path).await {
|
||||||
|
Ok(()) => Ok(()),
|
||||||
|
Err(AppError::EmptyResponse { .. }) => {
|
||||||
|
log::info!("no records on the site yet for {rid} — skipped");
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
Err(e) => Err(e),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
@ -734,6 +751,30 @@ mod tests {
|
||||||
assert_eq!(ctx.best_cars.len(), 0, "No entries for missing file");
|
assert_eq!(ctx.best_cars.len(), 0, "No entries for missing file");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_parse_car_times_empty_file_treated_as_missing() {
|
||||||
|
let ctx = AnalysisContext::new();
|
||||||
|
let car = Car {
|
||||||
|
rid: "EmptyCar_2020".to_string(),
|
||||||
|
brand: "TestBrand".to_string(),
|
||||||
|
only_name: "TestModel".to_string(),
|
||||||
|
year: 2020,
|
||||||
|
rq: 85,
|
||||||
|
prize: false,
|
||||||
|
guid: "guid_test".to_string(),
|
||||||
|
};
|
||||||
|
|
||||||
|
let dir = tempfile::tempdir().unwrap();
|
||||||
|
let times_dir = dir.path().to_str().unwrap();
|
||||||
|
fs::write(dir.path().join("EmptyCar_2020.json"), "").unwrap();
|
||||||
|
|
||||||
|
let my_cars: HashSet<String> = HashSet::new();
|
||||||
|
// Empty file must be Ok(()) like a missing file, not a parse error.
|
||||||
|
parse_car_times(car, &my_cars, &ctx.best_cars, times_dir).unwrap();
|
||||||
|
|
||||||
|
assert_eq!(ctx.best_cars.len(), 0, "No entries for empty file");
|
||||||
|
}
|
||||||
|
|
||||||
// =======================================================================
|
// =======================================================================
|
||||||
// Track name / type tests (from lib)
|
// Track name / type tests (from lib)
|
||||||
// =======================================================================
|
// =======================================================================
|
||||||
|
|
|
||||||
10
src/types.rs
10
src/types.rs
|
|
@ -29,6 +29,16 @@ pub enum AppError {
|
||||||
reason: String,
|
reason: String,
|
||||||
},
|
},
|
||||||
|
|
||||||
|
/// The server responded with 200 but an empty body.
|
||||||
|
///
|
||||||
|
/// The records API uses this to signal "known resource, no data submitted yet"
|
||||||
|
/// (typically newly released prize cars). Deterministic — retrying cannot help.
|
||||||
|
#[error("Empty response from {url} (no data available on the server)")]
|
||||||
|
EmptyResponse {
|
||||||
|
/// The URL that returned the empty body
|
||||||
|
url: String,
|
||||||
|
},
|
||||||
|
|
||||||
/// A required data file is missing
|
/// A required data file is missing
|
||||||
#[error("Missing data file: {0}")]
|
#[error("Missing data file: {0}")]
|
||||||
MissingFile(String),
|
MissingFile(String),
|
||||||
|
|
|
||||||
|
|
@ -31,22 +31,23 @@ async fn test_get_file_success() {
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn test_get_file_empty_response_retries_and_fails() {
|
async fn test_get_file_empty_response_fails_immediately_without_retry() {
|
||||||
let mut server = Server::new_async().await;
|
let mut server = Server::new_async().await;
|
||||||
// Return 200 with empty body — triggers retry, then fails
|
// 200 with empty body means "no data available yet" — permanent, so exactly
|
||||||
let m = server
|
// one request must be made (no retries) and a distinguishable error returned.
|
||||||
.mock("GET", "/data.json")
|
let m = server.mock("GET", "/data.json").with_status(200).with_body("").expect(1).create();
|
||||||
.with_status(200)
|
|
||||||
.with_body("")
|
|
||||||
.expect(4) // 1 initial + 3 retries
|
|
||||||
.create();
|
|
||||||
|
|
||||||
let dir = tempfile::tempdir().unwrap();
|
let dir = tempfile::tempdir().unwrap();
|
||||||
let out_path = dir.path().join("output.txt");
|
let out_path = dir.path().join("output.txt");
|
||||||
let url = format!("{}/data.json", server.url());
|
let url = format!("{}/data.json", server.url());
|
||||||
|
|
||||||
let result = get_file(&url, out_path.to_str().unwrap()).await;
|
let result = get_file(&url, out_path.to_str().unwrap()).await;
|
||||||
assert!(result.is_err(), "Empty response should fail after retries");
|
let err = result.expect_err("Empty response should fail immediately");
|
||||||
|
assert!(
|
||||||
|
matches!(err, top_drives::AppError::EmptyResponse { .. }),
|
||||||
|
"Empty response should yield AppError::EmptyResponse, got: {err}"
|
||||||
|
);
|
||||||
|
assert!(!out_path.exists(), "No file should be written for an empty response");
|
||||||
m.assert();
|
m.assert();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue