Skip to content

added flag for clearing log files at restart #3203

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+29 −1
Open

added flag for clearing log files at restart #3203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a252ba
added flag for clearing log files at restart
seun-ja Jul 18, 2025
9d09742
refactor: renaming + login
seun-ja Jul 21, 2025
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
18 changes: 17 additions & 1 deletion crates/runtime-config/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ where
local_app_dir: Option<PathBuf>,
provided_state_dir: UserProvidedPath,
provided_log_dir: UserProvidedPath,
refresh_logs: bool,
) -> anyhow::Result<Self> {
let toml = match runtime_config_path {
Some(runtime_config_path) => {
Expand All @@ -120,13 +121,14 @@ where
let toml_resolver =
TomlResolver::new(&toml, local_app_dir, provided_state_dir, provided_log_dir);

Self::new(toml_resolver, runtime_config_path)
Self::new(toml_resolver, runtime_config_path, refresh_logs)
}

/// Creates a new resolved runtime configuration from a TOML table.
pub fn new(
toml_resolver: TomlResolver<'_>,
runtime_config_path: Option<&Path>,
refresh_logs: bool,
) -> anyhow::Result<Self> {
let runtime_config_dir = runtime_config_path
.and_then(Path::parent)
Expand All @@ -141,6 +143,11 @@ where

let toml = toml_resolver.toml();
let log_dir = toml_resolver.log_dir()?;

if refresh_logs {
let _ = Self::resolve_log_dir(&log_dir);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorry to be a nag but once again this is kind of jamming the delete code in somewhere that yes, it's convenient right now because this code knows what the log directory is, but will be surprising to anyone debugging in a few months. This is the constructor for the runtime configuration. It has nothing to do with creating or deleting directories.

What I really think we should do before diving into writing code is some design:

  • What is the desired behaviour of the feature?
  • What considerations do we have to think about when implementing the feature?
  • What code object(s) are responsible for the behaviour?

Let me give some examples.

  • The current behaviour is to delete the logs directory. Suppose, for some reason known only to myself, I set the logs directory to be my application directory (spin up --log-dir . --refresh-logs). Oops, Spin just deleted my application directory. Should the behaviour be to delete individual log files rather than the whole directory?
  • The current implementation deletes the directory as part of trigger startup. That means that if I have a multi-trigger application, each trigger will perform a directory delete as it starts. Suppose one trigger fails early, and another trigger starts slowly. The second trigger blew away my logs for the failure.
  • The existing log file creation happens in ComponentStdioWriter, in the spin_trigger::cli::stdio module. Does deleting existing logs fit nicely with the existing responsibilities of that type or module? I'm not saying it does, I'm just giving it as an example of a place I'd look at before the runtime-config subsystem.

I don't know if there are other factors to consider because I've not done any design on this, I just feel like this might run smoother if you were gathering feedback on a holistic proposal rather than trying to make local tweaks in response to a stream of comments. That said, this is your project and if you feel more comfortable working at the code level then no worries!

}

let max_instance_memory = toml_resolver.max_instance_memory()?;

let source = TomlRuntimeConfigSource::new(
Expand All @@ -166,6 +173,14 @@ where
})
}

fn resolve_log_dir(log_dir: &Option<PathBuf>) -> std::io::Result<()> {
if let Some(path) = log_dir {
std::fs::remove_dir_all(path)
} else {
Ok(())
}
}

/// The fully resolved state directory.
pub fn state_dir(&self) -> Option<PathBuf> {
self.state_dir.clone()
Expand Down Expand Up @@ -493,6 +508,7 @@ mod tests {
ResolvedRuntimeConfig::<TestFactorsRuntimeConfig>::new(
toml_resolver(&toml),
Some(path.as_ref()),
false,
)
}
};
Expand Down
1 change: 1 addition & 0 deletions crates/runtime-factors/src/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ impl RuntimeFactorsBuilder for FactorsBuilder {
config.local_app_dir.clone().map(PathBuf::from),
config.state_dir.clone(),
config.log_dir.clone(),
config.refresh_logs,
)?;

runtime_config.summarize(config.runtime_config_file.as_deref());
Expand Down
11 changes: 11 additions & 0 deletions crates/trigger/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub use stdio::StdioLoggingExecutorHooks;
pub use summary::{KeyValueDefaultStoreSummaryHook, SqliteDefaultStoreSummaryHook};

pub const APP_LOG_DIR: &str = "APP_LOG_DIR";
pub const SPIN_REFRESH_LOGS: &str = "SPIN_REFRESH_LOGS";
pub const DISABLE_WASMTIME_CACHE: &str = "DISABLE_WASMTIME_CACHE";
pub const FOLLOW_LOG_OPT: &str = "FOLLOW_ID";
pub const WASMTIME_CACHE_FILE: &str = "WASMTIME_CACHE_FILE";
Expand Down Expand Up @@ -54,6 +55,13 @@ pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilde
)]
pub log: Option<PathBuf>,

/// If set, Spin deletes the log files before starting the application.
#[clap(
name = SPIN_REFRESH_LOGS,
long = "refresh-logs",
)]
pub refresh_logs: bool,

/// Disable Wasmtime cache.
#[clap(
name = DISABLE_WASMTIME_CACHE,
Expand Down Expand Up @@ -139,6 +147,8 @@ pub struct FactorsConfig {
pub follow_components: FollowComponents,
/// Log directory for component stdout/stderr.
pub log_dir: UserProvidedPath,
/// Whether to refresh logs when restarted.
pub refresh_logs: bool,
}

/// An empty implementation of clap::Args to be used as TriggerExecutor::RunConfig
Expand Down Expand Up @@ -220,6 +230,7 @@ impl<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder> FactorsTriggerCommand<T,
local_app_dir: local_app_dir.clone(),
follow_components,
log_dir,
refresh_logs: self.refresh_logs,
};

let run_fut = builder
Expand Down