Skip to content

Commit dc9a5ff

Browse files
committed
chore: post-PR-triad fixes (review + simplify)
Code review (pr-review-toolkit): - rpc.go: drop dead `resp.Message = err.Error()` writes — Connect discards the response on any non-nil error, so the assignments were never observed by callers. Hoist resp allocation past the err guard so the success path is the only allocator. - plugin.go: introduce errPluginNotFound sentinel and an errors.Is-based mapping helper (connectCodeFor) so the rpc handler returns connect.CodeNotFound for missing plugins instead of CodeInternal. CodeInternal is still the default for any other error. - plugin.go: rewrite stale doc on the private status()/ready() helpers (the prior wording was a copy-paste leftover from the goridge era describing a public method that no longer exists). - tests/plugin_test.go: drive-by lint cleanups — 3 more http.NewRequest -> NewRequestWithContext(t.Context(), ...) sites surfaced after fixing the first batch. - tests/plugin_test.go: assert connect.CodeNotFound on the StatusNonExistent / ReadyNonExistent subtests instead of string- matching err.Error() — the typed sentinel makes the wire-code check more robust. - tests/plugin_test.go: TestRPCNonExistentPlugin no longer registers http+server plugins. The test only exercises the "no such plugin" path of the status RPC handler, doesn't need any Checker/Readiness providers, and the previous setup pulled in a PHP worker dependency that broke the test in PHP-less environments. Simplify: - Tighten doc comments on errPluginNotFound and connectCodeFor (cut WHAT, keep WHY). - Drop the now-unused errors.Op constants in status() and ready() after dropping the errors.E wrap on the not-found path (the wrap was hiding the sentinel from errors.Is — *errors.Error has no Unwrap method).
1 parent 558fc67 commit dc9a5ff

3 files changed

Lines changed: 33 additions & 21 deletions

File tree

plugin.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package status
33
import (
44
"context"
55
stderr "errors"
6+
"fmt"
67
"log/slog"
78
"net/http"
89
"sync"
@@ -16,6 +17,9 @@ import (
1617
"github.com/roadrunner-server/errors"
1718
)
1819

20+
// Mapped to connect.CodeNotFound by the rpc handler; other failures surface as CodeInternal.
21+
var errPluginNotFound = stderr.New("no such plugin")
22+
1923
const (
2024
// PluginName declares public plugin name.
2125
PluginName = "status"
@@ -133,25 +137,25 @@ func (c *Plugin) Stop(_ context.Context) error {
133137
return nil
134138
}
135139

136-
// Status returns a Checker interface implementation
137-
// Reset named service.
138-
// This is not a Status interface implementation
140+
// status looks up the named plugin in the status registry and delegates to its
141+
// Checker.Status. Returns errPluginNotFound (wrapped) if the name is not
142+
// registered.
139143
func (c *Plugin) status(name string) (*status.Status, error) {
140-
const op = errors.Op("checker_plugin_status")
141144
svc, ok := c.statusRegistry[name]
142145
if !ok {
143-
return nil, errors.E(op, errors.Errorf("no such plugin: %s", name))
146+
return nil, fmt.Errorf("%w: %s", errPluginNotFound, name)
144147
}
145148

146149
return svc.Status()
147150
}
148151

149-
// ready is used to provide a readiness check for the plugin
152+
// ready looks up the named plugin in the readiness registry and delegates to
153+
// its Readiness.Ready. Returns errPluginNotFound (wrapped) if the name is not
154+
// registered.
150155
func (c *Plugin) ready(name string) (*status.Status, error) {
151-
const op = errors.Op("checker_plugin_ready")
152156
svc, ok := c.readyRegistry[name]
153157
if !ok {
154-
return nil, errors.E(op, errors.Errorf("no such plugin: %s", name))
158+
return nil, fmt.Errorf("%w: %s", errPluginNotFound, name)
155159
}
156160

157161
return svc.Ready()

rpc.go

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package status
22

33
import (
44
"context"
5+
stderr "errors"
56
"log/slog"
67

78
"connectrpc.com/connect"
@@ -14,19 +15,26 @@ type rpc struct {
1415
log *slog.Logger
1516
}
1617

18+
// connectCodeFor returns CodeNotFound for a missing plugin, CodeInternal otherwise.
19+
func connectCodeFor(err error) connect.Code {
20+
if stderr.Is(err, errPluginNotFound) {
21+
return connect.CodeNotFound
22+
}
23+
return connect.CodeInternal
24+
}
25+
1726
// Status returns the current status of the provided plugin.
1827
func (r *rpc) Status(_ context.Context, req *connect.Request[statusV2.StatusRequest]) (*connect.Response[statusV2.StatusResponse], error) {
1928
const op = errors.Op("checker_rpc_status")
2029
plugin := req.Msg.GetPlugin()
2130
r.log.Debug("Status method was invoked", "plugin", plugin)
2231

23-
resp := &statusV2.StatusResponse{}
2432
st, err := r.srv.status(plugin)
2533
if err != nil {
26-
resp.Message = err.Error()
27-
return nil, connect.NewError(connect.CodeInternal, errors.E(op, err))
34+
return nil, connect.NewError(connectCodeFor(err), errors.E(op, err))
2835
}
2936

37+
resp := &statusV2.StatusResponse{}
3038
if st != nil {
3139
resp.Code = int64(st.Code)
3240
r.log.Debug("status code", "code", st.Code)
@@ -42,13 +50,12 @@ func (r *rpc) Ready(_ context.Context, req *connect.Request[statusV2.StatusReque
4250
plugin := req.Msg.GetPlugin()
4351
r.log.Debug("Ready method was invoked", "plugin", plugin)
4452

45-
resp := &statusV2.StatusResponse{}
4653
st, err := r.srv.ready(plugin)
4754
if err != nil {
48-
resp.Message = err.Error()
49-
return nil, connect.NewError(connect.CodeInternal, errors.E(op, err))
55+
return nil, connect.NewError(connectCodeFor(err), errors.E(op, err))
5056
}
5157

58+
resp := &statusV2.StatusResponse{}
5259
if st != nil {
5360
resp.Code = int64(st.Code)
5461
r.log.Debug("status code", "code", st.Code)

tests/plugin_test.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -669,13 +669,14 @@ func TestRPCNonExistentPlugin(t *testing.T) {
669669
Path: "configs/.rr-status-init.yaml",
670670
}
671671

672+
// Minimal stack: this test only verifies the "no such plugin" path of the
673+
// status RPC handler, so no Checker/Readiness providers are needed and
674+
// http/server are dropped to keep the test PHP-independent.
672675
sp := &status.Plugin{}
673676
err := cont.RegisterAll(
674677
cfg,
675678
&rpcPlugin.Plugin{},
676679
&logger.Plugin{},
677-
&server.Plugin{},
678-
&httpPlugin.Plugin{},
679680
sp,
680681
)
681682
assert.NoError(t, err)
@@ -728,14 +729,14 @@ func TestRPCNonExistentPlugin(t *testing.T) {
728729
client := newStatusClient(t, "127.0.0.1:6005")
729730
_, err := client.Status(t.Context(), connect.NewRequest(&statusV2.StatusRequest{Plugin: "nonexistent"}))
730731
require.Error(t, err)
731-
assert.Contains(t, err.Error(), "no such plugin")
732+
assert.Equal(t, connect.CodeNotFound, connect.CodeOf(err))
732733
})
733734

734735
t.Run("ReadyNonExistent", func(t *testing.T) {
735736
client := newStatusClient(t, "127.0.0.1:6005")
736737
_, err := client.Ready(t.Context(), connect.NewRequest(&statusV2.StatusRequest{Plugin: "nonexistent"}))
737738
require.Error(t, err)
738-
assert.Contains(t, err.Error(), "no such plugin")
739+
assert.Equal(t, connect.CodeNotFound, connect.CodeOf(err))
739740
})
740741

741742
stopCh <- struct{}{}
@@ -830,7 +831,7 @@ func doHTTPReq(t *testing.T) {
830831
}
831832

832833
func checkHTTPReadiness2(t *testing.T) {
833-
req, err := http.NewRequest("GET", "http://127.0.0.1:34334/ready?plugin=http&plugin=rpc", nil)
834+
req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:34334/ready?plugin=http&plugin=rpc", nil)
834835
assert.NoError(t, err)
835836

836837
r, err := http.DefaultClient.Do(req)
@@ -852,7 +853,7 @@ func checkHTTPReadiness2(t *testing.T) {
852853
}
853854

854855
func checkHTTPReadinessAll(t *testing.T) {
855-
req, err := http.NewRequest("GET", "http://127.0.0.1:34333/ready", nil)
856+
req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:34333/ready", nil)
856857
assert.NoError(t, err)
857858

858859
r, err := http.DefaultClient.Do(req)
@@ -869,7 +870,7 @@ func checkHTTPReadinessAll(t *testing.T) {
869870
}
870871

871872
func checkHTTPReadiness(t *testing.T) {
872-
req, err := http.NewRequest("GET", "http://127.0.0.1:34333/ready?plugin=http&plugin=rpc", nil)
873+
req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:34333/ready?plugin=http&plugin=rpc", nil)
873874
assert.NoError(t, err)
874875

875876
r, err := http.DefaultClient.Do(req)

0 commit comments

Comments
 (0)