Skip to content

Commit c001761

Browse files
aeneasrory-bot
authored andcommitted
fix: prevent oathkeeper rule fetcher watch deadlock
GitOrigin-RevId: 179003cbf9bffe7e234761a0a20724738cd05f2a
1 parent fad2dc2 commit c001761

4 files changed

Lines changed: 28 additions & 8 deletions

File tree

oryx/watcherx/definitions.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"context"
88
"fmt"
99
"net/url"
10+
11+
"github.com/pkg/errors"
1012
)
1113

1214
type (
@@ -22,6 +24,7 @@ type (
2224
DispatchNow() (<-chan int, error)
2325
}
2426
dispatcher struct {
27+
ctx context.Context
2528
trigger chan struct{}
2629
done chan int
2730
}
@@ -42,8 +45,9 @@ func (e *errSchemeUnknown) Error() string {
4245
return fmt.Sprintf("unknown scheme '%s' to watch", e.scheme)
4346
}
4447

45-
func newDispatcher() *dispatcher {
48+
func newDispatcher(ctx context.Context) *dispatcher {
4649
return &dispatcher{
50+
ctx: ctx,
4751
trigger: make(chan struct{}),
4852
done: make(chan int),
4953
}
@@ -53,8 +57,15 @@ func (d *dispatcher) DispatchNow() (<-chan int, error) {
5357
if d.trigger == nil {
5458
return nil, ErrWatcherNotRunning
5559
}
56-
d.trigger <- struct{}{}
57-
return d.done, nil
60+
// The trigger send must respect cancellation. Once the watcher's context is
61+
// done its receiver goroutine has returned, so a bare send would block
62+
// forever and wedge the caller.
63+
select {
64+
case d.trigger <- struct{}{}:
65+
return d.done, nil
66+
case <-d.ctx.Done():
67+
return nil, errors.WithStack(ErrWatcherNotRunning)
68+
}
5869
}
5970

6071
func Watch(ctx context.Context, u *url.URL, c EventChannel) (Watcher, error) {

oryx/watcherx/directory.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func WatchDirectory(ctx context.Context, dir string, c EventChannel) (Watcher, e
3535
}
3636

3737
dw := &directoryWatcher{
38-
dispatcher: newDispatcher(),
38+
dispatcher: newDispatcher(ctx),
3939
c: c,
4040
dir: dir,
4141
subDirs: subDirs,

oryx/watcherx/file.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func WatchFile(ctx context.Context, file string, c EventChannel) (Watcher, error
3939
return nil, errors.WithStack(err)
4040
}
4141
}
42-
d := newDispatcher()
42+
d := newDispatcher(ctx)
4343
go streamFileEvents(ctx, watcher, c, d.trigger, d.done, file, resolvedFile)
4444
return d, nil
4545
}

rule/fetcher_default.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,12 @@ func (f *FetcherDefault) watchLocalFiles(ctx context.Context) {
102102
if cancel, ok := f.cancelWatchers[fp]; !ok {
103103
// watch all files we are not yet watching
104104
repoChanged = true
105-
ctx, cancelWatchers[fp] = context.WithCancel(ctx)
106-
w, err := watcherx.WatchFile(ctx, fp, f.events)
105+
// Derive each file's context from the caller's ctx, not from the
106+
// previous iteration's, so cancelling one file's watcher does not
107+
// cancel the others.
108+
var fileCtx context.Context
109+
fileCtx, cancelWatchers[fp] = context.WithCancel(ctx)
110+
w, err := watcherx.WatchFile(fileCtx, fp, f.events)
107111
if err != nil {
108112
f.registry.Logger().WithError(err).WithField("file", fp).Error("Unable to watch file, ignoring it.")
109113
continue
@@ -144,6 +148,12 @@ func (f *FetcherDefault) watchLocalFiles(ctx context.Context) {
144148
}
145149

146150
func (f *FetcherDefault) Watch(ctx context.Context) error {
151+
// Start the local update consumer before watching files. watchLocalFiles
152+
// forces an initial read via DispatchNow, which sends on the unbuffered
153+
// f.events channel; without a reader already running, that send can block
154+
// and deadlock the watcher setup.
155+
go f.processLocalUpdates(ctx)
156+
147157
f.watchLocalFiles(ctx)
148158

149159
getRemoteRepos := func() map[url.URL]struct{} {
@@ -191,7 +201,6 @@ func (f *FetcherDefault) Watch(ctx context.Context) error {
191201
remoteRepos = newRemoteRepos
192202
})
193203

194-
go f.processLocalUpdates(ctx)
195204
return nil
196205
}
197206

0 commit comments

Comments
 (0)