Skip to content

Commit b598368

Browse files
committed
fix: defer migration warning until after tracing subscriber is initialised
In my infinite wisdom, my previous commit attempted to fix the silent migration warning by calling init_logging("warn") before Config::load. The real problem was that load_json_and_migrate fires tracing::warn! from inside Config::load, but the subscriber can only be initialised after Config::load returns, because the user's log_level lives inside the config that hasn't been read yet. Classic chicken-and-egg. Locking to "warn" early meant the second init_logging(&config.log_level) call was a silent no-op (try_init semantics), so users lost their configured log level entirely. Fix: instead of logging directly inside load_json_and_migrate, collect the warning as an Option<String> and return it alongside the Config up the call chain. main.rs logs it with tracing::warn! after init_logging(&config.log_level) — one subscriber, correct level, migration warning visible with proper timestamp and severity. Config::load return type changes from Result<Self> to Result<(Self, Option<String>)>; load_toml paths return None, the JSON migration path returns Some(message). Test call sites updated with .0 to destructure the tuple.
1 parent 7c9d6da commit b598368

2 files changed

Lines changed: 24 additions & 23 deletions

File tree

src/config.rs

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -551,26 +551,26 @@ fn default_verify_ssl() -> bool {
551551
}
552552

553553
impl Config {
554-
pub fn load(path: &Path) -> Result<Self, ConfigError> {
554+
pub fn load(path: &Path) -> Result<(Self, Option<String>), ConfigError> {
555555
let ext = path
556556
.extension()
557557
.and_then(|e| e.to_str())
558558
.unwrap_or("")
559559
.to_ascii_lowercase();
560560

561561
match ext.as_str() {
562-
"toml" => Self::load_toml(path),
562+
"toml" => Self::load_toml(path).map(|c| (c, None)),
563563
"json" => Self::load_json_and_migrate(path),
564564
_ => {
565565
// No extension or unrecognised: try TOML first, then JSON.
566566
// JSON success also triggers migration. On double failure,
567567
// surface the TOML error (the format new configs expect).
568568
let toml_err = match Self::load_toml(path) {
569-
Ok(cfg) => return Ok(cfg),
569+
Ok(cfg) => return Ok((cfg, None)),
570570
Err(e) => e,
571571
};
572572
match Self::load_json_and_migrate(path) {
573-
Ok(cfg) => Ok(cfg),
573+
Ok((cfg, msg)) => Ok((cfg, msg)),
574574
Err(_) => Err(toml_err),
575575
}
576576
}
@@ -587,7 +587,7 @@ impl Config {
587587
Ok(cfg)
588588
}
589589

590-
fn load_json_and_migrate(path: &Path) -> Result<Self, ConfigError> {
590+
fn load_json_and_migrate(path: &Path) -> Result<(Self, Option<String>), ConfigError> {
591591
let data = std::fs::read_to_string(path)
592592
.map_err(|e| ConfigError::Read(path.display().to_string(), e))?;
593593
let cfg: Config = serde_json::from_str(&data)?;
@@ -596,29 +596,28 @@ impl Config {
596596
// Write a .toml equivalent alongside the .json file. Failure is
597597
// non-fatal: the in-memory Config is still valid and returned.
598598
let toml_path = path.with_extension("toml");
599-
match toml::to_string_pretty(&TomlConfig::from(&cfg)) {
599+
let msg = match toml::to_string_pretty(&TomlConfig::from(&cfg)) {
600600
Ok(toml_str) => match std::fs::write(&toml_path, &toml_str) {
601-
Ok(()) => tracing::warn!(
601+
Ok(()) => Some(format!(
602602
"Found legacy config.json. Translated to {} automatically. \
603603
config.json has been left in place but will no longer be read. \
604604
You can delete it.",
605605
toml_path.display()
606-
),
607-
Err(e) => tracing::warn!(
606+
)),
607+
Err(e) => Some(format!(
608608
"Found legacy config.json but could not write {}: {}. \
609609
Continuing from the JSON config.",
610-
toml_path.display(),
611-
e
612-
),
610+
toml_path.display(), e
611+
)),
613612
},
614-
Err(e) => tracing::warn!(
613+
Err(e) => Some(format!(
615614
"Found legacy config.json but could not serialize to TOML: {}. \
616615
Continuing from the JSON config.",
617616
e
618-
),
619-
}
620-
621-
Ok(cfg)
617+
)),
618+
};
619+
620+
Ok((cfg, msg))
622621
}
623622

624623
fn validate(&self) -> Result<(), ConfigError> {
@@ -1216,7 +1215,7 @@ mod rt_tests {
12161215
}"#;
12171216
let tmp = std::env::temp_dir().join("mhrv-rt-test.json");
12181217
std::fs::write(&tmp, json).unwrap();
1219-
let cfg = Config::load(&tmp).expect("config should load");
1218+
let cfg = Config::load(&tmp).expect("config should load").0;
12201219
assert_eq!(cfg.mode, "apps_script");
12211220
assert_eq!(cfg.auth_key, "testtesttest");
12221221
assert_eq!(cfg.listen_port, 8085);
@@ -1280,7 +1279,7 @@ mod rt_tests {
12801279
}"#;
12811280
let tmp = std::env::temp_dir().join("mhrv-rt-min.json");
12821281
std::fs::write(&tmp, json).unwrap();
1283-
let cfg = Config::load(&tmp).expect("minimal config should load");
1282+
let cfg = Config::load(&tmp).expect("minimal config should load").0;
12841283
assert_eq!(cfg.mode, "apps_script");
12851284
let _ = std::fs::remove_file(tmp.with_extension("toml"));
12861285
let _ = std::fs::remove_file(&tmp);
@@ -1412,7 +1411,7 @@ script_id = "ABCDEF"
14121411
"#;
14131412
let tmp = std::env::temp_dir().join("mhrv-load-toml-test.toml");
14141413
std::fs::write(&tmp, s).unwrap();
1415-
let cfg = Config::load(&tmp).expect("Config::load must handle .toml extension");
1414+
let cfg = Config::load(&tmp).expect("Config::load must handle .toml extension").0;
14161415
assert_eq!(cfg.mode, "apps_script");
14171416
assert_eq!(cfg.script_ids_resolved(), vec!["ABCDEF".to_string()]);
14181417
let _ = std::fs::remove_file(&tmp);
@@ -1434,7 +1433,7 @@ script_id = "ABCDEF"
14341433

14351434
std::fs::write(&json_path, json).unwrap();
14361435
let cfg = Config::load(&json_path)
1437-
.expect("JSON config must load and trigger migration");
1436+
.expect("JSON config must load and trigger migration").0;
14381437

14391438
assert!(toml_path.exists(), "migration must write config.toml alongside config.json");
14401439

src/main.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -201,8 +201,7 @@ async fn main() -> ExitCode {
201201
}
202202

203203
let config_path = mhrv_rs::data_dir::resolve_config_path(args.config_path.as_deref());
204-
init_logging("warn"); // boot-time logging so migration warning is visible
205-
let config = match Config::load(&config_path) {
204+
let (config, migration_warn) = match Config::load(&config_path) {
206205
Ok(c) => c,
207206
Err(e) => {
208207
eprintln!("{}", e);
@@ -215,6 +214,9 @@ async fn main() -> ExitCode {
215214
};
216215

217216
init_logging(&config.log_level);
217+
if let Some(msg) = migration_warn {
218+
tracing::warn!("{}", msg);
219+
}
218220

219221
// Bump RLIMIT_NOFILE now that tracing is live — OpenWRT/Alpine hosts
220222
// often ship a default so low (issue #8, issue #18) that we run out

0 commit comments

Comments
 (0)