fix: respect logging output and ignore discovery cancel

[sg-shag]

Summary

This PR addresses two operational issues:

1. Respect logging.output from config during startup logger initialization.
2. Treat discovery context.Canceled during coordinated shutdown/restart as a normal lifecycle event, not endpoint failure.

Changes

• main.go

• Load config before logger setup so logging config is available at logger init.

• Build logger config from loaded config values.

• internal/logger/logger.go

• Add OutputPath to logger config.

• Resolve output file path and ensure directory creation against the final output target.

• internal/adapter/discovery/service.go

• Short-circuit context.Canceled in discovery error handling.

• Skip failure-count escalation for expected shutdown cancellation.

• internal/adapter/discovery/service_test.go

• Add TestDiscoverEndpointContextCanceledDoesNotCountAsFailure.

Validation

• go test ./internal/adapter/discovery ./internal/logger

Related

• Closes Respect logging.output at startup and avoid counting discovery cancellation as endpoint failure #179

[thushan]

@CodeRabbit can you review, I think this may break the output: <file> in config.yaml and the OLLA_LOG_LEVEL bits.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR diff for the files in question
gh pr diff 180 --repo thushan/olla

Length of output: 6466

---

🏁 Script executed:

#!/bin/bash
# Get the current state of main.go, logger.go
cat main.go
echo "---SEPARATOR---"
cat internal/logger/logger.go
echo "---SEPARATOR---"
# Look for config struct definitions related to logging
rg -n "LogLevel\|OutputPath\|OLLA_LOG\|logging\|output" --type go -C3 internal/logger/
echo "---SEPARATOR---"
# Find config struct
rg -n "LogLevel\|OutputPath\|Logging\b" --type go -C3 internal/config/ 2>/dev/null || rg -n "LogLevel\|OutputPath\|Logging\b" --type go -C3

Length of output: 15832

---

🏁 Script executed:

#!/bin/bash
# Check config yaml files for logging output examples
fd -e yaml -e yml | xargs grep -l "log\|output" 2>/dev/null | head -20
echo "---SEPARATOR---"
fd -e yaml -e yml | head -20 | xargs cat 2>/dev/null

Length of output: 107381

---

🏁 Script executed:

#!/bin/bash
# Find and show the config struct with Logging fields
rg -n "Logging\|LoggingConfig\|type.*Config\b" --type go -C5 internal/config/
echo "---"
# Check if there's an env.IsSet or similar function that could help with precedence
rg -n "GetEnv\|IsSet\|IsEnvSet" --type go internal/env/ 2>/dev/null || fd -e go . internal/env/ | xargs cat

Length of output: 995

---

---

@thushan Your suspicion is correct — the PR has two precedence bugs in buildLoggerConfig.

---

Bug 1 — OLLA_LOG_LEVEL is silently overridden by logging.level in config

// env var is read first...
lcfg := &logger.Config{
    Level: env.GetEnvOrDefault("OLLA_LOG_LEVEL", DefaultLoggerLevel),
    ...
}
// ...then config file unconditionally overwrites it
if cfg.Logging.Level != "" {
    lcfg.Level = cfg.Logging.Level  // ← wins even when OLLA_LOG_LEVEL was explicitly set
}

If someone runs with OLLA_LOG_LEVEL=debug but config.yaml has logging.level: info, the logger will be info. Env vars should take precedence over config file values.

Fix:

if cfg.Logging.Level != "" && os.Getenv("OLLA_LOG_LEVEL") == "" {
    lcfg.Level = cfg.Logging.Level
}

---

Bug 2 — Empty logging.output disables file logging, overriding OLLA_FILE_OUTPUT

// OLLA_FILE_OUTPUT (default=true) is read first...
lcfg := &logger.Config{
    FileOutput: env.GetEnvBoolOrDefault("OLLA_FILE_OUTPUT", DefaultFileOutput),
    ...
}
// ...then the switch hits the "" case when output isn't set in config at all
output := strings.TrimSpace(cfg.Logging.Output)
switch strings.ToLower(output) {
case "", "stdout":          // ← "" means "not configured in YAML", not "stdout"
    lcfg.FileOutput = false // ← kills file logging even if OLLA_FILE_OUTPUT=true

Any user who doesn't set logging.output in their config will have file logging silently disabled, regardless of OLLA_FILE_OUTPUT.

Fix: Only apply the switch when output is actually configured:

output := strings.TrimSpace(cfg.Logging.Output)
if output != "" {
    switch strings.ToLower(output) {
    case "stdout", "stderr":
        lcfg.FileOutput = false
        lcfg.OutputPath = ""
    default:
        lcfg.FileOutput = true
        if filepath.IsAbs(output) {
            lcfg.OutputPath = output
            lcfg.LogDir = filepath.Dir(output)
        } else {
            lcfg.OutputPath = filepath.Clean(output)
            lcfg.LogDir = filepath.Dir(lcfg.OutputPath)
        }
    }
}

---

Also worth adding an env.IsSet(key string) bool helper to the env package to make the intent of Bug 1's fix explicit rather than relying on os.Getenv() == "" inline.

[thushan]

@coderabbitai can you do the review for full context.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== PR DIFF ==="
gh pr diff 180 --repo thushan/olla

echo "=== main.go ==="
cat main.go

echo "=== internal/logger/logger.go ==="
cat internal/logger/logger.go

echo "=== internal/adapter/discovery/service.go ==="
cat internal/adapter/discovery/service.go

echo "=== internal/adapter/discovery/service_test.go ==="
cat internal/adapter/discovery/service_test.go

echo "=== internal/env/ ==="
fd -e go . internal/env/ | xargs cat

echo "=== internal/config/ ==="
fd -e go . internal/config/ | xargs cat

Length of output: 137888

---

🏁 Script executed:

#!/bin/bash
# Check DiscoveryError and NetworkError Unwrap implementations
rg -n "func.*DiscoveryError.*Unwrap\|func.*NetworkError.*Unwrap\|type DiscoveryError\|type NetworkError\|type ParseError" --type go internal/adapter/discovery/

Length of output: 477

---

🏁 Script executed:

#!/bin/bash
rg -n "type DiscoveryError\|type NetworkError\|type ParseError\|func.*Unwrap" --type go internal/adapter/discovery/
echo "---"
fd -e go . internal/adapter/discovery/ | xargs grep -n "Unwrap\|DiscoveryError\|NetworkError\|ParseError" | head -60

Length of output: 5885

---

---

@thushan Here is the full review of PR #180.

---

Overall Assessment

The discovery context.Canceled fix is correct and well-tested — errors.Is chains properly through DiscoveryError → NetworkError → context.Canceled thanks to both types implementing Unwrap(). ✅

The config-before-logger reordering is also the right move. ✅

However the buildLoggerConfig output/level logic introduces two precedence bugs that effectively make OLLA_LOG_LEVEL and OLLA_FILE_OUTPUT silently ineffective.

---

Bug 1 — OLLA_LOG_LEVEL is always ignored

DefaultConfig() unconditionally seeds Logging.Level = "info", so cfg.Logging.Level != "" is always true and will always overwrite whatever OLLA_LOG_LEVEL resolved to:

// DefaultConfig() in config.go — always "info"
Logging: LoggingConfig{
    Level:  "info",   // ← never empty
    ...
}

// buildLoggerConfig — this branch always fires
if cfg.Logging.Level != "" {          // always true
    lcfg.Level = cfg.Logging.Level    // silently clobbers OLLA_LOG_LEVEL
}

There is also a naming collision worth noting: applyEnvOverrides maps OLLA_LOGGING_LEVEL → cfg.Logging.Level, while buildLoggerConfig reads OLLA_LOG_LEVEL. After this PR, OLLA_LOG_LEVEL has no effect; users would have to know to use OLLA_LOGGING_LEVEL instead — but that's undocumented and the two names are easy to confuse.

---

Bug 2 — OLLA_FILE_OUTPUT=true is always overridden

DefaultConfig() seeds Logging.Output = "stdout", so the switch always hits the "stdout" arm and sets FileOutput = false, regardless of the env var:

// DefaultConfig() — always "stdout"
Logging: LoggingConfig{
    Output: "stdout",  // ← always non-empty, always "stdout"
}

// buildLoggerConfig — always fires, always disables file output
output := strings.TrimSpace(cfg.Logging.Output) // = "stdout"
switch strings.ToLower(output) {
case "", "stdout":             // ← always hits here
    lcfg.FileOutput = false    // ← kills OLLA_FILE_OUTPUT=true

DefaultFileOutput = true in main.go is now effectively dead code for any user who doesn't explicitly set output: <filepath> in their YAML.

---

Recommended Fix

The cleanest fix with minimal churn is to change the seeded defaults in DefaultConfig() to empty strings, which the buildLoggerConfig switch already treats as "not configured in YAML, defer to env vars":

// internal/config/types.go (DefaultConfig)
Logging: LoggingConfig{
    Level:  "",  // empty = "not set by user YAML, use OLLA_LOG_LEVEL / fallback"
    Format: "json",
    Output: "",  // empty = "not set by user YAML, use OLLA_FILE_OUTPUT / fallback"
},

This makes the switch in buildLoggerConfig correct as written — case "": already means "stdout, disable file output". The only follow-up is updating TestDefaultConfig which currently asserts cfg.Logging.Level == "info".

Alternatively, if you want to keep the defaults as documentation, guard with the env var:

if cfg.Logging.Level != "" && os.Getenv("OLLA_LOG_LEVEL") == "" {
    lcfg.Level = cfg.Logging.Level
}

output := strings.TrimSpace(cfg.Logging.Output)
if output != "" {  // only apply when explicitly configured
    switch strings.ToLower(output) {
    ...
    }
}

---

Minor Items

Redundant context.Canceled check — discoverConcurrently already suppressed context.Canceled errors before this PR. The new check in handleDiscoveryError is still needed (it prevents the failure counter increment), so both are correct. Not a bug, just worth a comment explaining each serves a different purpose.

OLLA_LOG_LEVEL vs OLLA_LOGGING_LEVEL — these two env var names now do different things (one is silently ignored, one works). Consider either deprecating OLLA_LOG_LEVEL explicitly or having buildLoggerConfig also check OLLA_LOGGING_LEVEL so both names work.