diff --git a/server/src/main.rs b/server/src/main.rs index f27158c64..db4a5ee7c 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -180,7 +180,7 @@ fn main() { // Initialize culture resolver (#679, D-128). // systems.db is shipped read-only alongside the binary. - let systems_db_path = std::path::PathBuf::from("data/systems.db"); + let systems_db_path = resolve_systems_db_path(); match settled_reach_server::knowledge::CultureResolver::open(&systems_db_path) { Ok(resolver) => { tracing::info!("Culture resolver opened: {:?}", systems_db_path); @@ -198,7 +198,16 @@ fn main() { // Mod-first body source resolver for the atlas layer proxy (#969, D-225). // terrain_reference is repo-root-relative; the repo root is systems.db's - // 3rd ancestor (/server/data/systems.db). + // 3rd ancestor (/server/data/systems.db). This ancestor arithmetic + // is only correct against an ABSOLUTE path — `.canonicalize()` resolves a + // *relative* path against the CURRENT CWD, so if `systems_db_path` were + // still cwd-relative (as it was before `resolve_systems_db_path()`, T-1131 + // follow-up), a wrong-cwd launch (e.g. the D-254 companion spawning from + // the repo root) would make `.canonicalize()` fail outright, falling back + // to the nonsense `".."` default below. `resolve_systems_db_path()` + // already verified `systems_db_path` exists before returning it, so + // `.canonicalize()` here always succeeds and `nth(3)` is genuinely correct + // — not "pretends to work by accident when cwd happens to be server/". let world_root = systems_db_path .canonicalize() .ok() @@ -484,6 +493,258 @@ fn main() { tracing::info!("Simulation server shutting down"); } +/// Candidate `systems.db` paths, in try-order, for a given executable path +/// (T-1131 follow-up). Pure/no I/O — the ONLY thing that makes this +/// deterministic and unit-testable given `exe_path` (unlike +/// [`resolve_systems_db_path`], which additionally calls +/// `std::env::current_exe()` and stats the filesystem). Kept separate +/// specifically so the candidate ORDER and SHAPE can be tested without a +/// process-spawning harness — see `tests::` below. +/// +/// 1. Exe-anchored `/../../data/systems.db` (unjoined — the caller +/// canonicalizes and existence-checks; this function never touches disk). +/// Only present if `exe_path` has a parent directory. +/// 2. Cwd-relative `data/systems.db` (today's pre-fix behavior — +/// `cd server && cargo run` leaves cwd at `server/`). +/// 3. Cwd-relative `server/data/systems.db` (repo-root invocations). +fn systems_db_candidates(exe_path: Option<&std::path::Path>) -> Vec { + let mut candidates = Vec::with_capacity(3); + if let Some(exe_dir) = exe_path.and_then(std::path::Path::parent) { + candidates.push(exe_dir.join("../../data/systems.db")); + } + candidates.push(std::path::PathBuf::from("data/systems.db")); + candidates.push(std::path::PathBuf::from("server/data/systems.db")); + candidates +} + +#[cfg(test)] +mod tests { + use super::*; + + /// T-1131 follow-up: the exe-anchored candidate must resolve to + /// `server/data/systems.db` from the DEV BUILD LAYOUT exe path + /// (`server/target/debug/settled-reach-server`) — this is the whole + /// point of the fix, so pin the exact join shape, not just "some path + /// containing systems.db". + #[test] + fn exe_anchored_candidate_targets_server_data_from_dev_build_layout() { + let exe = std::path::Path::new("/repo/server/target/debug/settled-reach-server"); + let candidates = systems_db_candidates(Some(exe)); + + assert_eq!( + candidates.len(), + 3, + "exe with a parent dir must produce all three candidates" + ); + assert_eq!( + candidates[0], + std::path::PathBuf::from("/repo/server/target/debug/../../data/systems.db"), + "exe-anchored candidate must be unjoined (caller canonicalizes) \ + but built from exe_dir/../../data/systems.db" + ); + + // The whole point: once normalized (what canonicalize() does at + // runtime against a real filesystem), this lands on + // /repo/server/data/systems.db — the actual DB location — not + // /repo/data/systems.db (the pre-fix cwd-relative bug's target). + let normalized = normalize_lexically(&candidates[0]); + assert_eq!( + normalized, + std::path::PathBuf::from("/repo/server/data/systems.db") + ); + } + + /// The two cwd-relative fallback candidates are present regardless of + /// whether an exe path resolved, in the documented order: `data/systems.db` + /// before `server/data/systems.db` (today's pre-fix behavior stays the + /// first fallback, not silently reordered behind the new repo-root case). + #[test] + fn cwd_relative_candidates_present_and_ordered_when_exe_path_is_some() { + let exe = std::path::Path::new("/repo/server/target/debug/settled-reach-server"); + let candidates = systems_db_candidates(Some(exe)); + assert_eq!(candidates[1], std::path::PathBuf::from("data/systems.db")); + assert_eq!( + candidates[2], + std::path::PathBuf::from("server/data/systems.db") + ); + } + + /// `current_exe()` can fail (documented caveat, e.g. sandboxed + /// environments) — `None` must degrade to exactly the two cwd-relative + /// candidates, not panic or produce a malformed exe-anchored entry. + #[test] + fn no_exe_path_yields_only_the_two_cwd_relative_candidates() { + let candidates = systems_db_candidates(None); + assert_eq!(candidates.len(), 2); + assert_eq!(candidates[0], std::path::PathBuf::from("data/systems.db")); + assert_eq!( + candidates[1], + std::path::PathBuf::from("server/data/systems.db") + ); + } + + /// An exe path that IS genuinely parentless (`Path::parent()` returns + /// `None` only for the empty path or filesystem root — confirmed against + /// the standard library, not assumed) must not panic and must degrade + /// the same as `exe_path: None`. + #[test] + fn genuinely_parentless_exe_path_degrades_like_no_exe_path() { + let exe = std::path::Path::new(""); + assert!( + exe.parent().is_none(), + "test premise: Path::new(\"\").parent() must be None" + ); + let candidates = systems_db_candidates(Some(exe)); + assert_eq!(candidates.len(), 2); + assert_eq!(candidates[0], std::path::PathBuf::from("data/systems.db")); + } + + /// A bare relative filename with no directory separator (e.g. the exe + /// path Godot's `OS.create_process` might report on some platform/launch + /// combination) is NOT the parentless case above — `Path::parent()` + /// returns `Some("")` for it (an empty-but-present parent), a real + /// standard-library quirk worth pinning explicitly since it's easy to + /// assume `.parent()` is `None` whenever there's "no directory in the + /// string". The exe-anchored candidate still gets produced (joined onto + /// the empty parent), just degenerately — `../../data/systems.db` + /// relative to cwd, which is harmless: it'll fail existence-checks + /// exactly like any other wrong candidate and fall through the loop. + #[test] + fn bare_filename_exe_path_has_an_empty_but_present_parent() { + let exe = std::path::Path::new("settled-reach-server"); + assert_eq!( + exe.parent(), + Some(std::path::Path::new("")), + "Path::parent() of a bare filename is Some(\"\"), not None — \ + pinning this stdlib behavior since it's the reason a bare \ + filename still produces 3 candidates, not 2" + ); + let candidates = systems_db_candidates(Some(exe)); + assert_eq!( + candidates.len(), + 3, + "a present-but-empty parent still yields an exe-anchored candidate" + ); + assert_eq!( + candidates[0], + std::path::PathBuf::from("../../data/systems.db"), + "joined onto an empty parent, the exe-anchored candidate is bare \ + ../../data/systems.db (cwd-relative in practice, but still a \ + DISTINCT candidate from candidates[1]'s exact data/systems.db)" + ); + } + + /// Lexical `..`/`.` normalization for test assertions ONLY — a stand-in + /// for `Path::canonicalize()` (which needs a real filesystem + cwd, + /// which unit tests must not depend on per the coordinator's "don't + /// build a process-spawning/filesystem harness for this" guidance). + /// `resolve_systems_db_path` itself still uses the real + /// `canonicalize()` at runtime — this helper exists only so + /// `exe_anchored_candidate_targets_server_data_from_dev_build_layout` + /// can assert the join shape actually lands on the right final path + /// without touching disk. + fn normalize_lexically(path: &std::path::Path) -> std::path::PathBuf { + let mut out = std::path::PathBuf::new(); + for component in path.components() { + match component { + std::path::Component::ParentDir => { + out.pop(); + } + std::path::Component::CurDir => {} + other => out.push(other.as_os_str()), + } + } + out + } +} + +/// Resolve `systems.db`'s path, ANCHORED TO THE EXECUTABLE rather than the +/// current working directory (T-1131 follow-up). +/// +/// **The bug this fixes:** `PathBuf::from("data/systems.db")` is cwd-relative. +/// `make game` (`cd server && cargo run`) happens to leave cwd at `server/`, +/// so that path resolves — but the D-254 companion app spawns this binary via +/// Godot's `OS.create_process`/`OS.execute_with_pipe` (`server_process.gd`), +/// neither of which sets a working directory: the child inherits GODOT's cwd, +/// which for `make atlas`/`make game` is the REPO ROOT (the Makefile has no +/// `cd` before launching Godot itself — only before `cargo run`). From the +/// repo root, `data/systems.db` doesn't exist (it's `server/data/systems.db`), +/// so every DB-backed reader (`CultureResolver`, `CityContextReader`, and now +/// `BrowseReader`) silently fails to open in every spawned-server context. +/// This went unnoticed through T-1130's wave 1 because the star map is served +/// from `star_map_data.json` via `world_root` (itself derived from +/// `systems_db_path`, so ALSO broken — but `.exists()`-checked with a `warn`, +/// not a hard dependency any single-connection smoke test would surface) — +/// "renders 301 systems" never actually touched `systems.db`. +/// +/// **The fix:** resolve relative to `std::env::current_exe()` first — in the +/// dev build layout the binary is `server/target/debug/settled-reach-server`, +/// so `exe_dir/../../data/systems.db` is `server/data/systems.db` regardless +/// of cwd. Falls through to the two cwd-relative candidates (today's +/// behavior, and the repo-root equivalent) so `cd server && cargo run` and a +/// repo-root-relative invocation both keep working without needing the +/// exe-anchoring to succeed (e.g. `current_exe()` can fail in exotic +/// sandboxed environments per its own documented caveats). +/// +/// Candidate order/shape lives in [`systems_db_candidates`] (pure, +/// unit-tested); this function adds the I/O layer: canonicalize + existence +/// check per candidate, first EXISTING one wins, with an `info` log +/// recording which candidate resolved (so a future "browse reader +/// unavailable" report is diagnosable from the startup log alone). +/// +/// If none exist, returns the cwd-relative `data/systems.db` default — +/// today's pre-fix behavior — so every downstream `Reader::open()` call +/// still gets a path to fail on and log its own existing +/// `warn`-and-degrade message. This function does not invent a new failure +/// mode, it just tries harder before giving up. +fn resolve_systems_db_path() -> std::path::PathBuf { + let exe_path = std::env::current_exe().ok(); + let candidates = systems_db_candidates(exe_path.as_deref()); + + for candidate in &candidates { + let canonical = candidate.canonicalize(); + if let Ok(ref resolved) = canonical { + if resolved.exists() { + tracing::info!( + "systems.db resolved: {:?} (candidate: {:?}, exe: {:?})", + resolved, + candidate, + exe_path + ); + return resolved.clone(); + } + } else if candidate.exists() { + // canonicalize() can fail even when the path exists (e.g. a + // component permission error) — exists() is the true signal; + // canonicalize() is just how we get an absolute path for + // world_root's ancestor arithmetic to work correctly. + tracing::info!( + "systems.db resolved (uncanonicalized): {:?} (exe: {:?})", + candidate, + exe_path + ); + return candidate.clone(); + } + } + + // None of the candidates exist. Fall back to the cwd-relative + // `data/systems.db` default — today's pre-fix behavior — NOT the + // exe-anchored candidate (which, per systems_db_candidates' doc, is + // unjoined/uncanonicalized and only meaningful once verified to exist; + // returning it here unverified would be a worse default than the plain + // relative path every downstream Reader::open() already knows how to + // fail on cleanly). + let fallback = std::path::PathBuf::from("data/systems.db"); + tracing::warn!( + "systems.db not found via any of {:?} (exe: {:?}) — falling back to {:?} \ + (every DB-backed reader will report unavailable and degrade)", + candidates, + exe_path, + fallback + ); + fallback +} + /// Best-effort: send a final SimError snapshot to the client on panic (#85). /// /// Builds a minimal ObserverSnapshot with the panic error and sends it