Skip to content

feat(execbroker): propagate middleware errors #167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
luoliwoshang merged 1 commit into from
Aug 13, 2026
Merged

feat(execbroker): propagate middleware errors #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions internal/execbroker/execbroker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ type Request struct {
Stderr io.Writer
}

// Middleware may rewrite a command before execution.
type Middleware func(Request) Request
// Middleware may rewrite a command before execution. Returning an error
// prevents the command from starting.
type Middleware func(Request) (Request, error)

// Scope supplies process-independent defaults for commands created while fn
// runs. Custom fields already set on a command take precedence.
Expand Down Expand Up @@ -78,16 +79,22 @@ func Println(a ...any) (int, error) {

// Command is the brokered equivalent of exec.Command.
func Command(name string, args ...string) *exec.Cmd {
req := rewrite(Request{Name: name, Args: clone(args)})
req, err := rewrite(Request{Name: name, Args: clone(args)})
cmd := exec.Command(req.Name, req.Args...)
if err != nil {
cmd.Err = err
}
apply(cmd, req)

@fennoai fennoai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On the error path, Command/CommandContext still build the cmd from the middleware-mutated req.Name (running a LookPath) and then run apply(), copying the mutated env/dir/streams onto a command that was rejected. This is functionally safe because cmd.Err short-circuits Start()/Run(), but it's asymmetric with Run (which returns early at line 121 without mutating the caller's cmd) and exposes rejected-command mutations to any caller that inspects cmd before running it. Consider short-circuiting on error to keep the three paths symmetric, e.g.

if err != nil {
    cmd := exec.Command(name, args...)
    cmd.Err = err
    return cmd
}

return cmd
}

// CommandContext is the brokered equivalent of exec.CommandContext.
func CommandContext(ctx context.Context, name string, args ...string) *exec.Cmd {
req := rewrite(Request{Name: name, Args: clone(args)})
req, err := rewrite(Request{Name: name, Args: clone(args)})
cmd := exec.CommandContext(ctx, req.Name, req.Args...)
if err != nil {
cmd.Err = err
}
apply(cmd, req)
return cmd
}
Expand All @@ -101,7 +108,7 @@ func Run(cmd *exec.Cmd) error {
name = cmd.Args[0]
args = cmd.Args[1:]
}
req := rewrite(Request{
req, err := rewrite(Request{
Name: name,
Args: clone(args),
Env: clone(cmd.Env),
Expand All @@ -110,6 +117,9 @@ func Run(cmd *exec.Cmd) error {
Stdout: cmd.Stdout,
Stderr: cmd.Stderr,
})
if err != nil {
return err
}
resolved := exec.Command(req.Name, req.Args...)
cmd.Path = resolved.Path
cmd.Args = resolved.Args
Expand All @@ -118,7 +128,7 @@ func Run(cmd *exec.Cmd) error {
return cmd.Run()
}

func rewrite(req Request) Request {
func rewrite(req Request) (Request, error) {
req.Args = clone(req.Args)
req.Env = clone(req.Env)

Expand All @@ -143,14 +153,19 @@ func rewrite(req Request) Request {
req.Stderr = scope.Stderr
}
if scope.Middleware != nil {
req = scope.Middleware(req)
var err error
req, err = scope.Middleware(req)

@fennoai fennoai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The user-supplied middleware now runs while scopeMu.RLock() is held (acquired at line 135, released at 159/164). Since this change lets middleware run arbitrary logic (and return errors), the re-entrancy risk is worth flagging: if a middleware calls back into Do (which takes scopeMu.Lock()) on the same goroutine it will deadlock, and holding the shared read lock for the full duration of arbitrary user code lengthens lock-hold time on the hot Command/CommandContext/Run path. Consider snapshotting the needed Scope fields (including the Middleware value) under the lock, RUnlock, then invoking middleware outside the critical section.

if err != nil {
scopeMu.RUnlock()
return req, err
}
}
}
scopeMu.RUnlock()

req.Args = clone(req.Args)
req.Env = clone(req.Env)
return req
return req, nil
}

func apply(cmd *exec.Cmd, req Request) {
Expand Down
56 changes: 54 additions & 2 deletions internal/execbroker/execbroker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ func TestPrintlnScope(t *testing.T) {
}

func TestCommandMiddleware(t *testing.T) {
err := Do(Scope{Middleware: func(req Request) Request {
err := Do(Scope{Middleware: func(req Request) (Request, error) {
req.Name = "replacement"
req.Args = append([]string{"prefix"}, req.Args...)
req.Env = []string{"KEY=value"}
req.Dir = "/work"
return req
return req, nil
}}, func() error {
cmd := Command("original", "arg")
if got, want := cmd.Args, []string{"replacement", "prefix", "arg"}; !reflect.DeepEqual(got, want) {
Expand All @@ -81,6 +81,58 @@ func TestCommandMiddleware(t *testing.T) {
}
}

func TestMiddlewareErrorStopsCommand(t *testing.T) {
want := errors.New("command rejected")
tests := []struct {
name string
run func(*bytes.Buffer) error
}{
{
name: "Command",
run: func(output *bytes.Buffer) error {
cmd := Command(os.Args[0], "-test.run=TestExecBrokerHelperProcess")
cmd.Env = append(os.Environ(), "EXECBROKER_HELPER=executed")
cmd.Stdout = output
return cmd.Run()
},
},
{
name: "CommandContext",
run: func(output *bytes.Buffer) error {
cmd := CommandContext(context.Background(), os.Args[0], "-test.run=TestExecBrokerHelperProcess")
cmd.Env = append(os.Environ(), "EXECBROKER_HELPER=executed")
cmd.Stdout = output
return cmd.Run()
},
},
{
name: "Run",
run: func(output *bytes.Buffer) error {
cmd := exec.Command(os.Args[0], "-test.run=TestExecBrokerHelperProcess")
cmd.Env = append(os.Environ(), "EXECBROKER_HELPER=executed")
cmd.Stdout = output
return Run(cmd)
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var output bytes.Buffer
err := Do(Scope{Middleware: func(req Request) (Request, error) {
return req, want
}}, func() error {
return tt.run(&output)
})
if !errors.Is(err, want) {
t.Fatalf("error = %v, want %v", err, want)
}
if output.Len() != 0 {
t.Fatalf("command executed with output %q", output.String())
}
})
}
}

func TestCommandContext(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
Expand Down
8 changes: 4 additions & 4 deletions x/pkgconfig/pkgconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,12 @@ func TestQueries(t *testing.T) {
var request execbroker.Request
var output string
err := execbroker.Do(execbroker.Scope{
Middleware: func(req execbroker.Request) execbroker.Request {
Middleware: func(req execbroker.Request) (execbroker.Request, error) {
request = req
req.Name = os.Args[0]
req.Args = []string{"-test.run=TestLookupHelperProcess"}
req.Env = append(os.Environ(), "GO_WANT_PKGCONFIG_HELPER=1")
return req
return req, nil
},
}, func() error {
var err error
Expand Down Expand Up @@ -92,15 +92,15 @@ func TestQueryErrors(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := execbroker.Do(execbroker.Scope{
Middleware: func(req execbroker.Request) execbroker.Request {
Middleware: func(req execbroker.Request) (execbroker.Request, error) {
req.Name = os.Args[0]
req.Args = []string{"-test.run=TestLookupHelperProcess"}
req.Env = append(os.Environ(),
"GO_WANT_PKGCONFIG_HELPER=1",
"GO_PKGCONFIG_HELPER_FAIL=1",
"GO_PKGCONFIG_HELPER_STDERR="+tt.detail,
)
return req
return req, nil
},
}, func() error {
_, err := Libs("demo")
Expand Down
Loading