Skip to content

Replace manual logger close calls in postRun with CloseAllLoggers()#4881

Merged
lpcox merged 2 commits intomainfrom
copilot/fix-duplicate-logger-close-calls
Apr 30, 2026
Merged

Replace manual logger close calls in postRun with CloseAllLoggers()#4881
lpcox merged 2 commits intomainfrom
copilot/fix-duplicate-logger-close-calls

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 30, 2026

postRun() manually called five individual CloseXxx() functions, duplicating the logic already consolidated in logger.CloseAllLoggers() — and silently discarding all close errors. Any new logger added to CloseAllLoggers() would be silently skipped at shutdown.

Changes

  • internal/cmd/root.go: Replace five individual close calls with logger.CloseAllLoggers() and surface errors via log.Printf
// Before
logger.CloseMarkdownLogger()
logger.CloseJSONLLogger()
logger.CloseServerFileLogger()
logger.CloseToolsLogger()
logger.CloseGlobalLogger()

// After
if err := logger.CloseAllLoggers(); err != nil {
    log.Printf("Warning: error closing loggers: %v", err)
}

Shutdown cleanup now mirrors the single-call pattern already used for initialisation (InitGatewayLoggers) and will automatically include any future loggers added to CloseAllLoggers().

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • example.com
    • Triggering command: /tmp/go-build361366619/b513/launcher.test /tmp/go-build361366619/b513/launcher.test -test.testlogfile=/tmp/go-build361366619/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I g_.a -I x_amd64/vet --gdwarf-5 grpcutil -o x_amd64/vet -W .cfg /tmp/go-build3371211061/b151/ x_amd64/vet 1211061/b314/ --gdwarf2 ctor x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build361366619/b495/config.test /tmp/go-build361366619/b495/config.test -test.testlogfile=/tmp/go-build361366619/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build361366619/b396/vet.cfg /idna/go118.go /idna/idna10.0.0.go x_amd64/vet --gdwarf-5 --64 ut-1387901200.c x_amd64/vet --de�� g_.a --debug-prefix-m-ifaceassert x_amd64/vet -I dns -I x_amd64/vet (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build361366619/b513/launcher.test /tmp/go-build361366619/b513/launcher.test -test.testlogfile=/tmp/go-build361366619/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I g_.a -I x_amd64/vet --gdwarf-5 grpcutil -o x_amd64/vet -W .cfg /tmp/go-build3371211061/b151/ x_amd64/vet 1211061/b314/ --gdwarf2 ctor x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build361366619/b513/launcher.test /tmp/go-build361366619/b513/launcher.test -test.testlogfile=/tmp/go-build361366619/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I g_.a -I x_amd64/vet --gdwarf-5 grpcutil -o x_amd64/vet -W .cfg /tmp/go-build3371211061/b151/ x_amd64/vet 1211061/b314/ --gdwarf2 ctor x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build361366619/b522/mcp.test /tmp/go-build361366619/b522/mcp.test -test.testlogfile=/tmp/go-build361366619/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� olang.org/grpc@v-errorsas /tmp/go-build337-ifaceassert x_amd64/vet . contextprotocol//usr/bin/runc --64 x_amd64/vet .cfg�� 1211061/b391/_pkg_.a pkg/mod/go.opentelemetry.io/otel@v1.43.0/baggage/context.go x_amd64/vet --gdwarf-5 g/protobuf/encodinfo -o x_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Fix duplicate logger close calls in postRun function Replace manual logger close calls in postRun with CloseAllLoggers() Apr 30, 2026
Copilot AI requested a review from lpcox April 30, 2026 14:04
@lpcox lpcox marked this pull request as ready for review April 30, 2026 14:47
Copilot AI review requested due to automatic review settings April 30, 2026 14:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR simplifies CLI shutdown cleanup by using the centralized logger.CloseAllLoggers() helper in postRun(), ensuring all configured log sinks are closed consistently and close errors are surfaced.

Changes:

  • Replace five individual CloseXxxLogger() calls in postRun() with a single logger.CloseAllLoggers() call.
  • Log a warning to stderr (via log.Printf) if any logger close returns an error.
Show a summary per file
File Description
internal/cmd/root.go Switches shutdown cleanup to logger.CloseAllLoggers() and reports close errors via log.Printf.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[duplicate-code] Duplicate Code Pattern: Manual Logger Close Calls Instead of CloseAllLoggers()

3 participants