Skip to content

fix: handle dashboard cmd config BOM #145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zouyonghe merged 1 commit into from
Jun 30, 2026
Merged

fix: handle dashboard cmd config BOM #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
248 changes: 226 additions & 22 deletions src-tauri/src/backend/launch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ use std::{
env,
ffi::{OsStr, OsString},
fs::{self, OpenOptions},
io,
path::Path,
process::{Command, Stdio},
thread,
time::Duration,
};

#[cfg(target_os = "windows")]
Expand All @@ -30,6 +33,11 @@ const DASHBOARD_SKIP_DEFAULT_PASSWORD_AUTH_ENV: &str = "DASHBOARD_SKIP_DEFAULT_P
const DEFAULT_DASHBOARD_HOST: &str = "127.0.0.1";
const DEFAULT_DASHBOARD_PORT: &str = "6185";
const CMD_CONFIG_RELATIVE_PATH: &str = "data/cmd_config.json";
const CMD_CONFIG_READ_RETRY_ATTEMPTS: usize = 5;
#[cfg(not(test))]
const CMD_CONFIG_READ_RETRY_DELAY: Duration = Duration::from_millis(100);
#[cfg(test)]
const CMD_CONFIG_READ_RETRY_DELAY: Duration = Duration::from_millis(1);

#[derive(Debug, Default)]
struct CmdDashboardConfig {
Expand Down Expand Up @@ -200,31 +208,124 @@ fn read_cmd_dashboard_config(
return CmdDashboardConfig::default();
}

let raw = match fs::read_to_string(&config_path) {
Ok(raw) => raw,
Err(error) => {
log(&format!(
"failed to read cmd_config {}: {}",
config_path.display(),
error
));
return CmdDashboardConfig::default();
}
};
let parsed: CmdConfigFile = match serde_json::from_str(&raw) {
let parsed = match read_cmd_config_file_with_retry(&config_path) {
Ok(parsed) => parsed,
Err(error) => {
log(&format!(
"failed to parse cmd_config {}: {}",
config_path.display(),
error
));
match error {
CmdConfigError::Read(error) => log(&format!(
"failed to read cmd_config {}: {}",
config_path.display(),
error
)),
CmdConfigError::Parse { error, .. } => log(&format!(
"failed to parse cmd_config {}: {}",
config_path.display(),
error
)),
}
return CmdDashboardConfig::default();
}
};
CmdDashboardConfig::from_file_config(parsed, log)
}

#[derive(Debug)]
enum CmdConfigError {
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
Read(io::Error),
Parse {
error: serde_json::Error,
is_empty: bool,
is_nonempty_eof: bool,
},
}

fn is_retryable_cmd_config_io_error(error: &io::Error) -> bool {

@sourcery-ai sourcery-ai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (complexity): Consider inlining the error-classification logic into the retry loop to simplify the control flow while preserving the new behavior and tests.

You can keep the new behavior and tests while reducing indirection by collapsing the error-classification helpers into the retry loop. This removes is_retryable_cmd_config_io_error, should_retry_cmd_config_read, and describe_cmd_config_error without changing semantics.

For example, replace the current retry helpers and loop with something like:

fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
    read_cmd_config_file_with_retry_and_hook(config_path, |_| {})
}

fn read_cmd_config_file_with_retry_and_hook<F>(
    config_path: &Path,
    mut after_retryable_error: F,
) -> Result<CmdConfigFile, CmdConfigError>
where
    F: FnMut(usize),
{
    let mut attempt = 1;
    loop {
        match read_cmd_config_file_once(config_path) {
            Ok(config) => return Ok(config),
            Err(error) => {
                let retryable = match &error {
                    CmdConfigError::Read(e) => matches!(
                        e.kind(),
                        io::ErrorKind::NotFound
                            | io::ErrorKind::Interrupted
                            | io::ErrorKind::WouldBlock
                            | io::ErrorKind::TimedOut
                    ),
                    CmdConfigError::Parse {
                        is_empty,
                        is_nonempty_eof,
                        ..
                    } => *is_empty || *is_nonempty_eof,
                };

                if !retryable || attempt >= CMD_CONFIG_READ_RETRY_ATTEMPTS {
                    return Err(error);
                }

                append_desktop_log(&format!(
                    "retrying cmd_config read {}/{} for {}: {error}",
                    attempt,
                    CMD_CONFIG_READ_RETRY_ATTEMPTS,
                    config_path.display(),
                ));
                after_retryable_error(attempt - 1);
                thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
                attempt += 1;
            }
        }
    }
}

Then you can delete:

fn is_retryable_cmd_config_io_error(error: &io::Error) -> bool { ... }

fn should_retry_cmd_config_read(error: &CmdConfigError) -> bool { ... }

fn describe_cmd_config_error(error: &CmdConfigError) -> String { ... }

This keeps:

  • The CmdConfigError enum and its extra parse metadata
  • All existing retry behavior (including empty vs truncated vs whitespace-only handling)
  • The test hook via read_cmd_config_file_with_retry_and_hook

but reduces the number of conceptual layers in the retry path and makes the loop easier to follow.

matches!(
error.kind(),
io::ErrorKind::NotFound
| io::ErrorKind::Interrupted
| io::ErrorKind::WouldBlock
| io::ErrorKind::TimedOut
)
}

fn should_retry_cmd_config_read(error: &CmdConfigError) -> bool {
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
match error {
CmdConfigError::Read(error) => is_retryable_cmd_config_io_error(error),
CmdConfigError::Parse {
is_empty,
is_nonempty_eof,
..
} => {
// Empty files and truncated JSON can occur while cmd_config is being rewritten.
// Whitespace-only files are non-empty invalid JSON, so treat them as misconfiguration.
*is_empty || *is_nonempty_eof
}
}
}

fn describe_cmd_config_error(error: &CmdConfigError) -> String {
match error {
CmdConfigError::Read(error) => format!("read failed: {error}"),
CmdConfigError::Parse { error, .. } => format!("parse failed: {error}"),
}
}

fn read_cmd_config_file_once(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
let mut raw = fs::read_to_string(config_path).map_err(CmdConfigError::Read)?;
if raw.starts_with('\u{feff}') {
raw.remove(0);
}
match serde_json::from_str(&raw) {
Ok(parsed) => Ok(parsed),
Err(error) => {
let is_empty = raw.is_empty();
let is_nonempty_eof = error.is_eof() && !raw.trim().is_empty();
Err(CmdConfigError::Parse {
error,
is_empty,
is_nonempty_eof,
})
}
}
}

fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
read_cmd_config_file_with_retry_and_hook(config_path, |_| {})
}

fn read_cmd_config_file_with_retry_and_hook<F>(
config_path: &Path,
mut after_retryable_error: F,
) -> Result<CmdConfigFile, CmdConfigError>
where
F: FnMut(usize),
{
let mut attempt = 1;
loop {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
if !should_retry_cmd_config_read(&error)
|| attempt >= CMD_CONFIG_READ_RETRY_ATTEMPTS
{
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
return Err(error);
}
append_desktop_log(&format!(
"retrying cmd_config read {}/{} for {}: {}",
attempt,
CMD_CONFIG_READ_RETRY_ATTEMPTS,
config_path.display(),
describe_cmd_config_error(&error)
));
after_retryable_error(attempt - 1);
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
attempt += 1;
}
}
}
}

fn should_skip_default_password_auth(
has_explicit_skip_auth: bool,
effective_host: Option<&OsStr>,
Expand Down Expand Up @@ -430,11 +531,11 @@ mod tests {
use std::os::unix::ffi::OsStringExt;

use super::{
configure_desktop_dashboard_environment, sanitize_packaged_python_environment,
ASTRBOT_DASHBOARD_HOST_ENV, ASTRBOT_DASHBOARD_PORT_ENV,
ASTRBOT_DASHBOARD_SKIP_DEFAULT_PASSWORD_AUTH_ENV, CMD_CONFIG_RELATIVE_PATH,
DASHBOARD_HOST_ENV, DASHBOARD_PORT_ENV, DASHBOARD_SKIP_DEFAULT_PASSWORD_AUTH_ENV,
DEFAULT_DASHBOARD_HOST, DEFAULT_DASHBOARD_PORT,
configure_desktop_dashboard_environment, read_cmd_config_file_with_retry_and_hook,
sanitize_packaged_python_environment, ASTRBOT_DASHBOARD_HOST_ENV,
ASTRBOT_DASHBOARD_PORT_ENV, ASTRBOT_DASHBOARD_SKIP_DEFAULT_PASSWORD_AUTH_ENV,
CMD_CONFIG_RELATIVE_PATH, DASHBOARD_HOST_ENV, DASHBOARD_PORT_ENV,
DASHBOARD_SKIP_DEFAULT_PASSWORD_AUTH_ENV, DEFAULT_DASHBOARD_HOST, DEFAULT_DASHBOARD_PORT,
};

static ENV_TEST_LOCK: Mutex<()> = Mutex::new(());
Expand Down Expand Up @@ -624,6 +725,109 @@ mod tests {
});
}

#[test]
fn configure_desktop_dashboard_environment_accepts_utf8_bom_cmd_config() {
with_clean_dashboard_env(|| {
let root = tempfile::tempdir().expect("temp root");
write_cmd_config(
root.path(),
"\u{feff}{\"dashboard\":{\"host\":\"0.0.0.0\",\"port\":6185}}",
);
let mut command = Command::new("sh");

configure_desktop_dashboard_environment(&mut command, Some(root.path()), &mut |_| {});

assert_eq!(
get_command_env_value(&command, DASHBOARD_HOST_ENV),
Some(Some("0.0.0.0".to_string()))
);
assert_eq!(
get_command_env_value(&command, DASHBOARD_PORT_ENV),
Some(Some("6185".to_string()))
);
});
}

#[test]
fn configure_desktop_dashboard_environment_retries_transient_empty_cmd_config() {
with_clean_dashboard_env(|| {
let root = tempfile::tempdir().expect("temp root");
write_cmd_config(root.path(), "");
let config_path = root.path().join(CMD_CONFIG_RELATIVE_PATH);
let mut retry_count = 0;

let config = read_cmd_config_file_with_retry_and_hook(&config_path, |attempt| {
assert_eq!(attempt, 0);
retry_count += 1;
fs::write(
&config_path,
r#"{"dashboard":{"host":"0.0.0.0","port":6185}}"#,
)
.expect("write completed cmd config");
})
.expect("retry reads completed cmd config");
let parsed = super::CmdDashboardConfig::from_file_config(config, &mut |_| {});

assert_eq!(retry_count, 1);
assert_eq!(parsed.host.as_deref(), Some("0.0.0.0"));
assert_eq!(parsed.port.as_deref(), Some("6185"));
});
}

#[test]
fn configure_desktop_dashboard_environment_retries_transient_missing_cmd_config() {
with_clean_dashboard_env(|| {
let root = tempfile::tempdir().expect("temp root");
write_cmd_config(root.path(), r#"{"dashboard":{"host":"127.0.0.1"}}"#);
let config_path = root.path().join(CMD_CONFIG_RELATIVE_PATH);
fs::remove_file(&config_path).expect("remove cmd config during rewrite");
let mut retry_count = 0;

let config = read_cmd_config_file_with_retry_and_hook(&config_path, |attempt| {
assert_eq!(attempt, 0);
retry_count += 1;
fs::write(
&config_path,
r#"{"dashboard":{"host":"0.0.0.0","port":6185}}"#,
)
.expect("write completed cmd config");
})
.expect("retry reads completed cmd config");
let parsed = super::CmdDashboardConfig::from_file_config(config, &mut |_| {});

assert_eq!(retry_count, 1);
assert_eq!(parsed.host.as_deref(), Some("0.0.0.0"));
assert_eq!(parsed.port.as_deref(), Some("6185"));
});
}

#[test]
fn configure_desktop_dashboard_environment_does_not_retry_whitespace_only_cmd_config() {
with_clean_dashboard_env(|| {
let root = tempfile::tempdir().expect("temp root");
write_cmd_config(root.path(), "\n\n ");
let config_path = root.path().join(CMD_CONFIG_RELATIVE_PATH);
let mut retry_count = 0;
let mut command = Command::new("sh");

configure_desktop_dashboard_environment(&mut command, Some(root.path()), &mut |_| {});
let result = read_cmd_config_file_with_retry_and_hook(&config_path, |_| {
retry_count += 1;
});

assert_eq!(
get_command_env_value(&command, DASHBOARD_HOST_ENV),
Some(Some(DEFAULT_DASHBOARD_HOST.to_string()))
);
assert_eq!(
get_command_env_value(&command, DASHBOARD_PORT_ENV),
Some(Some(DEFAULT_DASHBOARD_PORT.to_string()))
);
assert!(result.is_err());
assert_eq!(retry_count, 0);
});
}

#[test]
fn configure_desktop_dashboard_environment_env_overrides_cmd_config() {
with_clean_dashboard_env(|| {
Expand Down
27 changes: 27 additions & 0 deletions src-tauri/src/desktop_settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,28 @@ mod tests {
root.join("data").join("desktop_state.json")
}

struct EnvVarGuard {
key: &'static str,
previous: Option<String>,
}

impl EnvVarGuard {
fn clear(key: &'static str) -> Self {
let previous = std::env::var(key).ok();
std::env::remove_var(key);
Self { key, previous }
}
}
Comment on lines +227 to +238

@gemini-code-assist gemini-code-assist Bot Jun 30, 2026

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.

high

Rust tests run in parallel by default. Since environment variables are process-global, modifying them concurrently in multiple tests without synchronization can cause race conditions and flaky test failures.

By adding a static Mutex and holding its guard inside EnvVarGuard, we can ensure that all tests utilizing EnvVarGuard are executed sequentially, preventing environment variable pollution and race conditions.

    static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

    struct EnvVarGuard {
        key: &'static str,
        previous: Option<String>,
        _lock: std::sync::MutexGuard<'static, ()>,
    }

    impl EnvVarGuard {
        fn clear(key: &'static str) -> Self {
            let _lock = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner());
            let previous = std::env::var(key).ok();
            std::env::remove_var(key);
            Self {
                key,
                previous,
                _lock,
            }
        }
    }


impl Drop for EnvVarGuard {
fn drop(&mut self) {
match &self.previous {
Some(value) => std::env::set_var(self.key, value),
None => std::env::remove_var(self.key),
}
}
}

fn settings(
launch_at_login: bool,
silent_launch: bool,
Expand All @@ -244,6 +266,7 @@ mod tests {

#[test]
fn read_desktop_settings_maps_camel_case_fields() {
let _root_guard = EnvVarGuard::clear(crate::ASTRBOT_ROOT_ENV);
let root = create_temp_case_dir("read");
let path = state_path(&root);
fs::create_dir_all(path.parent().expect("state parent")).expect("create state parent");
Expand All @@ -261,6 +284,7 @@ mod tests {

#[test]
fn read_desktop_settings_applies_field_defaults_for_missing_values() {
let _root_guard = EnvVarGuard::clear(crate::ASTRBOT_ROOT_ENV);
let root = create_temp_case_dir("defaults");
let path = state_path(&root);
fs::create_dir_all(path.parent().expect("state parent")).expect("create state parent");
Expand All @@ -284,6 +308,7 @@ mod tests {

#[test]
fn write_desktop_setting_preserves_unknown_fields() {
let _root_guard = EnvVarGuard::clear(crate::ASTRBOT_ROOT_ENV);
let root = create_temp_case_dir("preserve");
let path = state_path(&root);
fs::create_dir_all(path.parent().expect("state parent")).expect("create state parent");
Expand Down Expand Up @@ -315,6 +340,7 @@ mod tests {

#[test]
fn invalid_state_falls_back_to_defaults_and_write_resets_object() {
let _root_guard = EnvVarGuard::clear(crate::ASTRBOT_ROOT_ENV);
let root = create_temp_case_dir("invalid");
let path = state_path(&root);
fs::create_dir_all(path.parent().expect("state parent")).expect("create state parent");
Expand All @@ -339,6 +365,7 @@ mod tests {

#[test]
fn invalid_state_is_rewritten_to_defaults_on_read() {
let _root_guard = EnvVarGuard::clear(crate::ASTRBOT_ROOT_ENV);
let root = create_temp_case_dir("invalid-read-reset");
let path = state_path(&root);
fs::create_dir_all(path.parent().expect("state parent")).expect("create state parent");
Expand Down