Skip to content

Commit e52a699

Browse files
leehinmanmergify[bot]
authored andcommitted
handle PreInit error case for runReady.Close (#51911)
* handle PreInit error case for runReady.Close * fix fmt issue (cherry picked from commit 397822f) # Conflicts: # filebeat/beater/stop_test.go
1 parent c9fd1ae commit e52a699

2 files changed

Lines changed: 111 additions & 0 deletions

File tree

filebeat/beater/filebeat.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,7 @@ func (fb *Filebeat) Run(b *beat.Beat) error {
294294
// Start the check-in loop, so Filebeat can respond to Elastic Agent,
295295
// but it won't start any inputs/output
296296
if err := b.Manager.PreInit(); err != nil {
297+
fb.runReady.Close()
297298
return err
298299
}
299300

filebeat/beater/stop_test.go

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
// Licensed to Elasticsearch B.V. under one or more contributor
2+
// license agreements. See the NOTICE file distributed with
3+
// this work for additional information regarding copyright
4+
// ownership. Elasticsearch B.V. licenses this file to you under
5+
// the Apache License, Version 2.0 (the "License"); you may
6+
// not use this file except in compliance with the License.
7+
// You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package beater
19+
20+
import (
21+
"errors"
22+
"testing"
23+
"time"
24+
25+
"github.com/stretchr/testify/require"
26+
27+
"github.com/elastic/beats/v7/filebeat/fileset"
28+
"github.com/elastic/beats/v7/libbeat/beat"
29+
"github.com/elastic/beats/v7/libbeat/beatmonitoring"
30+
"github.com/elastic/beats/v7/libbeat/management"
31+
"github.com/elastic/elastic-agent-libs/logp"
32+
)
33+
34+
// TestRunClosesRunReadyOnPreInitFailure guards against the regression in
35+
// commit 00068f7 where the defer fb.runReady.Close() was placed after the
36+
// PreInit call, leaving runReady unclosed when PreInit fails. An unclosed
37+
// runReady causes StopWithContext to wait the full five-second timeout.
38+
func TestRunClosesRunReadyOnPreInitFailure(t *testing.T) {
39+
preInitErr := errors.New("preinit failed")
40+
fb := &Filebeat{
41+
done: make(chan struct{}),
42+
runReady: &closeOnce{ch: make(chan struct{})},
43+
logger: logp.NewNopLogger(),
44+
moduleRegistry: new(fileset.ModuleRegistry),
45+
}
46+
b := &beat.Beat{
47+
Info: beat.Info{Logger: logp.NewNopLogger()},
48+
Manager: &preinitFailManager{err: preInitErr},
49+
Monitoring: beatmonitoring.NewMonitoring(),
50+
}
51+
52+
err := fb.Run(b)
53+
require.ErrorIs(t, err, preInitErr)
54+
55+
select {
56+
case <-fb.runReady.ch:
57+
// runReady was closed — StopWithContext will not block
58+
default:
59+
t.Fatal("runReady was not closed after PreInit failure; StopWithContext would wait five seconds")
60+
}
61+
}
62+
63+
// preinitFailManager is a management.Manager stub whose PreInit returns an
64+
// error. Only the methods called by Run before PreInit are implemented; any
65+
// other call panics, catching unexpected code paths during the test.
66+
type preinitFailManager struct {
67+
management.Manager // zero-valued; panics on any unimplemented method
68+
err error
69+
}
70+
71+
func (m *preinitFailManager) Enabled() bool { return true }
72+
func (m *preinitFailManager) PreInit() error { return m.err }
73+
func (m *preinitFailManager) RegisterDiagnosticHook(_, _, _, _ string, _ management.DiagnosticHook) {
74+
}
75+
76+
// TestStopWaitsForRunReady proves that Stop does not close the done channel
77+
// until Run has closed runReady (i.e., reached waitFinished.Wait).
78+
// This guards against a race where the OTel collector calls Shutdown before
79+
// the beat's Run goroutine has initialised its shutdown-signal machinery.
80+
func TestStopWaitsForRunReady(t *testing.T) {
81+
fb := &Filebeat{
82+
done: make(chan struct{}),
83+
runReady: &closeOnce{ch: make(chan struct{})},
84+
logger: logp.NewNopLogger(),
85+
}
86+
87+
stopDone := make(chan struct{})
88+
go func() {
89+
defer close(stopDone)
90+
fb.Stop()
91+
}()
92+
93+
// done must still be open: Stop is waiting for runReady to be closed.
94+
select {
95+
case <-fb.done:
96+
t.Fatal("Stop closed done before Run closed runReady")
97+
default:
98+
}
99+
100+
// Simulate Run() reaching the waitFinished.Wait() call.
101+
fb.runReady.Close()
102+
103+
select {
104+
case <-fb.done:
105+
case <-time.After(500 * time.Millisecond):
106+
t.Fatal("Stop did not close done after runReady was closed")
107+
}
108+
109+
<-stopDone
110+
}

0 commit comments

Comments
 (0)