diff --git a/CLAUDE.md b/CLAUDE.md index ff94487..10f759d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,21 +38,21 @@ go mod vendor Single-binary Go application in `app/` package (uses `package main`, not a library). -**Control flow**: `main()` → `do()` → `runEventLoop()` which listens on `EventNotif.Channel()` for container events and creates/destroys `LogStreamer` instances. +**Control flow**: `main()` → `do()` → `runEventLoop()` which listens on `EventNotif.Channel()` for container events and creates/destroys `LogStreamer` instances. Exits on context cancellation or when events channel is closed (e.g., docker event listener failure). Uses `closeStreamer` helper for resource cleanup. ### Packages -- **`app/`** — entry point, CLI options (`go-flags`), event loop, log writer factory (`makeLogWriters`). Wires discovery events to log streamers. Creates `MultiWriter` combining file (lumberjack) and syslog destinations. -- **`app/discovery/`** — `EventNotif` watches Docker daemon for container start/stop events via `go-dockerclient`. Emits `Event` structs on a channel. Handles include/exclude filtering by name lists and regex patterns. Extracts group name from image path. +- **`app/`** — entry point, CLI options (`go-flags`), event loop, log writer factory (`makeLogWriters` returns `logWriter, errWriter, error`). Wires discovery events to log streamers. Creates `MultiWriter` combining file (lumberjack) and syslog destinations. Uses `writeNopCloser` wrapper to prevent double-close when syslog writer is shared between log and err MultiWriters. +- **`app/discovery/`** — `EventNotif` watches Docker daemon for container start/stop events via `go-dockerclient`. `NewEventNotif(client, EventNotifOpts)` accepts filtering options via struct. Emits `Event` structs on a channel; closes the channel on listener failure. Handles include/exclude filtering by name lists and regex patterns. Extracts group name from image path. - **`app/logger/`** — `LogStreamer` attaches to a container's log stream (blocking `Logs()` call in a goroutine). `MultiWriter` fans out writes to multiple `io.WriteCloser` destinations, optionally wrapping in JSON envelope. - **`app/syslog/`** — platform-specific syslog writer. Build-tagged: real implementation on unix, stub on windows. ### Key Patterns - **Docker client interface**: `discovery.DockerClient` and `logger.LogClient` are consumer-side interfaces wrapping `go-dockerclient`. Mocks generated with `moq` into `mocks/` subdirectories. -- **LogStreamer lifecycle**: `Go(ctx)` starts streaming in a goroutine, `Close()` cancels context and waits, `Wait()` blocks on `ctx.Done()`. Has retry logic for Docker EOF errors. +- **LogStreamer lifecycle**: `Go(ctx)` starts streaming in a goroutine, `Close()` cancels context and waits, `Wait()` blocks on `ctx.Done()`, `Err()` retrieves the error (if any) after `Wait()` returns. Has retry logic for Docker EOF errors. - **MultiWriter**: ignores individual write errors unless all writers fail. `Close()` collects errors via `go-multierror`. -- **Container filtering**: supports name lists (`--exclude`/`--include`) and regex patterns (`--exclude-pattern`/`--include-pattern`), mutually exclusive within each group. +- **Container filtering**: supports name lists (`--exclude`/`--include`) and regex patterns (`--exclude-pattern`/`--include-pattern`), mutually exclusive within each group and across groups (e.g., `--include` + `--exclude-pattern` is also invalid). ## Dependencies @@ -68,5 +68,6 @@ Single-binary Go application in `app/` package (uses `package main`, not a libra - Tests use `moq`-generated mocks in `mocks/` subdirectories - Most `app/` tests use mocks; no live Docker needed. `Test_Do` requires a live Docker daemon but is skipped unless `TEST_DOCKER` env var is set -- Channel-based synchronization preferred over `time.Sleep` for race-free tests +- Channel-based synchronization preferred over `time.Sleep` for race-free tests; use `require.Eventually` with condition checks for async operations - Uses `t.TempDir()` for temporary files and `t.Context()` for test contexts +- Sentinel event technique: send a known event after the one being tested, wait for it via `require.Eventually` to confirm both events were processed diff --git a/README.md b/README.md index 657a420..cd76215 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ the `-t` option and configured with a logging driver that works with docker logs (journald and json-file). It can forward both stdout and stderr of containers to local, rotated files and/or to remote syslog. -_note: [dkll](https://github.com/umputun/dkll) inlcudes all functionality of docker-logger, but adds server and cli client_ +_note: [dkll](https://github.com/umputun/dkll) includes all functionality of docker-logger, but adds server and cli client_ ## Install @@ -30,13 +30,18 @@ All changes can be done via container's environment in `docker-compose.yml` or w | `--include-pattern` | `INCLUDE_PATTERN` | | only include container names matching a regex | | `--exclude-pattern` | `EXCLUDE_PATTERN` | | only exclude container names matching a regex | | | `TIME_ZONE` | UTC | time zone for container | +| `--loc` | `LOG_FILES_LOC` | logs | log files location | +| `--syslog-prefix` | `SYSLOG_PREFIX` | docker/ | syslog prefix | | `--json`, `-j` | `JSON` | false | output formatted as JSON | +| `--dbg` | `DEBUG` | false | debug mode | - at least one of destinations (`files` or `syslog`) should be allowed - location of log files can be mapped to host via `volume`, ex: `- ./logs:/srv/logs` (see `docker-compose.yml`) -- both `--exclude` and `--include` flags are optional and mutually exclusive, i.e. if `--exclude` defined `--include` not allowed, and vise versa. -- both `--include` and `--include-pattern` flags are optional and mutually exclusive, i.e. if `--include` defined `--include-pattern` not allowed, and vise versa. +- both `--exclude` and `--include` flags are optional and mutually exclusive, i.e. if `--exclude` defined `--include` not allowed, and vice versa. +- both `--include` and `--include-pattern` flags are optional and mutually exclusive, i.e. if `--include` defined `--include-pattern` not allowed, and vice versa. +- both `--exclude` and `--exclude-pattern` flags are optional and mutually exclusive, i.e. if `--exclude` defined `--exclude-pattern` not allowed, and vice versa. +- cross-kind combinations are also mutually exclusive: `--include` + `--exclude-pattern`, `--include-pattern` + `--exclude`, and `--include-pattern` + `--exclude-pattern` are not allowed. ## Build from the source diff --git a/app/discovery/events.go b/app/discovery/events.go index 8339529..648e181 100644 --- a/app/discovery/events.go +++ b/app/discovery/events.go @@ -19,6 +19,7 @@ type EventNotif struct { includesRegexp *regexp.Regexp excludesRegexp *regexp.Regexp eventsCh chan Event + listenerErr chan error // communicates activate() failure back to the caller } // Event is simplified docker.APIEvents for containers only, exposed to caller @@ -40,23 +41,31 @@ type DockerClient interface { var reGroup = regexp.MustCompile(`/(.*?)/`) +// EventNotifOpts contains options for NewEventNotif +type EventNotifOpts struct { + Excludes []string + Includes []string + IncludesPattern string + ExcludesPattern string +} + // NewEventNotif makes EventNotif publishing all changes to eventsCh -func NewEventNotif(dockerClient DockerClient, excludes, includes []string, includesPattern, excludesPattern string) (*EventNotif, error) { +func NewEventNotif(dockerClient DockerClient, opts EventNotifOpts) (*EventNotif, error) { log.Printf("[DEBUG] create events notif, excludes: %+v, includes: %+v, includesPattern: %+v, excludesPattern: %+v", - excludes, includes, includesPattern, excludesPattern) + opts.Excludes, opts.Includes, opts.IncludesPattern, opts.ExcludesPattern) var err error var includesRe *regexp.Regexp - if includesPattern != "" { - includesRe, err = regexp.Compile(includesPattern) + if opts.IncludesPattern != "" { + includesRe, err = regexp.Compile(opts.IncludesPattern) if err != nil { return nil, errors.Wrap(err, "failed to compile includesPattern") } } var excludesRe *regexp.Regexp - if excludesPattern != "" { - excludesRe, err = regexp.Compile(excludesPattern) + if opts.ExcludesPattern != "" { + excludesRe, err = regexp.Compile(opts.ExcludesPattern) if err != nil { return nil, errors.Wrap(err, "failed to compile excludesPattern") } @@ -64,11 +73,12 @@ func NewEventNotif(dockerClient DockerClient, excludes, includes []string, inclu res := EventNotif{ dockerClient: dockerClient, - excludes: excludes, - includes: includes, + excludes: opts.Excludes, + includes: opts.Includes, includesRegexp: includesRe, excludesRegexp: excludesRe, eventsCh: make(chan Event, 100), + listenerErr: make(chan error, 1), } // first get all currently running containers @@ -88,12 +98,22 @@ func (e *EventNotif) Channel() (res <-chan Event) { return e.eventsCh } +// Err returns a channel that receives an error if the event listener fails to start. +// the channel is buffered (size 1) and will receive at most one error. +func (e *EventNotif) Err() <-chan error { + return e.listenerErr +} + // activate starts blocking listener for all docker events -// filters everything except "container" type, detects stop/start events and publishes to eventsCh +// filters everything except "container" type, detects stop/start events and publishes to eventsCh. +// on failure or channel close, it closes eventsCh to signal consumers. func (e *EventNotif) activate(client DockerClient) { dockerEventsCh := make(chan *docker.APIEvents) if err := client.AddEventListener(dockerEventsCh); err != nil { - log.Fatalf("[ERROR] can't add even listener, %v", err) + log.Printf("[ERROR] can't add event listener, %v", err) + e.listenerErr <- errors.Wrap(err, "can't add event listener") + close(e.eventsCh) + return } upStatuses := []string{"start", "restart"} @@ -116,17 +136,23 @@ func (e *EventNotif) activate(client DockerClient) { continue } + ts := time.Unix(0, dockerEvent.TimeNano) + if dockerEvent.TimeNano == 0 { + ts = time.Unix(dockerEvent.Time, 0) + } + event := Event{ ContainerID: dockerEvent.Actor.ID, ContainerName: containerName, Status: slices.Contains(upStatuses, dockerEvent.Status), - TS: time.Unix(dockerEvent.Time/1000, dockerEvent.TimeNano), + TS: ts, Group: e.group(dockerEvent.From), } log.Printf("[INFO] new event %+v", event) e.eventsCh <- event } - log.Fatalf("[ERROR] event listener failed") + log.Printf("[WARN] event listener closed") + close(e.eventsCh) } // emitRunningContainers gets all currently running containers and publishes them as "Status=true" (started) events @@ -138,6 +164,10 @@ func (e *EventNotif) emitRunningContainers() error { log.Printf("[DEBUG] total containers = %d", len(containers)) for _, c := range containers { + if len(c.Names) == 0 { + log.Printf("[WARN] container %s has no names, skipped", c.ID) + continue + } containerName := strings.TrimPrefix(c.Names[0], "/") if !e.isAllowed(containerName) { log.Printf("[INFO] container %s excluded", containerName) @@ -147,7 +177,7 @@ func (e *EventNotif) emitRunningContainers() error { Status: true, ContainerName: containerName, ContainerID: c.ID, - TS: time.Unix(c.Created/1000, 0), + TS: time.Unix(c.Created, 0), Group: e.group(c.Image), } log.Printf("[DEBUG] running container added, %+v", event) diff --git a/app/discovery/events_test.go b/app/discovery/events_test.go index b7c9a8f..debc8ab 100644 --- a/app/discovery/events_test.go +++ b/app/discovery/events_test.go @@ -3,6 +3,7 @@ package discovery import ( "errors" "testing" + "time" dockerclient "github.com/fsouza/go-dockerclient" "github.com/stretchr/testify/assert" @@ -32,7 +33,7 @@ func makeListenerMock() ( func TestEvents(t *testing.T) { mock, getEventsCh := makeListenerMock() - events, err := NewEventNotif(mock, []string{"tst_exclude"}, []string{}, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{Excludes: []string{"tst_exclude"}}) require.NoError(t, err) eventsCh := getEventsCh() @@ -63,7 +64,7 @@ func TestEvents(t *testing.T) { func TestEventsIncludes(t *testing.T) { mock, getEventsCh := makeListenerMock() - events, err := NewEventNotif(mock, []string{}, []string{"tst_included"}, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{Includes: []string{"tst_included"}}) require.NoError(t, err) eventsCh := getEventsCh() @@ -88,10 +89,11 @@ func TestEventsIncludes(t *testing.T) { } func TestEmit(t *testing.T) { + now := time.Now() containers := []dockerclient.APIContainers{ - {ID: "id1", Names: []string{"name1"}, Image: "docker.umputun.com/group1/img:latest"}, - {ID: "id2", Names: []string{"tst_exclude"}, Image: "img:latest"}, - {ID: "id3", Names: []string{"name2"}, Image: "docker.umputun.com/group2/img:latest"}, + {ID: "id1", Names: []string{"name1"}, Image: "docker.umputun.com/group1/img:latest", Created: now.Unix()}, + {ID: "id2", Names: []string{"tst_exclude"}, Image: "img:latest", Created: now.Unix()}, + {ID: "id3", Names: []string{"name2"}, Image: "docker.umputun.com/group2/img:latest", Created: now.Unix()}, } mock := &mocks.DockerClientMock{ @@ -103,18 +105,42 @@ func TestEmit(t *testing.T) { }, } - events, err := NewEventNotif(mock, []string{"tst_exclude"}, []string{}, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{Excludes: []string{"tst_exclude"}}) require.NoError(t, err) ev := <-events.Channel() assert.Equal(t, "name1", ev.ContainerName) assert.True(t, ev.Status, "started") assert.Equal(t, "group1", ev.Group) + assert.WithinDuration(t, now, ev.TS, time.Second, "timestamp should be close to now") ev = <-events.Channel() assert.Equal(t, "name2", ev.ContainerName) assert.True(t, ev.Status, "started") assert.Equal(t, "group2", ev.Group) + assert.WithinDuration(t, now, ev.TS, time.Second, "timestamp should be close to now") +} + +func TestEmitSkipsContainersWithNoNames(t *testing.T) { + containers := []dockerclient.APIContainers{ + {ID: "id1", Names: nil, Image: "img:latest"}, + {ID: "id2", Names: []string{"name2"}, Image: "img:latest", Created: time.Now().Unix()}, + } + + mock := &mocks.DockerClientMock{ + ListContainersFunc: func(opts dockerclient.ListContainersOptions) ([]dockerclient.APIContainers, error) { + return containers, nil + }, + AddEventListenerFunc: func(listener chan<- *dockerclient.APIEvents) error { + return nil + }, + } + + events, err := NewEventNotif(mock, EventNotifOpts{}) + require.NoError(t, err) + + ev := <-events.Channel() + assert.Equal(t, "name2", ev.ContainerName, "container with no names should be skipped") } func TestEmitIncludes(t *testing.T) { @@ -133,7 +159,7 @@ func TestEmitIncludes(t *testing.T) { }, } - events, err := NewEventNotif(mock, []string{}, []string{"tst_include"}, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{Includes: []string{"tst_include"}}) require.NoError(t, err) ev := <-events.Channel() @@ -150,20 +176,20 @@ func TestNewEventNotifWithNils(t *testing.T) { return nil }, } - _, err := NewEventNotif(mock, nil, nil, "", "") + _, err := NewEventNotif(mock, EventNotifOpts{}) require.NoError(t, err) } func TestNewEventNotifInvalidIncludesPattern(t *testing.T) { mock := &mocks.DockerClientMock{} - _, err := NewEventNotif(mock, nil, nil, "[invalid", "") + _, err := NewEventNotif(mock, EventNotifOpts{IncludesPattern: "[invalid"}) require.Error(t, err) assert.Contains(t, err.Error(), "failed to compile includesPattern") } func TestNewEventNotifInvalidExcludesPattern(t *testing.T) { mock := &mocks.DockerClientMock{} - _, err := NewEventNotif(mock, nil, nil, "", "[invalid") + _, err := NewEventNotif(mock, EventNotifOpts{ExcludesPattern: "[invalid"}) require.Error(t, err) assert.Contains(t, err.Error(), "failed to compile excludesPattern") } @@ -174,7 +200,7 @@ func TestNewEventNotifListContainersError(t *testing.T) { return nil, errors.New("connection refused") }, } - _, err := NewEventNotif(mock, nil, nil, "", "") + _, err := NewEventNotif(mock, EventNotifOpts{}) require.Error(t, err) assert.Contains(t, err.Error(), "failed to emit containers") } @@ -182,7 +208,7 @@ func TestNewEventNotifListContainersError(t *testing.T) { func TestActivateFiltersNonContainerEvents(t *testing.T) { mock, getEventsCh := makeListenerMock() - events, err := NewEventNotif(mock, nil, nil, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{}) require.NoError(t, err) eventsCh := getEventsCh() @@ -218,7 +244,7 @@ func TestActivateExcludedContainerFiltered(t *testing.T) { }, } - events, err := NewEventNotif(mock, []string{"excluded"}, nil, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{Excludes: []string{"excluded"}}) require.NoError(t, err) eventsCh := <-ready @@ -241,7 +267,7 @@ func TestActivateExcludedContainerFiltered(t *testing.T) { func TestActivateGroupFromImage(t *testing.T) { mock, getEventsCh := makeListenerMock() - events, err := NewEventNotif(mock, nil, nil, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{}) require.NoError(t, err) eventsCh := getEventsCh() @@ -257,7 +283,7 @@ func TestActivateGroupFromImage(t *testing.T) { func TestActivateAllContainerStatuses(t *testing.T) { mock, getEventsCh := makeListenerMock() - events, err := NewEventNotif(mock, nil, nil, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{}) require.NoError(t, err) eventsCh := getEventsCh() @@ -284,6 +310,44 @@ func TestActivateAllContainerStatuses(t *testing.T) { } } +func TestActivateTimestamp(t *testing.T) { + t.Run("with TimeNano", func(t *testing.T) { + mock, getEventsCh := makeListenerMock() + events, err := NewEventNotif(mock, EventNotifOpts{}) + require.NoError(t, err) + eventsCh := getEventsCh() + + now := time.Now() + ev := &dockerclient.APIEvents{Type: "container", Status: "start"} + ev.Actor.Attributes = map[string]string{"name": "ts_test"} + ev.Actor.ID = "id1" + ev.Time = now.Unix() + ev.TimeNano = now.UnixNano() + eventsCh <- ev + + received := <-events.Channel() + assert.WithinDuration(t, now, received.TS, time.Second, "timestamp should use TimeNano precision") + }) + + t.Run("without TimeNano fallback to Time", func(t *testing.T) { + mock, getEventsCh := makeListenerMock() + events, err := NewEventNotif(mock, EventNotifOpts{}) + require.NoError(t, err) + eventsCh := getEventsCh() + + now := time.Now() + ev := &dockerclient.APIEvents{Type: "container", Status: "start"} + ev.Actor.Attributes = map[string]string{"name": "ts_test2"} + ev.Actor.ID = "id2" + ev.Time = now.Unix() + ev.TimeNano = 0 // simulate older Docker API + eventsCh <- ev + + received := <-events.Channel() + assert.WithinDuration(t, now, received.TS, time.Second, "timestamp should fall back to Time field") + }) +} + func TestIsAllowedExclude(t *testing.T) { mock := &mocks.DockerClientMock{ ListContainersFunc: func(opts dockerclient.ListContainersOptions) ([]dockerclient.APIContainers, error) { @@ -293,7 +357,7 @@ func TestIsAllowedExclude(t *testing.T) { return nil }, } - events, err := NewEventNotif(mock, []string{"tst_exclude"}, nil, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{Excludes: []string{"tst_exclude"}}) require.NoError(t, err) assert.True(t, events.isAllowed("name1")) @@ -309,7 +373,7 @@ func TestIsAllowedExcludePattern(t *testing.T) { return nil }, } - events, err := NewEventNotif(mock, nil, nil, "", "tst_exclude.*") + events, err := NewEventNotif(mock, EventNotifOpts{ExcludesPattern: "tst_exclude.*"}) require.NoError(t, err) assert.True(t, events.isAllowed("tst_include")) @@ -327,7 +391,7 @@ func TestIsAllowedInclude(t *testing.T) { return nil }, } - events, err := NewEventNotif(mock, nil, []string{"tst_include"}, "", "") + events, err := NewEventNotif(mock, EventNotifOpts{Includes: []string{"tst_include"}}) require.NoError(t, err) assert.True(t, events.isAllowed("tst_include")) @@ -344,7 +408,7 @@ func TestIsAllowedIncludePattern(t *testing.T) { return nil }, } - events, err := NewEventNotif(mock, nil, nil, "tst_include.*", "") + events, err := NewEventNotif(mock, EventNotifOpts{IncludesPattern: "tst_include.*"}) require.NoError(t, err) assert.True(t, events.isAllowed("tst_include")) @@ -369,3 +433,45 @@ func TestGroup(t *testing.T) { assert.Equal(t, tt.out, d.group(tt.inp)) } } + +func TestActivateAddEventListenerError(t *testing.T) { + mock := &mocks.DockerClientMock{ + ListContainersFunc: func(opts dockerclient.ListContainersOptions) ([]dockerclient.APIContainers, error) { + return nil, nil + }, + AddEventListenerFunc: func(listener chan<- *dockerclient.APIEvents) error { + return errors.New("listener error") + }, + } + + events, err := NewEventNotif(mock, EventNotifOpts{}) + require.NoError(t, err) + + // eventsCh should be closed because AddEventListener failed + _, ok := <-events.Channel() + assert.False(t, ok, "events channel should be closed on AddEventListener error") +} + +func TestActivateEventChannelClosed(t *testing.T) { + ready := make(chan chan<- *dockerclient.APIEvents, 1) + mock := &mocks.DockerClientMock{ + ListContainersFunc: func(opts dockerclient.ListContainersOptions) ([]dockerclient.APIContainers, error) { + return nil, nil + }, + AddEventListenerFunc: func(listener chan<- *dockerclient.APIEvents) error { + ready <- listener + return nil + }, + } + + events, err := NewEventNotif(mock, EventNotifOpts{}) + require.NoError(t, err) + + // close the docker events channel to simulate docker daemon disconnect + dockerCh := <-ready + close(dockerCh) + + // eventsCh should be closed because the docker events channel was closed + _, ok := <-events.Channel() + assert.False(t, ok, "events channel should be closed when docker events channel closes") +} diff --git a/app/logger/logger.go b/app/logger/logger.go index c84753a..aebba4b 100644 --- a/app/logger/logger.go +++ b/app/logger/logger.go @@ -4,6 +4,7 @@ import ( "context" "io" "strings" + "sync/atomic" "time" docker "github.com/fsouza/go-dockerclient" @@ -28,9 +29,12 @@ type LogStreamer struct { ctx context.Context // nolint:containedctx cancel context.CancelFunc + err atomic.Value } -// Go activates streamer +// Go starts log streaming in a goroutine. It attaches to the container's log stream +// and writes to LogWriter/ErrWriter. Retries on Docker EOF errors with a 1s delay. +// After Wait() returns, use Err() to retrieve the error (if any) from the streaming goroutine. func (l *LogStreamer) Go(ctx context.Context) *LogStreamer { log.Printf("[INFO] start log streamer for %s", l.ContainerName) l.ctx, l.cancel = context.WithCancel(ctx) @@ -61,7 +65,8 @@ func (l *LogStreamer) Go(ctx context.Context) *LogStreamer { break } - if err != nil { + if err != nil && err != context.Canceled { + l.err.Store(err) log.Printf("[WARN] stream from %s terminated with error %v", l.ContainerID, err) return } @@ -71,14 +76,25 @@ func (l *LogStreamer) Go(ctx context.Context) *LogStreamer { return l } -// Close kills streamer +// Err returns the error from the streaming goroutine, if any. Returns nil if the stream +// completed normally or was canceled. Should be called after Wait() returns. +func (l *LogStreamer) Err() error { + v := l.err.Load() + if v == nil { + return nil + } + return v.(error) +} + +// Close cancels the streaming context and waits for the cancellation to propagate. +// The stream goroutine will exit once the Docker client observes the cancellation. func (l *LogStreamer) Close() { l.cancel() l.Wait() log.Printf("[DEBUG] close %s", l.ContainerID) } -// Wait for stream completion +// Wait blocks until the streaming context is canceled, either by Close or parent context. func (l *LogStreamer) Wait() { <-l.ctx.Done() } diff --git a/app/logger/logger_test.go b/app/logger/logger_test.go index 6d0788c..132c138 100644 --- a/app/logger/logger_test.go +++ b/app/logger/logger_test.go @@ -25,13 +25,10 @@ func TestLogStreamer_ContextCancel(t *testing.T) { l := &LogStreamer{ContainerID: "test_id", ContainerName: "test_name", DockerClient: mock} l = l.Go(ctx) - st := time.Now() - go func() { - time.Sleep(100 * time.Millisecond) - cancel() - }() + require.Eventually(t, func() bool { return len(mock.LogsCalls()) >= 1 }, + 5*time.Second, 10*time.Millisecond, "should have called Logs") + cancel() l.Wait() - assert.Less(t, time.Since(st), time.Second, "should terminate quickly after cancel") assert.Len(t, mock.LogsCalls(), 1) } @@ -44,14 +41,10 @@ func TestLogStreamer_Close(t *testing.T) { l := &LogStreamer{ContainerID: "test_id", ContainerName: "test_name", DockerClient: mock} l = l.Go(ctx) - - go func() { - time.Sleep(100 * time.Millisecond) - l.Close() - }() - st := time.Now() - l.Wait() - assert.Less(t, time.Since(st), time.Second, "should terminate after close") + require.Eventually(t, func() bool { return len(mock.LogsCalls()) >= 1 }, + 5*time.Second, 10*time.Millisecond, "should have called Logs") + l.Close() + assert.Len(t, mock.LogsCalls(), 1) } func TestLogStreamer_NormalCompletion(t *testing.T) { @@ -69,7 +62,8 @@ func TestLogStreamer_NormalCompletion(t *testing.T) { ErrWriter: nopWriteCloser{buf}, } l = l.Go(ctx) - time.Sleep(50 * time.Millisecond) + require.Eventually(t, func() bool { return len(mock.LogsCalls()) >= 1 }, + 5*time.Second, 10*time.Millisecond, "should have called Logs") l.Close() assert.Len(t, mock.LogsCalls(), 1) } @@ -111,7 +105,8 @@ func TestLogStreamer_ErrorTermination(t *testing.T) { l := &LogStreamer{ContainerID: "test_id", ContainerName: "test_name", DockerClient: mock} l = l.Go(ctx) - time.Sleep(50 * time.Millisecond) + require.Eventually(t, func() bool { return len(mock.LogsCalls()) >= 1 }, + 5*time.Second, 10*time.Millisecond, "should have called Logs") l.Close() assert.Len(t, mock.LogsCalls(), 1) } @@ -158,7 +153,8 @@ func TestLogStreamer_InitialTail(t *testing.T) { l := &LogStreamer{ContainerID: "test_id", ContainerName: "test_name", DockerClient: mock} l = l.Go(ctx) - time.Sleep(50 * time.Millisecond) + require.Eventually(t, func() bool { return len(mock.LogsCalls()) >= 1 }, + 5*time.Second, 10*time.Millisecond, "should have called Logs") cancel() l.Wait() } @@ -180,6 +176,38 @@ func TestLogStreamer_GoReturnsPointer(t *testing.T) { result.Wait() } +func TestLogStreamer_ErrAfterError(t *testing.T) { + ctx := context.Background() + mock := &mocks.LogClientMock{LogsFunc: func(opts docker.LogsOptions) error { + return errors.New("some docker error") + }} + + l := &LogStreamer{ContainerID: "test_id", ContainerName: "test_name", DockerClient: mock} + l = l.Go(ctx) + + require.Eventually(t, func() bool { return len(mock.LogsCalls()) >= 1 }, + 5*time.Second, 10*time.Millisecond, "should have called Logs") + l.Close() + assert.EqualError(t, l.Err(), "some docker error") +} + +func TestLogStreamer_ErrAfterNormalClose(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + mock := &mocks.LogClientMock{LogsFunc: func(opts docker.LogsOptions) error { + <-opts.Context.Done() + return opts.Context.Err() + }} + + l := &LogStreamer{ContainerID: "test_id", ContainerName: "test_name", DockerClient: mock} + l = l.Go(ctx) + + require.Eventually(t, func() bool { return len(mock.LogsCalls()) >= 1 }, + 5*time.Second, 10*time.Millisecond, "should have called Logs") + cancel() + l.Wait() + assert.NoError(t, l.Err(), "no error expected after normal context cancellation") +} + type nopWriteCloser struct { io.Writer } diff --git a/app/logger/multiwriter.go b/app/logger/multiwriter.go index 4ce85c0..aa14561 100644 --- a/app/logger/multiwriter.go +++ b/app/logger/multiwriter.go @@ -29,7 +29,7 @@ type jMsg struct { Host string `json:"host"` } -// NewMultiWriterIgnoreErrors create WriteCloser for multiple destinations +// NewMultiWriterIgnoreErrors creates WriteCloser for multiple destinations func NewMultiWriterIgnoreErrors(writers ...io.WriteCloser) *MultiWriter { w := make([]io.WriteCloser, len(writers)) copy(w, writers) @@ -61,8 +61,8 @@ func (w *MultiWriter) Write(p []byte) (n int, err error) { } numErrors := 0 - for _, w := range w.writers { - if _, err = w.Write(pp); err != nil { + for _, wr := range w.writers { + if _, err = wr.Write(pp); err != nil { numErrors++ } } @@ -78,8 +78,8 @@ func (w *MultiWriter) Write(p []byte) (n int, err error) { // Close all writers, collect errors func (w *MultiWriter) Close() error { errs := new(multierror.Error) - for _, w := range w.writers { - errs = multierror.Append(errs, w.Close()) + for _, wr := range w.writers { + errs = multierror.Append(errs, wr.Close()) } return errs.ErrorOrNil() } diff --git a/app/main.go b/app/main.go index 5bba5cd..e1ae65b 100644 --- a/app/main.go +++ b/app/main.go @@ -65,6 +65,7 @@ func main() { log.Printf("[INFO] options: %+v", opts) if err := do(ctx, &opts); err != nil { log.Printf("[ERROR] failed, %v", err) + os.Exit(1) } } @@ -81,6 +82,22 @@ func do(ctx context.Context, opts *cliOpts) error { return errors.New("only single option Excludes/ExcludesPattern are allowed") } + if opts.IncludesPattern != "" && opts.ExcludesPattern != "" { + return errors.New("only single option IncludesPattern/ExcludesPattern are allowed") + } + + if opts.Includes != nil && opts.ExcludesPattern != "" { + return errors.New("only single option Includes/ExcludesPattern are allowed") + } + + if opts.IncludesPattern != "" && opts.Excludes != nil { + return errors.New("only single option IncludesPattern/Excludes are allowed") + } + + if !opts.EnableFiles && !opts.EnableSyslog { + return errors.New("at least one log destination must be enabled (files or syslog)") + } + if opts.EnableSyslog && !syslog.IsSupported() { return errors.New("syslog is not supported on this OS") } @@ -90,16 +107,21 @@ func do(ctx context.Context, opts *cliOpts) error { return errors.Wrap(err, "failed to make docker client") } - events, err := discovery.NewEventNotif(client, opts.Excludes, opts.Includes, opts.IncludesPattern, opts.ExcludesPattern) + events, err := discovery.NewEventNotif(client, discovery.EventNotifOpts{ + Excludes: opts.Excludes, + Includes: opts.Includes, + IncludesPattern: opts.IncludesPattern, + ExcludesPattern: opts.ExcludesPattern, + }) if err != nil { return errors.Wrap(err, "failed to make event notifier") } - runEventLoop(ctx, opts, events.Channel(), client) - return nil + return runEventLoop(ctx, opts, events.Channel(), events.Err(), client) } -func runEventLoop(ctx context.Context, opts *cliOpts, eventsCh <-chan discovery.Event, logClient logger.LogClient) { +func runEventLoop(ctx context.Context, opts *cliOpts, eventsCh <-chan discovery.Event, + listenerErr <-chan error, logClient logger.LogClient) error { logStreams := map[string]*logger.LogStreamer{} procEvent := func(event discovery.Event) { @@ -111,7 +133,11 @@ func runEventLoop(ctx context.Context, opts *cliOpts, eventsCh <-chan discovery. return } - logWriter, errWriter := makeLogWriters(opts, event.ContainerName, event.Group) + logWriter, errWriter, err := makeLogWriters(opts, event.ContainerName, event.Group) + if err != nil { + log.Printf("[WARN] failed to create log writers for %s, %v", event.ContainerName, err) + return + } ls := &logger.LogStreamer{ DockerClient: logClient, ContainerID: event.ContainerID, @@ -133,50 +159,63 @@ func runEventLoop(ctx context.Context, opts *cliOpts, eventsCh <-chan discovery. } log.Printf("[DEBUG] close loggers for %+v", event) - ls.Close() - - if e := ls.LogWriter.Close(); e != nil { - log.Printf("[WARN] failed to close log writer for %+v, %s", event, e) - } - - if !opts.MixErr { // don't close err writer in mixed mode, closed already by LogWriter.Close() - if e := ls.ErrWriter.Close(); e != nil { - log.Printf("[WARN] failed to close err writer for %+v, %s", event, e) - } - } + closeStreamer(ls, event.ContainerName, opts.MixErr) delete(logStreams, event.ContainerID) log.Printf("[DEBUG] streaming for %d containers", len(logStreams)) } + closeAll := func() { + for _, v := range logStreams { + closeStreamer(v, v.ContainerName, opts.MixErr) + log.Printf("[INFO] close logger stream for %s", v.ContainerName) + } + } + for { select { case <-ctx.Done(): log.Print("[WARN] event loop terminated") - for _, v := range logStreams { - v.Close() - if e := v.LogWriter.Close(); e != nil { - log.Printf("[WARN] failed to close log writer for %s, %s", v.ContainerName, e) + closeAll() + return nil + case err := <-listenerErr: + log.Printf("[ERROR] event listener failed, %v", err) + closeAll() + return err + case event, ok := <-eventsCh: + if !ok { + log.Print("[WARN] events channel closed, terminating event loop") + closeAll() + // check if the listener sent a more specific error before closing the channel + select { + case err := <-listenerErr: + return err + default: + return errors.New("events channel closed unexpectedly") } - if !opts.MixErr { - if e := v.ErrWriter.Close(); e != nil { - log.Printf("[WARN] failed to close err writer for %s, %s", v.ContainerName, e) - } - } - log.Printf("[INFO] close logger stream for %s", v.ContainerName) } - return - case event := <-eventsCh: log.Printf("[DEBUG] received event %+v", event) procEvent(event) } } } +func closeStreamer(ls *logger.LogStreamer, name string, mixErr bool) { + ls.Close() + if e := ls.LogWriter.Close(); e != nil { + log.Printf("[WARN] failed to close log writer for %s, %s", name, e) + } + if !mixErr { + if e := ls.ErrWriter.Close(); e != nil { + log.Printf("[WARN] failed to close err writer for %s, %s", name, e) + } + } +} + // makeLogWriters creates io.WriteCloser with rotated out and separate err files. Also adds writer for remote syslog -func makeLogWriters(opts *cliOpts, containerName, group string) (logWriter, errWriter io.WriteCloser) { +func makeLogWriters(opts *cliOpts, containerName, group string) (logWriter, errWriter io.WriteCloser, err error) { log.Printf("[DEBUG] create log writer for %s", strings.TrimPrefix(group+"/"+containerName, "/")) if !opts.EnableFiles && !opts.EnableSyslog { - log.Fatalf("[ERROR] either files or syslog has to be enabled") + return nil, nil, errors.New("either files or syslog has to be enabled") } var logWriters []io.WriteCloser // collect log writers here, for MultiWriter use @@ -188,7 +227,7 @@ func makeLogWriters(opts *cliOpts, containerName, group string) (logWriter, errW logDir = fmt.Sprintf("%s/%s", opts.FilesLocation, group) } if err := os.MkdirAll(logDir, 0o750); err != nil { - log.Fatalf("[ERROR] can't make directory %s, %v", logDir, err) + return nil, nil, errors.Wrapf(err, "can't make directory %s", logDir) } logName := fmt.Sprintf("%s/%s.log", logDir, containerName) @@ -226,12 +265,16 @@ func makeLogWriters(opts *cliOpts, containerName, group string) (logWriter, errW if err == nil { logWriters = append(logWriters, syslogWriter) - errWriters = append(errWriters, syslogWriter) + errWriters = append(errWriters, writeNopCloser{syslogWriter}) // wrap to prevent double-close } else { - log.Printf("[WARN] can't connect to syslog, %v", err) + log.Printf("[ERROR] can't connect to syslog, %v", err) } } + if len(logWriters) == 0 { + return nil, nil, errors.New("no log destinations available") + } + lw := logger.NewMultiWriterIgnoreErrors(logWriters...) ew := logger.NewMultiWriterIgnoreErrors(errWriters...) if opts.ExtJSON { @@ -239,9 +282,17 @@ func makeLogWriters(opts *cliOpts, containerName, group string) (logWriter, errW ew = ew.WithExtJSON(containerName, group) } - return lw, ew + return lw, ew, nil +} + +// writeNopCloser wraps an io.Writer with a no-op Close method. +// used to prevent double-close when the same writer (e.g., syslog) is shared between log and err MultiWriters. +type writeNopCloser struct { + io.Writer } +func (writeNopCloser) Close() error { return nil } + func setupLog(dbg bool) { if dbg { log.Setup(log.Debug, log.CallerFile, log.CallerFunc, log.Msec, log.LevelBraces) diff --git a/app/main_test.go b/app/main_test.go index ecea44a..4db6400 100644 --- a/app/main_test.go +++ b/app/main_test.go @@ -2,6 +2,8 @@ package main import ( "context" + "errors" + "net" "os" "path/filepath" "sync/atomic" @@ -14,6 +16,7 @@ import ( "github.com/umputun/docker-logger/app/discovery" logmocks "github.com/umputun/docker-logger/app/logger/mocks" + "github.com/umputun/docker-logger/app/syslog" ) func Test_Do(t *testing.T) { @@ -33,8 +36,6 @@ func Test_Do(t *testing.T) { defer cancel() err := do(ctx, &opts) require.NoError(t, err) - - time.Sleep(200 * time.Millisecond) // let it start } func Test_doValidation(t *testing.T) { @@ -43,6 +44,9 @@ func Test_doValidation(t *testing.T) { opts cliOpts err string }{ + {name: "no destinations enabled", + opts: cliOpts{}, + err: "at least one log destination must be enabled"}, {name: "includes and includesPattern conflict", opts: cliOpts{Includes: []string{"foo"}, IncludesPattern: "bar.*", EnableFiles: true}, err: "only single option Includes/IncludesPattern are allowed"}, @@ -55,6 +59,15 @@ func Test_doValidation(t *testing.T) { {name: "excludes and excludesPattern conflict", opts: cliOpts{Excludes: []string{"foo"}, ExcludesPattern: "bar.*", EnableFiles: true}, err: "only single option Excludes/ExcludesPattern are allowed"}, + {name: "includesPattern and excludesPattern conflict", + opts: cliOpts{IncludesPattern: "foo.*", ExcludesPattern: "bar.*", EnableFiles: true}, + err: "only single option IncludesPattern/ExcludesPattern are allowed"}, + {name: "includes and excludesPattern conflict", + opts: cliOpts{Includes: []string{"foo"}, ExcludesPattern: "bar.*", EnableFiles: true}, + err: "only single option Includes/ExcludesPattern are allowed"}, + {name: "includesPattern and excludes conflict", + opts: cliOpts{IncludesPattern: "foo.*", Excludes: []string{"bar"}, EnableFiles: true}, + err: "only single option IncludesPattern/Excludes are allowed"}, {name: "invalid excludesPattern", opts: cliOpts{DockerHost: "unix:///var/run/docker.sock", ExcludesPattern: "[invalid", EnableFiles: true}, err: "failed to compile excludesPattern"}, @@ -81,6 +94,7 @@ func Test_runEventLoop(t *testing.T) { tmpDir := t.TempDir() opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -95,7 +109,7 @@ func Test_runEventLoop(t *testing.T) { done := make(chan struct{}) go func() { - runEventLoop(ctx, &opts, eventsCh, mockClient) + _ = runEventLoop(ctx, &opts, eventsCh, listenerErr, mockClient) close(done) }() @@ -108,7 +122,13 @@ func Test_runEventLoop(t *testing.T) { // send stop event eventsCh <- discovery.Event{ContainerID: "c1", ContainerName: "test1", Group: "gr1", Status: false} - time.Sleep(100 * time.Millisecond) + + // send a sentinel start event for a different container; when it's processed we know stop was handled + eventsCh <- discovery.Event{ContainerID: "c2", ContainerName: "test2", Group: "gr1", Status: true} + require.Eventually(t, func() bool { + _, err := os.Stat(filepath.Join(tmpDir, "gr1", "test2.log")) + return err == nil + }, time.Second, 10*time.Millisecond, "sentinel container should be processed, confirming stop was handled") cancel() <-done @@ -118,6 +138,7 @@ func Test_runEventLoop(t *testing.T) { tmpDir := t.TempDir() opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -131,18 +152,23 @@ func Test_runEventLoop(t *testing.T) { done := make(chan struct{}) go func() { - runEventLoop(ctx, &opts, eventsCh, mockClient) + _ = runEventLoop(ctx, &opts, eventsCh, listenerErr, mockClient) close(done) }() // send same container start event twice eventsCh <- discovery.Event{ContainerID: "c1", ContainerName: "test1", Status: true} - time.Sleep(50 * time.Millisecond) + require.Eventually(t, func() bool { return logsCalls.Load() == 1 }, + time.Second, 10*time.Millisecond, "first event should be processed") eventsCh <- discovery.Event{ContainerID: "c1", ContainerName: "test1", Status: true} - time.Sleep(50 * time.Millisecond) - // only one log stream should have been created - assert.Equal(t, int32(1), logsCalls.Load(), "duplicate start should be ignored") + // send a sentinel start for a different container to confirm the duplicate was processed + eventsCh <- discovery.Event{ContainerID: "c-sentinel", ContainerName: "sentinel", Status: true} + require.Eventually(t, func() bool { return logsCalls.Load() == 2 }, + time.Second, 10*time.Millisecond, "sentinel should be processed, confirming duplicate was handled") + + // only two log streams should have been created (c1 + sentinel, not c1 twice) + assert.Equal(t, int32(2), logsCalls.Load(), "duplicate start should be ignored") cancel() <-done @@ -152,6 +178,7 @@ func Test_runEventLoop(t *testing.T) { tmpDir := t.TempDir() opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -163,15 +190,20 @@ func Test_runEventLoop(t *testing.T) { done := make(chan struct{}) go func() { - runEventLoop(ctx, &opts, eventsCh, mockClient) + _ = runEventLoop(ctx, &opts, eventsCh, listenerErr, mockClient) close(done) }() // send stop event for non-existing container, should not panic or error eventsCh <- discovery.Event{ContainerID: "unknown", ContainerName: "unknown", Status: false} - time.Sleep(50 * time.Millisecond) - assert.Empty(t, mockClient.LogsCalls(), "no log streams should be created for stop-only events") + // send a sentinel start event to confirm the stop event was processed + eventsCh <- discovery.Event{ContainerID: "c-sentinel", ContainerName: "sentinel", Status: true} + require.Eventually(t, func() bool { return len(mockClient.LogsCalls()) == 1 }, + time.Second, 10*time.Millisecond, "sentinel should be processed, confirming stop was handled") + + // only one log stream should exist (sentinel), confirming stop didn't create any + assert.Len(t, mockClient.LogsCalls(), 1, "only sentinel log stream should be created") cancel() <-done @@ -181,6 +213,7 @@ func Test_runEventLoop(t *testing.T) { tmpDir := t.TempDir() opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) ctx, cancel := context.WithCancel(context.Background()) @@ -193,7 +226,7 @@ func Test_runEventLoop(t *testing.T) { done := make(chan struct{}) go func() { - runEventLoop(ctx, &opts, eventsCh, mockClient) + _ = runEventLoop(ctx, &opts, eventsCh, listenerErr, mockClient) close(done) }() @@ -208,6 +241,154 @@ func Test_runEventLoop(t *testing.T) { cancel() <-done // runEventLoop should close all streams and return }) + + t.Run("mixErr mode closes writer once", func(t *testing.T) { + // verify that with MixErr=true, the err writer is not separately closed on container stop. + // both stdout and stderr data should end up in the same .log file and no .err file should exist. + tmpDir := t.TempDir() + opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10, MixErr: true} + eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + mockClient := &logmocks.LogClientMock{LogsFunc: func(opts docker.LogsOptions) error { + if opts.OutputStream != nil { + _, _ = opts.OutputStream.Write([]byte("stdout line\n")) + } + if opts.ErrorStream != nil { + _, _ = opts.ErrorStream.Write([]byte("stderr line\n")) + } + <-opts.Context.Done() + return opts.Context.Err() + }} + + done := make(chan struct{}) + go func() { + _ = runEventLoop(ctx, &opts, eventsCh, listenerErr, mockClient) + close(done) + }() + + // start container + eventsCh <- discovery.Event{ContainerID: "c1", ContainerName: "test1", Group: "gr1", Status: true} + require.Eventually(t, func() bool { + _, err := os.Stat(filepath.Join(tmpDir, "gr1", "test1.log")) + return err == nil + }, time.Second, 10*time.Millisecond, "log file should be created") + + // stop the container — this should close LogWriter but skip closing ErrWriter + eventsCh <- discovery.Event{ContainerID: "c1", ContainerName: "test1", Group: "gr1", Status: false} + + // send sentinel to confirm stop was processed + eventsCh <- discovery.Event{ContainerID: "c2", ContainerName: "sentinel", Group: "gr1", Status: true} + require.Eventually(t, func() bool { + _, err := os.Stat(filepath.Join(tmpDir, "gr1", "sentinel.log")) + return err == nil + }, time.Second, 10*time.Millisecond, "sentinel container should be processed") + + // verify both stdout and stderr ended up in the same .log file + logFile := filepath.Join(tmpDir, "gr1", "test1.log") + data, err := os.ReadFile(logFile) //nolint:gosec // test file path + require.NoError(t, err) + assert.Contains(t, string(data), "stdout line") + assert.Contains(t, string(data), "stderr line") + + // verify no .err file was created + _, err = os.Stat(filepath.Join(tmpDir, "gr1", "test1.err")) + assert.True(t, os.IsNotExist(err), ".err file should not exist in MixErr mode") + + cancel() + <-done + }) + + t.Run("closed events channel exits loop with error", func(t *testing.T) { + tmpDir := t.TempDir() + opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} + eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) + + mockClient := &logmocks.LogClientMock{LogsFunc: func(opts docker.LogsOptions) error { + <-opts.Context.Done() + return opts.Context.Err() + }} + + errCh := make(chan error, 1) + go func() { + errCh <- runEventLoop(context.Background(), &opts, eventsCh, listenerErr, mockClient) + }() + + // close events channel to simulate EventNotif failure + close(eventsCh) + + select { + case err := <-errCh: + require.Error(t, err, "runEventLoop should return error when events channel closed") + assert.Contains(t, err.Error(), "events channel closed unexpectedly") + case <-time.After(5 * time.Second): + t.Fatal("runEventLoop should exit after events channel closed") + } + }) + + t.Run("listener error propagated", func(t *testing.T) { + tmpDir := t.TempDir() + opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} + eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) + + mockClient := &logmocks.LogClientMock{LogsFunc: func(opts docker.LogsOptions) error { + <-opts.Context.Done() + return opts.Context.Err() + }} + + errCh := make(chan error, 1) + go func() { + errCh <- runEventLoop(context.Background(), &opts, eventsCh, listenerErr, mockClient) + }() + + // simulate listener failure + listenerErr <- errors.New("can't add event listener: connection refused") + + select { + case err := <-errCh: + require.Error(t, err) + assert.Contains(t, err.Error(), "connection refused") + case <-time.After(5 * time.Second): + t.Fatal("runEventLoop should exit after listener error") + } + }) + + t.Run("listener error preferred over generic channel close", func(t *testing.T) { + // simulates the real activate() pattern: error sent to listenerErr AND eventsCh closed. + // regardless of which select case fires first, the listener error should be returned. + tmpDir := t.TempDir() + opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} + eventsCh := make(chan discovery.Event, 10) + listenerErr := make(chan error, 1) + + mockClient := &logmocks.LogClientMock{LogsFunc: func(opts docker.LogsOptions) error { + <-opts.Context.Done() + return opts.Context.Err() + }} + + // send error and close channel before starting the loop, so both are ready + listenerErr <- errors.New("can't add event listener: connection refused") + close(eventsCh) + + errCh := make(chan error, 1) + go func() { + errCh <- runEventLoop(context.Background(), &opts, eventsCh, listenerErr, mockClient) + }() + + select { + case err := <-errCh: + require.Error(t, err) + assert.Contains(t, err.Error(), "connection refused", + "should return the listener error, not generic 'events channel closed'") + case <-time.After(5 * time.Second): + t.Fatal("runEventLoop should exit after listener error with closed channel") + } + }) } func Test_makeLogWriters(t *testing.T) { @@ -215,10 +396,11 @@ func Test_makeLogWriters(t *testing.T) { setupLog(true) opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} - stdWr, errWr := makeLogWriters(&opts, "container1", "gr1") + stdWr, errWr, err := makeLogWriters(&opts, "container1", "gr1") + require.NoError(t, err) assert.NotEqual(t, stdWr, errWr, "different writers for out and err") - _, err := stdWr.Write([]byte("abc line 1\n")) + _, err = stdWr.Write([]byte("abc line 1\n")) require.NoError(t, err) _, err = stdWr.Write([]byte("xxx123 line 2\n")) require.NoError(t, err) @@ -247,10 +429,13 @@ func Test_makeLogWritersMixed(t *testing.T) { setupLog(false) opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10, MixErr: true} - stdWr, errWr := makeLogWriters(&opts, "container1", "gr1") - assert.Equal(t, stdWr, errWr, "same writer for out and err in mixed mode") + stdWr, errWr, err := makeLogWriters(&opts, "container1", "gr1") + require.NoError(t, err) + assert.NotNil(t, stdWr, "log writer should not be nil") + assert.NotNil(t, errWr, "err writer should not be nil") - _, err := stdWr.Write([]byte("abc line 1\n")) + // write to both log and err writers + _, err = stdWr.Write([]byte("abc line 1\n")) require.NoError(t, err) _, err = stdWr.Write([]byte("xxx123 line 2\n")) require.NoError(t, err) @@ -260,11 +445,16 @@ func Test_makeLogWritersMixed(t *testing.T) { _, err = errWr.Write([]byte("xxx123 line 2\n")) require.NoError(t, err) + // verify all data ends up in the single .log file logFile := filepath.Join(tmpDir, "gr1", "container1.log") r, err := os.ReadFile(logFile) //nolint:gosec // test file path require.NoError(t, err) assert.Equal(t, "abc line 1\nxxx123 line 2\nerr line 1\nxxx123 line 2\n", string(r)) + // verify no .err file was created in mixed mode + _, err = os.Stat(filepath.Join(tmpDir, "gr1", "container1.err")) + assert.True(t, os.IsNotExist(err), ".err file should not exist in mixed mode") + assert.NoError(t, stdWr.Close()) assert.NoError(t, errWr.Close()) } @@ -272,9 +462,10 @@ func Test_makeLogWritersMixed(t *testing.T) { func Test_makeLogWritersWithJSON(t *testing.T) { tmpDir := t.TempDir() opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10, ExtJSON: true} - stdWr, errWr := makeLogWriters(&opts, "container1", "gr1") + stdWr, errWr, err := makeLogWriters(&opts, "container1", "gr1") + require.NoError(t, err) - _, err := stdWr.Write([]byte("abc line 1")) + _, err = stdWr.Write([]byte("abc line 1")) require.NoError(t, err) logFile := filepath.Join(tmpDir, "gr1", "container1.log") @@ -292,9 +483,10 @@ func Test_makeLogWritersWithJSON(t *testing.T) { func Test_makeLogWritersNoGroup(t *testing.T) { tmpDir := t.TempDir() opts := cliOpts{FilesLocation: tmpDir, EnableFiles: true, MaxFileSize: 1, MaxFilesCount: 10} - stdWr, errWr := makeLogWriters(&opts, "container1", "") + stdWr, errWr, err := makeLogWriters(&opts, "container1", "") + require.NoError(t, err) - _, err := stdWr.Write([]byte("test line\n")) + _, err = stdWr.Write([]byte("test line\n")) require.NoError(t, err) logFile := filepath.Join(tmpDir, "container1.log") @@ -306,34 +498,152 @@ func Test_makeLogWritersNoGroup(t *testing.T) { assert.NoError(t, errWr.Close()) } -func Test_makeLogWritersSyslogFailed(t *testing.T) { - opts := cliOpts{EnableSyslog: true} - stdWr, errWr := makeLogWriters(&opts, "container1", "gr1") - assert.Equal(t, stdWr, errWr, "same writer for out and err in syslog") +func Test_makeLogWritersNeitherEnabled(t *testing.T) { + opts := cliOpts{} + _, _, err := makeLogWriters(&opts, "container1", "gr1") + require.Error(t, err) + assert.Contains(t, err.Error(), "either files or syslog has to be enabled") +} + +func Test_makeLogWritersInvalidDir(t *testing.T) { + // create a regular file, then use a path under it as FilesLocation to guarantee mkdir failure + invalidParent := filepath.Join(t.TempDir(), "not-a-dir") + require.NoError(t, os.WriteFile(invalidParent, []byte("x"), 0o600)) + + opts := cliOpts{EnableFiles: true, FilesLocation: filepath.Join(invalidParent, "subdir")} + _, _, err := makeLogWriters(&opts, "container1", "gr1") + require.Error(t, err) + assert.Contains(t, err.Error(), "can't make directory") +} - _, err := stdWr.Write([]byte("abc line 1\n")) +func Test_makeLogWritersFilesAndSyslogNoDoubleClose(t *testing.T) { + if !syslog.IsSupported() { + t.Skip("syslog not supported on this platform") + } + conn, err := net.ListenPacket("udp4", "127.0.0.1:0") require.NoError(t, err) - _, err = stdWr.Write([]byte("xxx123 line 2\n")) + defer conn.Close() + + tmpDir := t.TempDir() + opts := cliOpts{ + FilesLocation: tmpDir, EnableFiles: true, EnableSyslog: true, + SyslogHost: conn.LocalAddr().String(), SyslogPrefix: "docker/", + MaxFileSize: 1, MaxFilesCount: 10, + } + stdWr, errWr, err := makeLogWriters(&opts, "container1", "gr1") require.NoError(t, err) - _, err = errWr.Write([]byte("err line 1\n")) + // write to both writers + _, err = stdWr.Write([]byte("log line\n")) require.NoError(t, err) - _, err = errWr.Write([]byte("xxx123 line 2\n")) + _, err = errWr.Write([]byte("err line\n")) require.NoError(t, err) + + // close both writers without double-close errors + assert.NoError(t, stdWr.Close(), "log writer close should not error") + assert.NoError(t, errWr.Close(), "err writer close should not error") } -func Test_makeLogWritersSyslogPassed(t *testing.T) { - opts := cliOpts{EnableSyslog: true, SyslogHost: "127.0.0.1:514", SyslogPrefix: "docker/"} - stdWr, errWr := makeLogWriters(&opts, "container1", "gr1") - assert.Equal(t, stdWr, errWr, "same writer for out and err in syslog") +func Test_writeNopCloser(t *testing.T) { + var closeCalls int + mock := &mockWriteCloser{ + writeFunc: func(p []byte) (int, error) { return len(p), nil }, + closeFunc: func() error { closeCalls++; return nil }, + } - _, err := stdWr.Write([]byte("abc line 1\n")) + // writeNopCloser should write through but not close + nop := writeNopCloser{mock} + _, err := nop.Write([]byte("test")) require.NoError(t, err) - _, err = stdWr.Write([]byte("xxx123 line 2\n")) + assert.NoError(t, nop.Close()) + assert.Equal(t, 0, closeCalls, "underlying writer should not be closed via nop closer") + + // direct close should work + require.NoError(t, mock.Close()) + assert.Equal(t, 1, closeCalls, "underlying writer should be closed once via direct call") +} + +type mockWriteCloser struct { + writeFunc func(p []byte) (int, error) + closeFunc func() error +} + +func (m *mockWriteCloser) Write(p []byte) (int, error) { return m.writeFunc(p) } +func (m *mockWriteCloser) Close() error { return m.closeFunc() } + +func Test_makeLogWritersSyslogFailedNoFiles(t *testing.T) { + if !syslog.IsSupported() { + t.Skip("syslog not supported on this platform") + } + // syslog-only mode with invalid host should return error, not create empty writers + opts := cliOpts{EnableSyslog: true, SyslogHost: "invalid:::host"} + _, _, err := makeLogWriters(&opts, "container1", "gr1") + require.Error(t, err) + assert.Contains(t, err.Error(), "no log destinations available") +} + +func Test_makeLogWritersSyslogOnly(t *testing.T) { + if !syslog.IsSupported() { + t.Skip("syslog not supported on this platform") + } + // syslog-only mode (files disabled), syslog connects to a real UDP listener + conn, err := net.ListenPacket("udp4", "127.0.0.1:0") require.NoError(t, err) + defer conn.Close() - _, err = errWr.Write([]byte("err line 1\n")) + opts := cliOpts{EnableSyslog: true, SyslogHost: conn.LocalAddr().String(), SyslogPrefix: "docker/"} + stdWr, errWr, err := makeLogWriters(&opts, "container1", "gr1") require.NoError(t, err) - _, err = errWr.Write([]byte("xxx123 line 2\n")) + assert.NotEqual(t, stdWr, errWr, "err writer wraps syslog with nop closer") + + _, err = stdWr.Write([]byte("syslog test message\n")) + require.NoError(t, err) + + // verify data arrives at the UDP listener + require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second))) + buf := make([]byte, 1024) + n, _, err := conn.ReadFrom(buf) + require.NoError(t, err) + assert.Contains(t, string(buf[:n]), "syslog test message") + assert.Contains(t, string(buf[:n]), "docker/container1") + + assert.NoError(t, stdWr.Close()) + assert.NoError(t, errWr.Close()) +} + +func Test_makeLogWritersSyslogWithFiles(t *testing.T) { + if !syslog.IsSupported() { + t.Skip("syslog not supported on this platform") + } + // both files and syslog enabled, verify syslog data arrives and files are written + conn, err := net.ListenPacket("udp4", "127.0.0.1:0") + require.NoError(t, err) + defer conn.Close() + + tmpDir := t.TempDir() + opts := cliOpts{ + EnableFiles: true, FilesLocation: tmpDir, MaxFileSize: 1, MaxFilesCount: 10, + EnableSyslog: true, SyslogHost: conn.LocalAddr().String(), SyslogPrefix: "docker/", + } + stdWr, errWr, err := makeLogWriters(&opts, "container1", "gr1") + require.NoError(t, err) + + _, err = stdWr.Write([]byte("log message\n")) require.NoError(t, err) + + // verify syslog received the data + require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second))) + buf := make([]byte, 1024) + n, _, err := conn.ReadFrom(buf) + require.NoError(t, err) + assert.Contains(t, string(buf[:n]), "log message") + + // verify file was also written + logFile := filepath.Join(tmpDir, "gr1", "container1.log") + r, err := os.ReadFile(logFile) //nolint:gosec // test file path + require.NoError(t, err) + assert.Equal(t, "log message\n", string(r)) + + assert.NoError(t, stdWr.Close()) + assert.NoError(t, errWr.Close()) } diff --git a/app/syslog/syslog_unix.go b/app/syslog/syslog_unix.go index bfe0dcb..431bbd0 100644 --- a/app/syslog/syslog_unix.go +++ b/app/syslog/syslog_unix.go @@ -7,7 +7,8 @@ import ( "log/syslog" ) -// GetWriter returns syslog writer for given host and container +// GetWriter returns syslog writer for given host, prefix and container name. +// The syslogPrefix is prepended to containerName to form the syslog tag. func GetWriter(syslogHost, syslogPrefix, containerName string) (io.WriteCloser, error) { return syslog.Dial("udp4", syslogHost, syslog.LOG_WARNING|syslog.LOG_DAEMON, syslogPrefix+containerName) } diff --git a/docs/plans/completed/20260216-code-review-fixes.md b/docs/plans/completed/20260216-code-review-fixes.md new file mode 100644 index 0000000..3cc928e --- /dev/null +++ b/docs/plans/completed/20260216-code-review-fixes.md @@ -0,0 +1,227 @@ +# Code Review Fixes + +## Overview + +Address all issues discovered during full-project code review. Fixes span bugs, error handling, code quality, test reliability, and documentation gaps. + +Key changes: +- fix incorrect timestamp calculations in event discovery +- replace `log.Fatalf` with proper error returns in non-main functions +- fix syslog writer double-close when both files and syslog are enabled +- eliminate flaky `time.Sleep` patterns in tests +- update README with missing CLI flags and documentation gaps + +## Context (from discovery) + +Files/components involved: +- `app/discovery/events.go` — timestamp bugs, typo, Names guard, parameter count +- `app/main.go` — log.Fatalf paths, syslog double-close, incomplete validation, syslog error handling +- `app/logger/multiwriter.go` — variable shadowing in Write/Close loops +- `app/logger/logger.go` — godoc improvements +- `app/main_test.go` — flaky sleep patterns, syslog test quality, MixErr coverage gap +- `app/logger/logger_test.go` — flaky sleep patterns +- `app/syslog/syslog_unix.go` — godoc fix +- `README.md` — missing flags, typo, missing exclusivity note + +## Development Approach + +- **ALL issues are in scope** — this plan exists specifically to fix pre-existing problems found during code review. Nothing should be dismissed as "pre-existing", "not introduced by this branch", or "outside scope" +- **testing approach**: Regular (fix code, then add/update tests) +- complete each task fully before moving to the next +- make small, focused changes +- **CRITICAL: every task MUST include new/updated tests** for code changes in that task +- **CRITICAL: all tests must pass before starting next task** +- **CRITICAL: update this plan file when scope changes during implementation** +- run tests after each change +- maintain backward compatibility + +## Testing Strategy + +- **unit tests**: required for every task with code changes +- no e2e tests in this project + +## Progress Tracking + +- mark completed items with `[x]` immediately when done +- add newly discovered tasks with ➕ prefix +- document issues/blockers with ⚠️ prefix + +## Implementation Steps + +### Task 1: Fix timestamp calculations in events.go + +**Files:** +- Modify: `app/discovery/events.go` +- Modify: `app/discovery/events_test.go` + +- [x] fix `activate()` line 123: use `TimeNano` with fallback — `if dockerEvent.TimeNano != 0 { time.Unix(0, dockerEvent.TimeNano) } else { time.Unix(dockerEvent.Time, 0) }` to handle older Docker API versions that may not set `TimeNano` +- [x] fix `emitRunningContainers()` line 150: change `time.Unix(c.Created/1000, 0)` to `time.Unix(c.Created, 0)` — `Created` is already Unix seconds +- [x] fix typo line 96: change `"can't add even listener"` to `"can't add event listener"` +- [x] add guard in `emitRunningContainers()` for empty `c.Names` slice before accessing `c.Names[0]` +- [x] add test case in `TestEmit` verifying `Event.TS` is a reasonable timestamp (not ~1970) +- [x] add test case in `TestActivateAllContainerStatuses` or similar verifying `Event.TS` from activate path +- [x] run `go test ./app/discovery/ -count=1` — must pass before next task + +### Task 2: Replace log.Fatalf with error returns in makeLogWriters + +**Files:** +- Modify: `app/main.go` +- Modify: `app/main_test.go` + +- [x] change `makeLogWriters` signature to return `(logWriter, errWriter io.WriteCloser, err error)` +- [x] replace `log.Fatalf` on line 179 (neither destination enabled) with `return nil, nil, errors.New("either files or syslog has to be enabled")` +- [x] replace `log.Fatalf` on line 191 (directory creation failure) with error return +- [x] update `procEvent` in `runEventLoop` (the only caller of `makeLogWriters`) to handle the error — log warning and skip the container +- [x] add test case for `makeLogWriters` with neither files nor syslog enabled — verify error returned +- [x] add test case for `makeLogWriters` with invalid directory path — verify error returned +- [x] run `go test ./app/ -count=1` — must pass before next task + +### Task 3: Fix syslog writer double-close + +**Files:** +- Modify: `app/main.go` +- Modify: `app/main_test.go` + +- [x] in `makeLogWriters`, when syslog is enabled: track whether the same syslog writer is shared between logWriters and errWriters. When both files and syslog are enabled without MixErr, the syslog writer appears in both MultiWriters and gets closed twice +- [x] approach: create a `writeNopCloser` that wraps an `io.Writer` but makes `Close()` a no-op, use it for the errWriters syslog entry so only logWriters actually closes syslog. Note: the `MixErr` guard in `runEventLoop` remains necessary for file writers +- [x] add test case with both `EnableFiles: true` and `EnableSyslog: true` verifying no double-close errors +- [x] run `go test ./app/ -count=1 -race` — must pass before next task + +### Task 4a: Fix variable shadowing in multiwriter.go + +**Files:** +- Modify: `app/logger/multiwriter.go` + +- [x] rename loop variable in `MultiWriter.Write` from `w` to `wr`: `for _, wr := range w.writers` +- [x] rename loop variable in `MultiWriter.Close` from `w` to `wr`: `for _, wr := range w.writers` +- [x] run `go test ./app/logger/ -count=1` — must pass before next task + +### Task 4b: Add missing filter validation in do() + +**Files:** +- Modify: `app/main.go` +- Modify: `app/main_test.go` + +- [x] add validation in `do()` for `IncludesPattern + ExcludesPattern` combination — return error +- [x] add validation in `do()` for `Includes + ExcludesPattern` combination — return error +- [x] add validation in `do()` for `IncludesPattern + Excludes` combination — return error +- [x] add test cases in `Test_doValidation` for all three new conflict combinations +- [x] run `go test ./app/ -count=1` — must pass before next task + +### Task 5: Fix flaky test patterns in main_test.go + +**Files:** +- Modify: `app/main_test.go` + +- [x] `Test_runEventLoop:"start and stop container"` line 111: replace `time.Sleep(100ms)` after stop event with `require.Eventually` checking a verifiable condition (e.g., send a follow-up sentinel start event and wait for it to be processed, confirming the stop was handled first) +- [x] `Test_runEventLoop:"duplicate start ignored"` lines 140-145: replace `time.Sleep(50ms)` with `require.Eventually` to confirm first event processed (logsCalls == 1) before sending second event +- [x] `Test_runEventLoop:"stop non-mapped container ignored"` lines 171-174: replace `time.Sleep(50ms)` with a sentinel event technique — send a known start event after the stop, wait for it to be processed via `require.Eventually`, then assert LogsCalls == 1 (confirming stop was also processed and ignored) +- [x] remove `time.Sleep(200ms)` from `Test_Do` line 37 — serves no purpose after `do()` returns +- [x] run `go test ./app/ -count=1 -race` — must pass before next task + +### Task 6: Fix flaky test patterns in logger_test.go + +**Files:** +- Modify: `app/logger/logger_test.go` + +- [x] `TestLogStreamer_NormalCompletion` line 72: replace `time.Sleep(50ms)` with `require.Eventually` checking `mock.LogsCalls()` length == 1 before calling `Close()` +- [x] `TestLogStreamer_ErrorTermination` line 114: replace `time.Sleep(50ms)` with `require.Eventually` checking `mock.LogsCalls()` length == 1 before calling `Close()` +- [x] run `go test ./app/logger/ -count=1 -race` — must pass before next task + +### Task 7: Improve syslog test quality and add MixErr coverage + +**Files:** +- Modify: `app/main_test.go` + +- [x] fix `Test_makeLogWritersSyslogFailed`: make it actually verify syslog failure — e.g., assert that the returned writer has zero underlying file writers (only syslog was attempted and failed) +- [x] fix or remove `Test_makeLogWritersSyslogPassed`: either start a UDP listener (like syslog_unix_test.go does) and verify data, or consolidate with SyslogFailed since they exercise the same path +- [x] add `Test_runEventLoop_MixErr` test: verify that with `MixErr: true`, the err writer is not separately closed on container stop +- [x] run `go test ./app/ -count=1` — must pass before next task + +### Task 8: Update documentation + +**Files:** +- Modify: `README.md` +- Modify: `app/syslog/syslog_unix.go` +- Modify: `app/logger/logger.go` + +- [x] add missing rows to README options table: `--loc`/`LOG_FILES_LOC` (default "logs"), `--syslog-prefix`/`SYSLOG_PREFIX` (default "docker/"), `--dbg`/`DEBUG` (default false) +- [x] add bullet for `--exclude`/`--exclude-pattern` mutual exclusivity note +- [x] fix typo on line 8: "inlcudes" → "includes" +- [x] fix godoc for `GetWriter` in syslog_unix.go: mention syslogPrefix parameter +- [x] improve godoc for `LogStreamer.Go()`: mention goroutine lifecycle and retry behavior +- [x] improve godoc for `LogStreamer.Close()`: mention it blocks until stream completes +- [x] improve godoc for `LogStreamer.Wait()`: clarify it waits on context cancellation + +### Task 9: Verify acceptance criteria + +- [x] verify all timestamp calculations produce correct dates +- [x] verify `makeLogWriters` returns errors instead of calling Fatalf +- [x] verify no double-close warnings with files+syslog enabled +- [x] verify no `time.Sleep` patterns remain in tests (except `TestLogStreamer_RetryOnDockerEOF` which is unavoidable due to 1s sleep in production code) +- [x] run full test suite: `go test ./... -count=1` +- [x] run race detector: `go test -race -timeout=60s ./...` +- [x] run linter: `golangci-lint run` +- [x] run formatter: `~/.claude/format.sh` +- [x] verify test coverage: `go test -cover ./...` (target 80%+) + +### Task 10: [Final] Update documentation and finalize + +- [x] update CLAUDE.md if new patterns discovered +- [x] move this plan to `docs/plans/completed/` + +## Technical Details + +### Timestamp fix + +Docker API `Time` field is Unix seconds, `TimeNano` is full nanosecond-precision Unix timestamp. + +Current (wrong): +```go +TS: time.Unix(dockerEvent.Time/1000, dockerEvent.TimeNano) +TS: time.Unix(c.Created/1000, 0) +``` + +Fixed (with fallback for older Docker API versions where TimeNano may be zero): +```go +// activate path — prefer nanosecond precision with fallback +ts := time.Unix(0, dockerEvent.TimeNano) +if dockerEvent.TimeNano == 0 { + ts = time.Unix(dockerEvent.Time, 0) +} +TS: ts + +// emitRunningContainers path — Created is Unix seconds +TS: time.Unix(c.Created, 0) +``` + +### makeLogWriters error handling + +Change signature from: +```go +func makeLogWriters(opts *cliOpts, containerName, group string) (logWriter, errWriter io.WriteCloser) +``` +To: +```go +func makeLogWriters(opts *cliOpts, containerName, group string) (logWriter, errWriter io.WriteCloser, err error) +``` + +### Syslog double-close fix + +When both files and syslog are enabled, the same `syslogWriter` is added to both `logWriters` and `errWriters` slices. Both MultiWriters close it independently. Fix by wrapping the errWriters copy with a no-op closer: + +```go +type writeNopCloser struct { + io.Writer +} + +func (writeNopCloser) Close() error { return nil } +``` + +Add `syslogWriter` directly to `logWriters`, add `writeNopCloser{syslogWriter}` to `errWriters`. + +## Post-Completion + +**Manual verification:** +- test with a live Docker daemon (`TEST_DOCKER=1 go test ./app/ -run Test_Do`) +- verify log output shows correct timestamps for container events