log: file logging silently disabled when `--log.file.max-files` is unset, even with explicit `--log.file.directory`/`--log.file.filter`

[nekomoto911]

Summary

LogArgs::effective_log_file_max_files() resolves to 0 — which means "file logging disabled" — whenever --log.file.max-files is not passed explicitly. The node-subcommand default of 5 is applied by a separate, opt-in call (apply_node_defaults()), and the contract that callers must invoke it is enforced only by a doc comment.

When that call is missed, file logging is disabled completely and silently: no log file, no log directory, and no warning — even when the operator explicitly passed --log.file.directory and --log.file.filter. That combination (explicit file options + total silence) is the defect I'd like to raise.

greth's own binary is correct. The problem is that the correctness lives entirely outside LogArgs, so any embedder that constructs LogArgs itself inherits a silent, total loss of file logging.

Field report

An 8-node gravity-sdk cluster pinned at greth 4b6aa739e22b899fca6de4be852f0f3edf1eec00 ran for over a day with zero execution-layer log files, launched with:

--log.file.directory=/home/gravity/longtest-cluster/<node>/execution_logs/
--log.file.filter=info
--log.stdout.filter=error

Observed state:

• the target directory was never created — its mtime still equalled its ctime from cluster-deploy time;
• no reth.log* anywhere on the box;
• the process held no file descriptor anywhere under that path;
• no warning, no error, nothing on stdout.

Nothing distinguished this from a healthy run. The loss was found incidentally, a day later.

Code chain (verified at 4b6aa739e2)

crates/node/core/src/args/log.rs:52-57 — no clap default; the documented default is prose only:

/// The maximum amount of log files that will be stored. If set to 0, background file logging
/// is disabled.
///
/// Default: 5 for `node` command, 0 for non-node utility subcommands.
#[arg(long = "log.file.max-files", value_name = "COUNT", global = true)]
pub log_file_max_files: Option<usize>,

crates/node/core/src/args/log.rs:150-152 — unset silently means disabled:

pub fn effective_log_file_max_files(&self) -> usize {
    self.log_file_max_files.unwrap_or(0)
}

crates/node/core/src/args/log.rs:156-160 — the node default is a separate opt-in step (apply_node_defaults), documented at :148-149 as something callers should do before init_tracing.

crates/node/core/src/args/log.rs:213-217 — the file layer is gated on it, with no else branch:

if self.effective_log_file_max_files() > 0 {
    let info = self.file_info();
    let file = self.layer_info(self.log_file_format, self.log_file_filter.clone(), false);
    tracer = tracer.with_file(file, info);
}

Directory creation hangs off that same skipped path, which is why nothing exists at all rather than an empty directory:

• crates/tracing/src/lib.rs:293 — layers.file(...) runs only when self.file is Some
• crates/tracing/src/layers.rs:176 — file_info.create_log_writer()?
• crates/tracing/src/layers.rs:324-327 — create_log_writer() → create_log_dir()
• crates/tracing/src/layers.rs:314-321 — create_log_dir() → std::fs::create_dir_all

greth's own entrypoint does the right thing — crates/ethereum/cli/src/app.rs:123-128:

// Apply node-specific log defaults before initializing tracing
if matches!(self.cli.command, Commands::Node(_)) {
    self.cli.logs.apply_node_defaults();
}

self.init_tracing(&runner)?;

This is a behavioural change

The previous SDK pin, b49b4864aeaa3c35c6871a77d7133bb9486edbf1, had no such hazard — crates/node/core/src/args/log.rs:48-49:

#[arg(long = "log.file.max-files", value_name = "COUNT", global = true, default_value_t = 5)]
pub log_file_max_files: usize,

so the gate at :125 (if self.log_file_max_files > 0) was always true and file logging was always on, regardless of how the args were constructed. The flip arrived with 3bd8705cb6 (feat: merge reth v2.3.0 (#396)), which is an ancestor of 4b6aa739e2.

The downstream caller fix is gravity-sdk's, not greth's

To be explicit: gravity-sdk has its own hand-copied CLI entrypoint (bin/gravity_node/src/cli.rs) that does not route through greth's app.rs, and it calls self.logs.init_tracing() directly without ever calling apply_node_defaults(). That is a gravity-sdk bug and is being fixed downstream separately. No fix is being requested here for it.

The ask here is only about the silent failure mode, which is what turned a one-line downstream omission into a day of unnoticed total log loss.

Suggested options

Listing possibilities rather than proposing a specific patch — maintainers are better placed to pick:

1. Warn at init. In init_tracing_with_layers, if log_file_directory or log_file_filter was explicitly provided (clap can report this via ArgMatches::value_source) but effective_log_file_max_files() == 0, emit a warn! that file logging is disabled. Cheapest option, and it alone would have caught this within seconds.
2. Don't let "unset" silently mean "disabled" when file options were explicit. Have effective_log_file_max_files() fall back to DEFAULT_MAX_LOG_FILES_NODE when the user explicitly asked for file logging, reserving 0 for an explicit --log.file.max-files=0.
3. Surface it in the return value. Report whether the file layer was installed via TracingGuards (or the init_tracing result) so an embedder can assert on it, turning the doc-comment contract into something checkable.

Any one of these would be sufficient; (1) is the smallest.

Upstream

I checked: crates/node/core/src/args/log.rs is byte-identical between Galxe/gravity-reth@4b6aa739e2 and paradigmxyz/reth@a20cef633a (reth/main at time of writing), so upstream has the same shape. It was introduced upstream by 5c83eb0b06 — "feat(log): disable file logging by default for non-node commands (#21521)". Upstream's own crates/ethereum/cli/src/app.rs:132-133 applies the same Commands::Node(_) gating, so upstream's binary is equally fine and equally reliant on the caller. If this is worth fixing, it may be worth raising upstream too rather than carrying a local divergence.