From e322622ebc1d2771596d2b18ba8961417d339473 Mon Sep 17 00:00:00 2001 From: JonahMMay Date: Tue, 29 Sep 2026 02:58:04 +0000 Subject: [PATCH 1/2] fix(runtime): stop Host() re-dialing under the lock; bump grpc; harden CI - pluginHostState.host() returns the client dialed at bind time and no longer retries broker.Dial while holding s.mu. After a failed bind-time dial the host's connection info has expired, so the retry could only fail, and it blocked every concurrent Host() caller for up to go-plugin's five-second wait. Host() docs now say it never dials. - Bump google.golang.org/grpc v1.82.1 -> v1.83.2 (GO-2026-6348, -6443). - Fix pre-existing errcheck in pkg/pluginsdk/httpclient. - CI: pin actions to SHAs, pin golangci-lint v2.14.0, no persisted credentials in non-pushing release jobs; CONTRIBUTING lists the lint and coverage commands CI runs. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 14 ++++----- .github/workflows/release.yml | 13 +++++---- CONTRIBUTING.md | 8 ++++++ go.mod | 10 +++---- go.sum | 40 +++++++++++++------------- pkg/pluginsdk/httpclient/httpclient.go | 2 +- pkg/pluginsdk/runtime/runtime.go | 11 ++++--- 7 files changed, 56 insertions(+), 42 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0cd1802..0e00461 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,20 +21,20 @@ jobs: name: Go lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false fetch-depth: 0 - - uses: actions/setup-go@v7 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod cache: true - name: golangci-lint - uses: golangci/golangci-lint-action@v9 + uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 with: - version: latest + version: v2.14.0 only-new-issues: true args: --timeout=5m @@ -42,11 +42,11 @@ jobs: name: Go tests + coverage runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - - uses: actions/setup-go@v7 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod cache: true @@ -75,7 +75,7 @@ jobs: - name: Upload coverage profile if: always() - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: coverage-out path: coverage.out diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f27640d..71ae852 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -16,7 +16,7 @@ jobs: version: ${{ steps.version.outputs.version }} tag: ${{ steps.version.outputs.tag }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 0 @@ -51,11 +51,12 @@ jobs: needs: version runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ needs.version.outputs.tag }} + persist-credentials: false - - uses: actions/setup-go@v7 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version: "1.26" @@ -97,10 +98,12 @@ jobs: needs: [version, test] runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: Create GitHub Release - uses: softprops/action-gh-release@v3 + uses: softprops/action-gh-release@efb35369e0ad2afab669f228072c1b0d510eae64 # v3.0.3 with: tag_name: ${{ needs.version.outputs.tag }} generate_release_notes: true diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e2e2361..4f31bd0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,8 +35,16 @@ go build ./examples/hello-scheduled-task go build ./examples/hello-runtime-host go build ./examples/hello-network-access gofmt -l . +golangci-lint run ./... +go test $(go list ./pkg/pluginsdk/... | grep -vE '/(runtimedefault|runtime)$') -count=1 -covermode=atomic -coverprofile=coverage.out +./scripts/check-coverage.sh coverage.out ``` +CI runs golangci-lint v2.14.0 and enforces a 95% statement coverage floor +(`scripts/check-coverage.sh`) over `pkg/pluginsdk/...`, excluding the +`runtime` and `runtimedefault` wiring packages; the last three commands +reproduce those checks. + For protobuf or generated-code changes, also run: ```sh diff --git a/go.mod b/go.mod index db796a7..5abdf78 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/hashicorp/go-hclog v1.6.3 github.com/hashicorp/go-plugin v1.7.0 github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 - google.golang.org/grpc v1.82.1 + google.golang.org/grpc v1.83.2 google.golang.org/protobuf v1.36.11 ) @@ -17,8 +17,8 @@ require ( github.com/mattn/go-colorable v0.1.12 // indirect github.com/mattn/go-isatty v0.0.17 // indirect github.com/oklog/run v1.1.0 // indirect - golang.org/x/net v0.55.0 // indirect - golang.org/x/sys v0.45.0 // indirect - golang.org/x/text v0.37.0 // indirect - google.golang.org/genproto/googleapis/rpc v0.0.0-20260414002931-afd174a4e478 // indirect + golang.org/x/net v0.58.0 // indirect + golang.org/x/sys v0.47.0 // indirect + golang.org/x/text v0.41.0 // indirect + google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect ) diff --git a/go.sum b/go.sum index ac30320..6457c98 100644 --- a/go.sum +++ b/go.sum @@ -45,34 +45,34 @@ github.com/stretchr/testify v1.7.2 h1:4jaiDzPyXQvSd7D0EjG45355tLlV3VOECpq10pLC+8 github.com/stretchr/testify v1.7.2/go.mod h1:R6va5+xMeoiuVRoj+gSkQ7d3FALtqAAGI1FQKckRals= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= -go.opentelemetry.io/otel v1.43.0 h1:mYIM03dnh5zfN7HautFE4ieIig9amkNANT+xcVxAj9I= -go.opentelemetry.io/otel v1.43.0/go.mod h1:JuG+u74mvjvcm8vj8pI5XiHy1zDeoCS2LB1spIq7Ay0= -go.opentelemetry.io/otel/metric v1.43.0 h1:d7638QeInOnuwOONPp4JAOGfbCEpYb+K6DVWvdxGzgM= -go.opentelemetry.io/otel/metric v1.43.0/go.mod h1:RDnPtIxvqlgO8GRW18W6Z/4P462ldprJtfxHxyKd2PY= -go.opentelemetry.io/otel/sdk v1.43.0 h1:pi5mE86i5rTeLXqoF/hhiBtUNcrAGHLKQdhg4h4V9Dg= -go.opentelemetry.io/otel/sdk v1.43.0/go.mod h1:P+IkVU3iWukmiit/Yf9AWvpyRDlUeBaRg6Y+C58QHzg= -go.opentelemetry.io/otel/sdk/metric v1.43.0 h1:S88dyqXjJkuBNLeMcVPRFXpRw2fuwdvfCGLEo89fDkw= -go.opentelemetry.io/otel/sdk/metric v1.43.0/go.mod h1:C/RJtwSEJ5hzTiUz5pXF1kILHStzb9zFlIEe85bhj6A= -go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09nk+3A= -go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= -golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= -golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +go.opentelemetry.io/otel v1.44.0 h1:JjwHmHpA4iZ3wBxluu2fbbE7j4kqlE8jXyAyPXH7HqU= +go.opentelemetry.io/otel v1.44.0/go.mod h1:BMgjTHL9WPRlRjL2oZCBTL4whCGtXch2H4BhOPIAyYc= +go.opentelemetry.io/otel/metric v1.44.0 h1:1w0gILTcHdr3YI+ixLyjemwrVnsMURbTZFrSYCdDdmc= +go.opentelemetry.io/otel/metric v1.44.0/go.mod h1:8O7hanEPBNgEMmybD3s2VBKcgWOCsA6tzHBPODAiquo= +go.opentelemetry.io/otel/sdk v1.44.0 h1:nHYwb9lK+fJPU/dnT6s7W7Z8itMWyqrnVfbheVYrZ58= +go.opentelemetry.io/otel/sdk v1.44.0/go.mod h1:Osuydd3Se74nqjAKxid74N5eC+jfEqfTegHRnq58oK0= +go.opentelemetry.io/otel/sdk/metric v1.44.0 h1:3LlKgI+VjbVsjNRFZJZAJ30WjXC5VkNRks6si09iEfI= +go.opentelemetry.io/otel/sdk/metric v1.44.0/go.mod h1:5B5pMARnXxKhltooO4xUuCBorl65a4EpnTalObqOigA= +go.opentelemetry.io/otel/trace v1.44.0 h1:jxF5CsGYCe74MCRx2X4g7WsY/VBKRqqpNvXlX/6gtIk= +go.opentelemetry.io/otel/trace v1.44.0/go.mod h1:oLl1jrMQAVo6v3GAggN+1VH9VIz9iUSvW53sW1Q8PIE= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= golang.org/x/sys v0.0.0-20200116001909-b77594299b42/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200223170610-d5e6a3e2c0ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220503163025-988cb79eb6c6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= -golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= -golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= +golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260414002931-afd174a4e478 h1:RmoJA1ujG+/lRGNfUnOMfhCy5EipVMyvUE+KNbPbTlw= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260414002931-afd174a4e478/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= -google.golang.org/grpc v1.82.1 h1:NnAxzGRA0677vCa4BUkOAnO5+FfQqVl9iUXeD0IqcGE= -google.golang.org/grpc v1.82.1/go.mod h1:yzTZ1TB1Z3SG+LIYaI+WiE8D5+PZ3ArnrSp8zF3+/ZA= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa h1:mZHHdPZl0dbGHCflZgAq/Q468DWVFcU2whhB2KAo8fk= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= +google.golang.org/grpc v1.83.2 h1:EManeRomTObA0BU7I8vXgg/78uE5MJ9M8B39EX2WscU= +google.golang.org/grpc v1.83.2/go.mod h1:YPI1hK3kDked6iHvgX3tR0y+nX/qpMFKhPgFsokw1S8= google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/pkg/pluginsdk/httpclient/httpclient.go b/pkg/pluginsdk/httpclient/httpclient.go index 709833c..0c78473 100644 --- a/pkg/pluginsdk/httpclient/httpclient.go +++ b/pkg/pluginsdk/httpclient/httpclient.go @@ -110,7 +110,7 @@ func (c *Client) DoJSON(ctx context.Context, method, path string, body, dest any if err != nil { return fmt.Errorf("httpclient: request failed: %w", err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { raw, _ := io.ReadAll(io.LimitReader(resp.Body, maxResponseBody)) diff --git a/pkg/pluginsdk/runtime/runtime.go b/pkg/pluginsdk/runtime/runtime.go index 8388128..768dfd7 100644 --- a/pkg/pluginsdk/runtime/runtime.go +++ b/pkg/pluginsdk/runtime/runtime.go @@ -295,10 +295,13 @@ func (s *pluginHostState) dialLocked() { s.client = runtimehost.NewClient(conn) } +// host returns the client dialed at bind time. It never dials: if the +// bind-time dial failed, the host's connection info has already expired (see +// setBrokerID), so a retry here could only fail again, and it would block every +// concurrent Host() caller on s.mu for up to go-plugin's five-second wait. func (s *pluginHostState) host() *runtimehost.Client { s.mu.Lock() defer s.mu.Unlock() - s.dialLocked() return s.client } @@ -311,11 +314,11 @@ func SetHostBrokerID(id uint32) { pluginHost.setBrokerID(id) } // Host returns a runtimehost.Client connected to the prairie host. Returns // nil before the host has invoked Runtime.BindHostBroker (i.e. very briefly // during plugin startup) or if the broker dial failed at bind time. -// Capability handlers should treat nil as transient and either skip or +// Capability handlers should treat nil as unavailable and either skip or // surface a temporary error. // -// The first successful call dials the host broker stream and caches the -// *runtimehost.Client; later calls reuse the same client. +// The client is dialed once, when the host binds the broker stream, and every +// call returns that same *runtimehost.Client. Host never dials. func Host() *runtimehost.Client { return pluginHost.host() } func Serve(cfg ServeConfig) { From e148fd85e5788b6002383ab331c70ff20578ba10 Mon Sep 17 00:00:00 2001 From: JonahMMay Date: Tue, 29 Sep 2026 03:09:20 +0000 Subject: [PATCH 2/2] test(runtime): host() must not redial after a failed bind-time dial A zero-value GRPCBroker panics or blocks on Dial, so this fails if host() ever dials again. Verified it fails against the previous host(). Co-Authored-By: Claude Opus 5.5 (1M context) --- pkg/pluginsdk/runtime/host_state_test.go | 33 ++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 pkg/pluginsdk/runtime/host_state_test.go diff --git a/pkg/pluginsdk/runtime/host_state_test.go b/pkg/pluginsdk/runtime/host_state_test.go new file mode 100644 index 0000000..c4eb146 --- /dev/null +++ b/pkg/pluginsdk/runtime/host_state_test.go @@ -0,0 +1,33 @@ +package runtime + +import ( + "testing" + "time" + + "github.com/hashicorp/go-plugin" +) + +// After a failed bind-time dial the state holds a broker and a stream id but +// no client. host() must return nil without dialing again: the host's +// connection info has expired by then, and a retry would hold s.mu for up to +// go-plugin's five-second wait. The zero-value broker has no stream plumbing, +// so any Dial on it panics or blocks and this test fails. +func TestHostDoesNotRedialAfterFailedBind(t *testing.T) { + s := &pluginHostState{broker: &plugin.GRPCBroker{}, brokerID: 7} + + done := make(chan any, 1) + go func() { + defer func() { done <- recover() }() + if got := s.host(); got != nil { + t.Errorf("host() = %v, want nil after a failed bind-time dial", got) + } + }() + select { + case r := <-done: + if r != nil { + t.Fatalf("host() dialed the broker: %v", r) + } + case <-time.After(time.Second): + t.Fatal("host() blocked; it must not dial the broker") + } +}