Skip to content

Commit 16133bb

Browse files
committed
fix(syslog): don't close the shared acquisition output channel
The syslog datasource's Stream() does `defer close(out)` on the pipeline output channel. That channel is shared: StartAcquisition() passes the same `out` to every configured datasource, and it is owned by the acquisition orchestrator, which never closes it (shutdown is driven by context cancellation, then drainChan). syslog is the only datasource that closes it. When syslog and another datasource run together, syslog's close races with the other datasource still sending. This is reproducible with the AppSec datasource: on shutdown/reload, an in-flight out-of-band request in appsec_runner's handleOutBandInterrupt does `r.outChan <- evt` after syslog has closed the channel, panicking with "send on closed channel" (seen in production on 1.7.8 under a syslog+appsec config). Remove the close. No consumer relies on it: runParse ranges via a `select { case <-ctx.Done(); case ev := <-input }` loop and drainChan is a non-blocking best-effort drain, so both terminate on context cancellation regardless. This aligns syslog with every other datasource (file, journalctl, docker, ...), none of which close the shared channel. The "never close out" contract is documented on the RestartableStreamer and Tailer interfaces. Adds TestDatasourcesDoNotCloseSharedOutputChannel: a static check (parses the AST of every pkg/acquisition/modules/<name> directory, no per-module config or runtime needed) asserting no Stream/StreamingAcquisition implementation calls close(out). It covers every present and future datasource automatically and fails naming the exact file/line if the bug recurs anywhere.
1 parent 06be627 commit 16133bb

3 files changed

Lines changed: 102 additions & 2 deletions

File tree

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
package modules
2+
3+
import (
4+
"go/ast"
5+
"go/parser"
6+
"go/token"
7+
"os"
8+
"path/filepath"
9+
"runtime"
10+
"testing"
11+
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
// TestDatasourcesDoNotCloseSharedOutputChannel statically checks every
16+
// datasource module for `close(out)` inside a Stream() or StreamingAcquisition()
17+
// method. `out` is the pipeline output channel shared across every configured
18+
// datasource (see acquisition.StartAcquisition); it is owned by the acquisition
19+
// orchestrator, not by any individual datasource, so a datasource closing it
20+
// races with any other datasource still sending on it and panics with "send on
21+
// closed channel" (this happened in production with the syslog datasource).
22+
//
23+
// This is independent of any one datasource and covers every module directory
24+
// under pkg/acquisition/modules automatically, including future ones, without
25+
// needing to configure or run any of them.
26+
func TestDatasourcesDoNotCloseSharedOutputChannel(t *testing.T) {
27+
_, thisFile, _, ok := runtime.Caller(0)
28+
require.True(t, ok)
29+
30+
modulesDir := filepath.Dir(thisFile)
31+
32+
entries, err := os.ReadDir(modulesDir)
33+
require.NoError(t, err)
34+
35+
for _, entry := range entries {
36+
if !entry.IsDir() {
37+
continue
38+
}
39+
40+
moduleDir := filepath.Join(modulesDir, entry.Name())
41+
42+
fset := token.NewFileSet()
43+
44+
pkgs, err := parser.ParseDir(fset, moduleDir, func(fi os.FileInfo) bool {
45+
return filepath.Ext(fi.Name()) == ".go"
46+
}, 0)
47+
require.NoError(t, err, "parsing module %s", entry.Name())
48+
49+
for _, pkg := range pkgs {
50+
for path, file := range pkg.Files {
51+
checkFileDoesNotCloseOut(t, fset, entry.Name(), path, file)
52+
}
53+
}
54+
}
55+
}
56+
57+
// checkFileDoesNotCloseOut walks a single file's AST for Stream/StreamingAcquisition
58+
// method declarations and fails if their body contains close(out).
59+
func checkFileDoesNotCloseOut(t *testing.T, fset *token.FileSet, module string, path string, file *ast.File) {
60+
t.Helper()
61+
62+
for _, decl := range file.Decls {
63+
fn, ok := decl.(*ast.FuncDecl)
64+
if !ok || fn.Body == nil {
65+
continue
66+
}
67+
68+
if fn.Name.Name != "Stream" && fn.Name.Name != "StreamingAcquisition" {
69+
continue
70+
}
71+
72+
ast.Inspect(fn.Body, func(n ast.Node) bool {
73+
call, ok := n.(*ast.CallExpr)
74+
if !ok {
75+
return true
76+
}
77+
78+
ident, ok := call.Fun.(*ast.Ident)
79+
if !ok || ident.Name != "close" || len(call.Args) != 1 {
80+
return true
81+
}
82+
83+
arg, ok := call.Args[0].(*ast.Ident)
84+
if ok && arg.Name == "out" {
85+
t.Errorf(
86+
"%s: %s.%s must not close(out) -- it is the shared acquisition "+
87+
"output channel, owned by the orchestrator, not by this datasource (%s:%d)",
88+
module, module, fn.Name.Name, path, fset.Position(call.Pos()).Line,
89+
)
90+
}
91+
92+
return true
93+
})
94+
}
95+
}

pkg/acquisition/modules/syslog/run.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,6 @@ func (s *Source) Stream(ctx context.Context, out chan pipeline.Event) error {
4343
})
4444

4545
g.Go(func() error {
46-
defer close(out)
47-
4846
for {
4947
select {
5048
case <-ctx.Done():

pkg/acquisition/types/types.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,10 @@ type Fetcher interface {
7676
// failures, but treat them as errors. The caller is responsible for supervising
7777
// Stream(), and restarting it as needed. There is currently no way to differentiate
7878
// retryable vs permanent errors.
79+
// - never close the output channel: it is shared and fed by every configured
80+
// datasource, so it is owned by the acquisition orchestrator, not by any single
81+
// Stream() implementation. Closing it here would race with (and can panic) any
82+
// other datasource still sending on it.
7983
type RestartableStreamer interface {
8084
// Start live acquisition (eg, tail a file)
8185
Stream(ctx context.Context, out chan pipeline.Event) error
@@ -84,6 +88,9 @@ type RestartableStreamer interface {
8488
// Tailer has the same pupose as RestartableStreamer (provide ongoing events) but
8589
// is responsible for spawning its own goroutines, and handling errors and retries.
8690
// New datasources are expected to implement RestartableStreamer instead.
91+
//
92+
// As with RestartableStreamer, the output channel is shared and orchestrator-owned:
93+
// an implementation must never close it.
8794
type Tailer interface {
8895
StreamingAcquisition(ctx context.Context, out chan pipeline.Event, acquisTomb *tomb.Tomb) error
8996
}

0 commit comments

Comments
 (0)