diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 862abce0109..25924f8f1c5 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -39,6 +39,7 @@ /wormchain/devnet/ @evan-gray /wormchain/devnet/txverifier @djb15 @johnsaigle @mdulin2 @pleasew8t /wormchain/ts-sdk/ @evan-gray @kev1n-peters @panoel +/linters/ @djb15 @johnsaigle @mdulin2 @pleasew8t @bemic # Protobuf for node diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ee4d306aa2b..b97d286a6e0 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -306,10 +306,8 @@ jobs: run: go install golang.org/x/tools/cmd/goimports@v0.8.0 - name: Formatting checks run: ./scripts/lint.sh -l -g format - - name: Install linters - run: curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.12.2 - name: Run linters - run: make generate && golangci-lint --version && ./scripts/lint.sh -g lint + run: make generate && ./scripts/lint.sh -g lint - name: Ensure generated proto matches run: | rm -rf node/pkg/proto @@ -377,6 +375,19 @@ jobs: - name: Check coverage against baseline run: ./coverage-check + # Run unit tests for the custom Go linters + linter-tests: + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 + with: + go-version: "1.25.10" + - name: Run custom linter tests + run: make -C linters test + # Run Rust lints and tests rust-lint-and-tests: runs-on: ubuntu-24.04 diff --git a/.gitignore b/.gitignore index e711c3f5226..36b73faba47 100644 --- a/.gitignore +++ b/.gitignore @@ -33,4 +33,4 @@ sui/examples/wrapped_coin /coverage-check # Only used for internal testing *.testnet.yaml -coverage.txt +coverage.txt \ No newline at end of file diff --git a/.golangci.yml b/.golangci.yml index ad67134f82d..fe7c74455d6 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,6 +7,8 @@ linters: - bidichk # Ensure that the body is closed on HTTP and websocket conns - bodyclose + # Blocking channel sends + - channelcheck - contextcheck - depguard # Duplicate word usage, such as 'and and' in a comment. @@ -206,6 +208,18 @@ linters: # # Default: false check-exported: true + custom: + channelcheck: + type: "module" + description: Static analysis for go channel issues + settings: + CheckBlockingSends: true + CheckUnbufferedChannels: false + CheckEmptyDefault: true + # msgC sends are intentionally blocking because we don't want to drop observations + IgnoreChannelsByName: + - msgC + - msgChan exclusions: generated: lax presets: @@ -231,6 +245,7 @@ linters: path: node/hack/ # Ignore test files for these tools. - linters: + - channelcheck - contextcheck - dupWord - exhaustruct @@ -408,4 +423,4 @@ formatters: paths: - third_party$ - builtin$ - - examples$ + - examples$ \ No newline at end of file diff --git a/Makefile b/Makefile index bb5d031552d..b3c460fa356 100755 --- a/Makefile +++ b/Makefile @@ -53,9 +53,9 @@ $(BIN)/guardiand: dirs generate github.com/certusone/wormhole/node .PHONY: test-coverage -## Run tests with coverage for node and sdk (matches CI) +## Run tests with coverage for node, and sdk test-coverage: - @echo "Running tests with coverage for node and sdk..." + @echo "Running tests with coverage for node, and sdk..." @set -o pipefail && (cd node && go test -count=1 -v -timeout 5m -race -cover ./...) 2>&1 | tee coverage.txt @set -o pipefail && (cd sdk && go test -count=1 -v -timeout 5m -race -cover ./...) 2>&1 | tee -a coverage.txt diff --git a/cspell-custom-words.txt b/cspell-custom-words.txt index 7cb57281011..39a9dae69f0 100644 --- a/cspell-custom-words.txt +++ b/cspell-custom-words.txt @@ -38,6 +38,8 @@ certusone chainid ChainID Chainlink +channelcheck +channellint Coinspect collateralization colour @@ -71,6 +73,7 @@ Finalizer fogo Fogo fogoshim +footguns forgeable FQTs frontends @@ -142,8 +145,10 @@ monad Monad moonscan moretags +multichecker Neodyme nhooyr +nolint obsv Obsv OP_CHECKMULTISIG @@ -222,6 +227,7 @@ superminority tendermint Tendermint terrad +testdata tokenbridge tokenfactory traceback @@ -233,6 +239,8 @@ txverifier uatom uluna unbond +Unbuffered +unbuffered Uncompromised undelegated undercollateralization diff --git a/linters/.custom-gcl.yml b/linters/.custom-gcl.yml new file mode 100644 index 00000000000..d62aa6aa3c9 --- /dev/null +++ b/linters/.custom-gcl.yml @@ -0,0 +1,6 @@ +version: v2.12.2 +name: wormhole-golangci-lint +destination: ./bin +plugins: + - module: github.com/wormhole-foundation/wormhole/linters/rules/channelcheck + path: ./rules/channelcheck diff --git a/linters/.gitignore b/linters/.gitignore new file mode 100644 index 00000000000..359b8d230ff --- /dev/null +++ b/linters/.gitignore @@ -0,0 +1,7 @@ +# Built binaries. +/bin/ + +# Stray root-level builds, e.g. from a bare `go build ./...` (the Makefile +# targets put these under bin/). +/wormhole-lint +/wormhole-golangci-lint diff --git a/linters/Makefile b/linters/Makefile new file mode 100644 index 00000000000..e9244d899ba --- /dev/null +++ b/linters/Makefile @@ -0,0 +1,36 @@ +# Pin by commit hash rather than tag, so a re-pointed/malleable tag can't change +# what we build. Keep the trailing comment's tag in sync with the hash and with +# the `version:` field in .custom-gcl.yml. +GOLANGCI_LINT_HASH ?= c0d3ddc9cf3faa61a4e378e879ece580256d76e5 # v2.12.2 + +BIN_DIR := bin + +.PHONY: build build-golangci-lint install-golangci-lint test clean + +# Standalone multichecker over every rules/.Analyzer. +build: + go build -o $(BIN_DIR)/wormhole-lint ./cmd/wormhole-lint + +# Pinned upstream golangci-lint, dropped into $(BIN_DIR). Used as the driver +# for `golangci-lint custom`; keep its version aligned with the `version:` +# field in .custom-gcl.yml. +install-golangci-lint: + GOOS= GOARCH= GOBIN=$(abspath $(BIN_DIR)) go install \ + github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_HASH) + +# Custom golangci-lint with our module plugins baked in. Driven by +# .custom-gcl.yml in this directory (golangci-lint custom reads it from the +# cwd); output goes to ./bin per `destination:`. +build-golangci-lint: install-golangci-lint + $(BIN_DIR)/golangci-lint custom + +test: + go test -v ./... + @for mod in $$(find rules -name go.mod); do \ + dir=$$(dirname $$mod); \ + echo "==> testing $$dir"; \ + (cd $$dir && go test -v ./...) || exit $$?; \ + done + +clean: + rm -rf $(BIN_DIR) diff --git a/linters/README.md b/linters/README.md new file mode 100644 index 00000000000..2484cd8eb48 --- /dev/null +++ b/linters/README.md @@ -0,0 +1,125 @@ +# Golang CI Lints + +Custom Go linters used on Wormhole CI. Each linter is a [golangci-lint +module plugin](https://golangci-lint.run/plugins/module-plugins/) and lives +as its own Go module under `rules//`. + +Prefer to use the `release` builds. + +Currently supported linters: + +| Name | Purpose | Features | +| -------------- | -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | +| `channelcheck` | Flag channel usage patterns that can deadlock or block. | • Blocking channel sends outside a `select`
• Unbuffered channel creation (`make(chan T)`)
• Empty `default:` that silently drops a send (advisory, off by default)
• Ignore specific channels | + + +## Build & test +``` +make build # bin/wormhole-lint +make build-golangci-lint # bin/wormhole-golangci-lint +make test # root + each rules/ module +``` + +`make build-golangci-lint` first installs the pinned upstream +`golangci-lint` into `bin/` (per `GOLANGCI_LINT_HASH`), then runs +`golangci-lint custom`. + +## Use + +Standalone: + +``` +bin/wormhole-lint ./... +``` + +Via the custom golangci-lint: + +``` +bin/wormhole-golangci-lint run --timeout=10m ./... +``` + +## Enable a plugin in `.golangci.yml` + +Module plugins are addressed under `linters.settings.custom.` and +enabled in `linters.enable` by the plugin's registered name. Example for +`channelcheck`: + +```yaml +version: "2" +linters: + enable: + - channelcheck + settings: + custom: + channelcheck: + type: module + description: reports channel blocking issues + settings: + blocking: true + unbuffered: false + bufferMax: 0 + emptyDefault: false + ignoreChannelsByName: [errC] +``` + +`channelcheck` settings: + +| Setting | Type | Default | Description | +| ---------------------- | ---------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------- | +| `blocking` | `bool` | `true` | Flag blocking sends that have no escape (timer, `default:`, or `ctx.Done()`), including sends outside any `select`. | +| `unbuffered` | `bool` | `false` | Flag unbuffered channel creation (`make(chan T)` / `make(chan T, 0)`). | +| `bufferMax` | `uint` | `0` | Flag channel buffers larger than this. `0` disables the check. | +| `emptyDefault` | `bool` | `false` | Advisory: flag an empty `default:` in a `select` with a send, since it silently drops the send. Off by default — it fires on the idiomatic non-blocking/coalescing-send pattern, so enable it only where dropping should always be logged or documented. | +| `ignoreChannelsByName` | `[]string` | `[]` | Channel/field names whose direct sends are exempt from the blocking-send, empty-default, and `ctx.Done()` checks. | + +## Development + +### Adding a new linter + +1. Scaffold the module: + ``` + mkdir -p rules/ && cd rules/ + go mod init github.com/wormhole-foundation/wormhole/linters/rules/ + ``` +2. Implement `.go` following the channelcheck reference + (`rules/channelcheck/channelcheck.go`): + - Export an `Analyzer` of type `*analysis.Analyzer`. + - Define a `Settings` struct and a `New(any) (register.LinterPlugin, error)` + constructor that decodes settings via + `register.DecodeSettings[Settings]`. + - Implement `BuildAnalyzers()` and `GetLoadMode()` on your plugin type. + - In `init()`, call `register.Plugin("", New)` so + `golangci-lint custom` picks it up. +3. Add tests + fixtures under `rules//testdata/` following + `rules/channelcheck/channelcheck_test.go`. +4. Wire it into the root module so the aggregator can import it: + - In root `go.mod`, add + `require github.com/wormhole-foundation/wormhole/linters/rules/ v0.0.0` and + `replace github.com/wormhole-foundation/wormhole/linters/rules/ => ./rules/`. + - In `cmd/wormhole-lint/main.go`, add the import and append + `.Analyzer` to the `multichecker.Main` call. +5. Wire it into `.custom-gcl.yml` so the custom golangci-lint picks it up: + ```yaml + plugins: + - module: github.com/wormhole-foundation/wormhole/linters/rules/ + path: ./rules/ + ``` +6. `make test && make build && make build-golangci-lint` to verify. +7. Add linter to `.golangci.yml`. +8. Fix linter errors in the monorepo with legitimate changes or a `nolint` comment. + +### Layout + +``` +.custom-gcl.yml # plugin manifest for `golangci-lint custom` +Makefile +go.mod # root module: cmd/* + replaces for each rules/ +cmd/ + wormhole-lint/ # multichecker aggregator binary +rules/ + channelcheck/ # standalone Go module per linter + channelcheck.go + channelcheck_test.go + go.mod + testdata/ +``` diff --git a/linters/cmd/wormhole-lint/main.go b/linters/cmd/wormhole-lint/main.go new file mode 100644 index 00000000000..81dfeba8615 --- /dev/null +++ b/linters/cmd/wormhole-lint/main.go @@ -0,0 +1,16 @@ +// Command wormhole-lint runs every custom linter under rules/ as a single +// standalone binary. Each rules// exports an Analyzer; add new +// linters by appending one import + one Analyzer below. +package main + +import ( + "golang.org/x/tools/go/analysis/multichecker" + + "github.com/wormhole-foundation/wormhole/linters/rules/channelcheck" +) + +func main() { + multichecker.Main( + channelcheck.Analyzer, + ) +} diff --git a/linters/go.mod b/linters/go.mod new file mode 100644 index 00000000000..bdff541867b --- /dev/null +++ b/linters/go.mod @@ -0,0 +1,16 @@ +module github.com/wormhole-foundation/wormhole/linters + +go 1.25.10 + +require ( + github.com/wormhole-foundation/wormhole/linters/rules/channelcheck v0.0.0 + golang.org/x/tools v0.30.0 +) + +require ( + github.com/golangci/plugin-module-register v0.1.1 // indirect + golang.org/x/mod v0.23.0 // indirect + golang.org/x/sync v0.11.0 // indirect +) + +replace github.com/wormhole-foundation/wormhole/linters/rules/channelcheck => ./rules/channelcheck diff --git a/linters/go.sum b/linters/go.sum new file mode 100644 index 00000000000..5ac45bec971 --- /dev/null +++ b/linters/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= +github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM= +golang.org/x/mod v0.23.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.30.0 h1:BgcpHewrV5AUp2G9MebG4XPFI1E2W41zU1SaqVA9vJY= +golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= diff --git a/linters/rules/channelcheck/README.md b/linters/rules/channelcheck/README.md new file mode 100644 index 00000000000..f481f692eb2 --- /dev/null +++ b/linters/rules/channelcheck/README.md @@ -0,0 +1,24 @@ +## Channel Check + +Channels are a great feature of Golang but have several footguns that can lead to deadlocks. In particular, if the receiving channel stops processing the messages, a *non-blocking* channel send would fail to continue. In certain mission-critical sections of code, this could lead to a complete deadlock. + +This linter currently has three features: +- Identify blocking sends +- Identify non-buffered channel creation +- Identify buffered channel size exceeds maximum size checks + +Many of these will lead to false positives or situations where we *want* a blocking channel send. In these cases, `nolint:channelcheck` is easy to add (assuming this is integrated directly with golangci-lint). Regardless, having this issue pointed out automatically is a good way to fix bugs. + +## Configuration + +Each option can be set two ways: as a golangci-lint module setting (under +`settings.custom.channelcheck.settings:`, using the **Setting** name) or as a +standalone analyzer flag (when running the `wormhole-lint` binary, using the +**Flag** name). + +| Setting | Flag | Type | Default | Description | +| ------------------------- | ------------ | ---------- | ------- | ----------------------------------------------------------------------------------------------- | +| `CheckBlockingSends` | `blocking` | bool | `true` | Flag blocking sends that lack a `default`/timeout/ticker escape in their enclosing `select`. | +| `CheckUnbufferedChannels` | `unbuffered` | bool | `false` | Flag creation of unbuffered channels (`make(chan T)`). | +| `CheckBufferAmount` | `bufferMax` | uint64 | `0` | Flag buffered channels whose size exceeds this max. `0` disables the check. | +| `IgnoreChannelsByName` | *(none)* | []string | `[]` | Channel/field names whose direct sends are exempt from the blocking-send check (e.g. `errC`). Settings-only; no standalone flag. | \ No newline at end of file diff --git a/linters/rules/channelcheck/channelcheck.go b/linters/rules/channelcheck/channelcheck.go new file mode 100644 index 00000000000..b10209ff38d --- /dev/null +++ b/linters/rules/channelcheck/channelcheck.go @@ -0,0 +1,488 @@ +package channelcheck + +/* + Strategy: Identify proper SendStmt's early. If we don't see it within a Select, then it's not being used correctly. + Or, we simply MISS it, making it a false positive which is fine. We'd rather fail open and get the user to + use nolints than miss a potential bug altogether. +*/ +import ( + "bytes" + "flag" + "fmt" + "go/ast" + "go/constant" + "go/printer" + "go/token" + "go/types" + + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +// ChannelCheckPlugin is the golangci-lint module-plugin entry point. Its +// configuration lives in the package-level settings var (populated by New), +// which is the single source of truth — shared with the standalone +// cmd/wormhole-lint multichecker, which populates the same var via flags. +type ChannelCheckPlugin struct{} + +// Settings holds the configuration for the channelcheck linter. +type Settings struct { + CheckUnbufferedChannels bool // Enable/disable checking for unbuffered channel creation. + CheckBufferAmount uint64 // The amount that can be in a buffer. 0 means don't do this check. + CheckBlockingSends bool // Enable/disable checking for blocking sends without default/timeout. + CheckEmptyDefault bool // Enable/disable the (advisory) empty-default-drops-the-send check. + IgnoreChannelsByName []string // Channel/field names whose direct sends are exempt from the blocking-send check. + + // ignoreChannelNames is the lookup form of IgnoreChannelsByName, built + // from the slice during configuration. + ignoreChannelNames map[string]bool +} + +// EscapeKind classifies a non-send CommClause in a select, describing what +// kind of escape valve (if any) it provides for a sibling send. +type EscapeKind int + +const ( + EscapeNone EscapeKind = iota + EscapeTimer // receive on <-chan time.Time + EscapeDefaultWithCode // default: with non-empty body + EscapeEmptyDefault // default: with empty body + EscapeContextDone // receive on a call to (context.Context).Done() + EscapeOther // any other clause in the statement +) + +// SelectAnalysis is the result of inspecting a select statement. +type SelectAnalysis struct { + Sends []*ast.SendStmt // SendStmts that are direct CommClause heads + Escapes []EscapeKind // one entry per non-send clause (excludes EscapeNone) + EmptyDefaultPos token.Pos // position of an empty default clause, or NoPos if none + ContextDonePos token.Pos // position of a <-ctx.Done() clause, or NoPos if none +} + +var Analyzer = &analysis.Analyzer{ + Name: "channelcheck", + Doc: "reports channel blocking issues", + Run: run, + Flags: flagSet, +} + +// Flags for the analyzer +var flagSet flag.FlagSet + +// Global structure to store the variables in +var settings Settings + +func New(settingsNew any) (register.LinterPlugin, error) { + s, err := register.DecodeSettings[Settings](settingsNew) + if err != nil { + return nil, err + } + // Assign the whole struct so a future Settings field can't be silently + // dropped, then derive the lookup map from the decoded slice. + settings = s + settings.ignoreChannelNames = buildIgnoreSet(s.IgnoreChannelsByName) + + return &ChannelCheckPlugin{}, nil +} + +func buildIgnoreSet(names []string) map[string]bool { + if len(names) == 0 { + return nil + } + out := make(map[string]bool, len(names)) + for _, n := range names { + out[n] = true + } + return out +} + +func (f *ChannelCheckPlugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + // Reuse the package-level Analyzer (the same one the standalone + // cmd/wormhole-lint multichecker runs) instead of defining a second one. + return []*analysis.Analyzer{Analyzer}, nil +} + +// Initialize the flags from the golangci-lint +func init() { + + flagSet.BoolVar(&settings.CheckUnbufferedChannels, "unbuffered", false, "Check for unbuffered channel creation") + flagSet.BoolVar(&settings.CheckBlockingSends, "blocking", true, "Check for blocking sends without default/timeout") + flagSet.BoolVar(&settings.CheckEmptyDefault, "emptyDefault", false, "Advise when an empty default case silently drops a send") + flagSet.Uint64Var(&settings.CheckBufferAmount, "bufferMax", 0, "Check for maximum length of channel buffer being exceeded") + Analyzer.Flags = flagSet + register.Plugin("channelcheck", New) +} + +func (f *ChannelCheckPlugin) GetLoadMode() string { + return register.LoadModeTypesInfo +} + +func run(pass *analysis.Pass) (interface{}, error) { + + for _, file := range pass.Files { + var seenPositions = make(map[token.Pos]bool) + + ast.Inspect(file, func(node ast.Node) bool { + switch n := node.(type) { + // Fails open by design. Will + case *ast.SelectStmt: // Select statement for channel matching + + if !settings.CheckBlockingSends { + break + } + selectAnalysis := processSelect(pass, n) + + /* + A send is considered safe (i.e., the SendStmt-level diagnostic is + suppressed) if its enclosing select has at least one of: + - Timer, DefaultWithCode, or EmptyDefault — real backpressure relief + - ctx.Done() — shutdown-safe; we suppress the bare blocking-send + diagnostic and emit a more specific ctx.Done() finding instead + (only when ctx.Done() is the lone escape — if a Timer/Default is + also present, that's already doing the real work). + + EscapeOther alone does NOT make the send safe. + */ + sendsAreSafe := false + foundContext := false + for _, escapeKind := range selectAnalysis.Escapes { + switch escapeKind { + case EscapeTimer, EscapeDefaultWithCode, EscapeEmptyDefault: + sendsAreSafe = true + case EscapeContextDone: + foundContext = true + case EscapeNone, EscapeOther: + // Neither marks the send safe: EscapeNone shouldn't appear + // here (Escapes excludes it) and EscapeOther alone provides + // no backpressure relief. + } + } + + if sendsAreSafe || foundContext { + for _, send := range selectAnalysis.Sends { + seenPositions[send.Pos()] = true + } + } + + // Collect the sends whose channels are NOT in the user's ignore list. + // These are the anchor points for the empty-default and ctx.Done() + // diagnostics so that a //nolint:channelcheck on the consciously- + // blocking send line suppresses them. + var trackedSends []*ast.SendStmt + for _, send := range selectAnalysis.Sends { + if name, named := sendChanName(send); named && settings.ignoreChannelNames[name] { + continue + } + trackedSends = append(trackedSends, send) + } + + // Empty default case. Only meaningful when there's a tracked send + // in the select (a receive-only select with an empty default has no + // backpressure concern). Anchor at each tracked send so users can + // suppress with a //nolint next to the blocking send. + if settings.CheckEmptyDefault && selectAnalysis.EmptyDefaultPos != token.NoPos { + for _, send := range trackedSends { + pass.Reportf(send.Pos(), + "empty default in channel select silently drops the send; log it or document why dropping is intended") + } + } + + // ctx.Done() was the only thing found alongside the send. Flag it. + // Only fire when there's a send in the select — a receive-only select + // with ctx.Done() is the canonical "wait or shutdown" idiom and has + // no backpressure concern. Anchor the diagnostic at each tracked send so that + // a //nolint:channelcheck next to the consciously-blocking send works. + if !sendsAreSafe && foundContext && selectAnalysis.ContextDonePos != token.NoPos { + for _, send := range trackedSends { + pass.Reportf(send.Pos(), + "ctx.Done() in channel select not backfill safe. Consider adding a timer, or default statement.") + } + } + + // Most of the work is done in the previous case statement. + case *ast.SendStmt: + + if !settings.CheckBlockingSends { + break + } + + // If the SendStmt was NOT found within a Select clause, then add a linter error. + tokenID := n.Pos() + if _, ok := seenPositions[tokenID]; !ok { + if name, named := sendChanName(n); named && settings.ignoreChannelNames[name] { + return true + } + pass.Reportf(tokenID, "Blocking send. Add timer, ticker, or default case: %q", render(pass.Fset, n)) + } + return true + case *ast.CallExpr: + // Channel creation that's unbuffered + didCreateChannelWithoutBuffering, bufferAmount := checkChannelCreation(pass, n) + if didCreateChannelWithoutBuffering && settings.CheckUnbufferedChannels { + pass.Reportf(n.Pos(), "unbuffered channel creation detected - consider specifying buffer size %q", render(pass.Fset, n)) + } + + if settings.CheckBufferAmount > 0 && bufferAmount > 0 && bufferAmount > settings.CheckBufferAmount { + pass.Reportf(n.Pos(), "channel buffer size exceeds the specified limit %q", render(pass.Fset, n)) + } + return true + + default: + return true // Continue traversing for other node types + } + + return true + }) + } + + return nil, nil +} + +/* +Walks a select statement and classifies each CommClause. + +Sends that are the direct Comm of a CommClause are recorded in SendPositions. +Every other clause is classified via classifyClause and appended to Escapes. +An empty default body is additionally recorded by position so the caller can +emit a separate finding for it. + +NOTE: A send is always a candidate for a finding unless its enclosing select has a +recognized escape (Timer, DefaultWithCode, or EmptyDefault). Fails open by design. +*/ +func processSelect(pass *analysis.Pass, selectStmt *ast.SelectStmt) SelectAnalysis { + var analysis SelectAnalysis + for _, clause := range selectStmt.Body.List { + commClause, ok := clause.(*ast.CommClause) + if !ok { + continue // Skip if not a CommClause (e.g., a declaration inside the select) + } + if sendNode, isSend := commClause.Comm.(*ast.SendStmt); isSend { + analysis.Sends = append(analysis.Sends, sendNode) + continue + } + clauseKind := classifyClause(pass, commClause) + if clauseKind == EscapeNone { + continue + } + analysis.Escapes = append(analysis.Escapes, clauseKind) + if clauseKind == EscapeEmptyDefault { + analysis.EmptyDefaultPos = commClause.Pos() + } + if clauseKind == EscapeContextDone { + analysis.ContextDonePos = commClause.Pos() + } + } + return analysis +} + +// classifyClause returns the EscapeKind for a non-send CommClause. +// Returns EscapeNone for SendStmt clauses (they aren't escapes themselves). +func classifyClause(pass *analysis.Pass, commClause *ast.CommClause) EscapeKind { + if commClause.Comm == nil { + if len(commClause.Body) == 0 { + return EscapeEmptyDefault + } + return EscapeDefaultWithCode + } + if _, isSend := commClause.Comm.(*ast.SendStmt); isSend { + return EscapeNone + } + channelExpr := extractRecvChannel(commClause.Comm) + if channelExpr == nil { + return EscapeOther + } + if isContextDoneCall(pass, channelExpr) { + return EscapeContextDone + } + elementType := recvElementType(pass, channelExpr) + if elementType != nil && isNamedType(elementType, "time", "Time") { + return EscapeTimer + } + return EscapeOther +} + +// isContextDoneCall reports whether expr is a call to (context.Context).Done(). +func isContextDoneCall(pass *analysis.Pass, expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + if selector.Sel.Name != "Done" { + return false + } + if pass.TypesInfo == nil { + return false + } + methodObj, ok := pass.TypesInfo.Uses[selector.Sel].(*types.Func) + if !ok { + return false + } + methodPkg := methodObj.Pkg() + return methodPkg != nil && methodPkg.Path() == "context" +} + +// extractRecvChannel returns the channel expression of a receive embedded in +// any of the three CommClause shapes: +// +// case <-ch: (*ast.ExprStmt wrapping *ast.UnaryExpr) +// case v := <-ch: (*ast.AssignStmt, one RHS) +// case v, ok := <-ch: (*ast.AssignStmt, one RHS, two LHS) +// +// Returns nil if the statement is not a receive. +func extractRecvChannel(commStmt ast.Stmt) ast.Expr { + var receiveExpr ast.Expr + switch typedStmt := commStmt.(type) { + case *ast.ExprStmt: + receiveExpr = typedStmt.X + case *ast.AssignStmt: + if len(typedStmt.Rhs) != 1 { + return nil + } + receiveExpr = typedStmt.Rhs[0] + default: + return nil + } + unaryExpr, ok := receiveExpr.(*ast.UnaryExpr) + if !ok || unaryExpr.Op != token.ARROW { + return nil + } + return unaryExpr.X +} + +// recvElementType returns the element type of the channel being received from, +// or nil if type info is unavailable or the expression is not a channel. +func recvElementType(pass *analysis.Pass, channelExpr ast.Expr) types.Type { + if pass.TypesInfo == nil { + return nil + } + channelExprType := pass.TypesInfo.TypeOf(channelExpr) + if channelExprType == nil { + return nil + } + channelType, ok := channelExprType.Underlying().(*types.Chan) + if !ok { + return nil + } + return channelType.Elem() +} + +// isNamedType reports whether t is the named type pkgPath.name (e.g. "time", "Time"). +func isNamedType(candidateType types.Type, pkgPath, typeName string) bool { + namedType, ok := candidateType.(*types.Named) + if !ok { + return false + } + typeObj := namedType.Obj() + if typeObj == nil || typeObj.Pkg() == nil { + return false + } + return typeObj.Pkg().Path() == pkgPath && typeObj.Name() == typeName +} + +// makeChanBufferedArgs is the argument count of a buffered channel make: +// make(chan T, size) — the channel type plus the buffer-size expression. +const makeChanBufferedArgs = 2 + +func checkChannelCreation(pass *analysis.Pass, node *ast.CallExpr) (bool, uint64) { + fun, ok := node.Fun.(*ast.Ident) + if !ok || fun == nil || fun.Name != "make" { + return false, 0 + } + + if len(node.Args) > 0 { + if _, ok := node.Args[0].(*ast.ChanType); ok { // It's a channel + if len(node.Args) == 1 { + return true, 0 // Unbuffered channel + } + + if len(node.Args) == makeChanBufferedArgs { + // Evaluate the buffer size expression + bufferSize, err := evalBufferSize(pass, node.Args[1]) + if err != nil { + // Has a buffer arg but the size is not statically determinable + // (e.g. a runtime variable). Don't flag it as unbuffered and + // don't check against the max — the size is just unknown. + return false, 0 + } + + // make(chan T, 0) is semantically identical to make(chan T) — + // both produce an unbuffered channel with synchronous rendezvous. + if bufferSize == 0 { + return true, 0 + } + return false, bufferSize + } + } + } + + return false, 0 +} + +/* +Evaluates the buffer size expression in a make(chan T, N) call. Returns the +constant value of N if it is statically determinable — which covers integer +literals (`make(chan T, 100)`), named constants (`const N = 100; make(chan T, N)`), +and constant arithmetic (`make(chan T, 2*N+1)`). + +Returns an error for runtime values (`var n = 100; make(chan T, n)`) which +cannot be evaluated at lint time. +*/ +func evalBufferSize(pass *analysis.Pass, expr ast.Expr) (uint64, error) { + if pass.TypesInfo == nil { + return 0, fmt.Errorf("type info unavailable") + } + typeAndValue, ok := pass.TypesInfo.Types[expr] + if !ok || typeAndValue.Value == nil { + return 0, fmt.Errorf("buffer size is not a constant expression") + } + intValue := constant.ToInt(typeAndValue.Value) + if intValue.Kind() != constant.Int { + return 0, fmt.Errorf("buffer size is not an integer: %v", typeAndValue.Value.Kind()) + } + bufferSize, exact := constant.Uint64Val(intValue) + if !exact { + return 0, fmt.Errorf("buffer size is too large or negative") + } + return bufferSize, nil +} + +// render returns the pretty-print of the given node +func render(fset *token.FileSet, x interface{}) string { + var buf bytes.Buffer + if err := printer.Fprint(&buf, fset, x); err != nil { + panic(err) + } + return buf.String() +} + +// sendChanName returns the receiving channel's variable name for a send +// statement (the channel on the left of `<-`). The second return value is +// false when the Chan expression has no meaningful single name — e.g. +// `channels[0] <- x`, `chFunc() <- x`, or a type assertion `iface.(chan T) <- x`. +// For those, callers should fall back to render(fset, sendStmt.Chan). +// +// Supported shapes: +// +// ch <- x → "ch" (*ast.Ident) +// s.eventCh <- x → "eventCh" (*ast.SelectorExpr; returns the field name) +// (ch) <- x → "ch" (*ast.ParenExpr; unwrapped recursively) +func sendChanName(sendStmt *ast.SendStmt) (string, bool) { + chanExpr := sendStmt.Chan + for { + switch typedExpr := chanExpr.(type) { + case *ast.Ident: + return typedExpr.Name, true + case *ast.SelectorExpr: + return typedExpr.Sel.Name, true + case *ast.ParenExpr: + chanExpr = typedExpr.X + continue + default: + return "", false + } + } +} diff --git a/linters/rules/channelcheck/channelcheck_test.go b/linters/rules/channelcheck/channelcheck_test.go new file mode 100644 index 00000000000..50c57a4ee6b --- /dev/null +++ b/linters/rules/channelcheck/channelcheck_test.go @@ -0,0 +1,137 @@ +package channelcheck + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +// runFixture runs the Analyzer against testdata/src/ under a transient +// settings override. It restores the previous settings on return so tests do +// not interfere with each other. +func runFixture(t *testing.T, pkg string, override Settings) { + t.Helper() + + saved := settings + defer func() { settings = saved }() + + // Default matches the flag defaults (blocking on, everything else off). + settings = Settings{CheckBlockingSends: true} + + settings.CheckUnbufferedChannels = override.CheckUnbufferedChannels + settings.CheckBlockingSends = override.CheckBlockingSends + settings.CheckEmptyDefault = override.CheckEmptyDefault + settings.CheckBufferAmount = override.CheckBufferAmount + settings.IgnoreChannelsByName = override.IgnoreChannelsByName + settings.ignoreChannelNames = buildIgnoreSet(override.IgnoreChannelsByName) + + analysistest.Run(t, analysistest.TestData(), Analyzer, pkg) +} + +func defaultSettings() Settings { + return Settings{CheckBlockingSends: true} +} + +func TestBlockingSend(t *testing.T) { + runFixture(t, "blocking_send", defaultSettings()) +} + +func TestEmptyDefault(t *testing.T) { + runFixture(t, "empty_default", Settings{CheckBlockingSends: true, CheckEmptyDefault: true}) +} + +func TestCtxDoneOnly(t *testing.T) { + runFixture(t, "ctx_done_only", defaultSettings()) +} + +func TestTimerSafe(t *testing.T) { + runFixture(t, "timer_safe", defaultSettings()) +} + +func TestDefaultSafe(t *testing.T) { + runFixture(t, "default_safe", defaultSettings()) +} + +func TestUnbufferedChannel(t *testing.T) { + runFixture(t, "unbuffered_chan", Settings{ + CheckUnbufferedChannels: true, + CheckBlockingSends: false, + }) +} + +func TestBufferTooLarge(t *testing.T) { + runFixture(t, "buffer_too_large", Settings{ + CheckBlockingSends: false, + CheckBufferAmount: 10, + }) +} + +func TestBlockingDisabled(t *testing.T) { + runFixture(t, "blocking_disabled", Settings{CheckBlockingSends: false}) +} + +func TestBlockingDisabledSuppressesEmptyDefault(t *testing.T) { + runFixture(t, "blocking_disabled_empty_default", Settings{CheckBlockingSends: false}) +} + +func TestBlockingDisabledSuppressesCtxDone(t *testing.T) { + runFixture(t, "blocking_disabled_ctx_done", Settings{CheckBlockingSends: false}) +} + +func TestUnbufferedDisabledByDefault(t *testing.T) { + // Default config has CheckUnbufferedChannels=false. unbuffered_chan.go's + // want marker only fires when the check is on; with the check off we need + // a fixture without a // want marker. + runFixture(t, "unbuffered_disabled", defaultSettings()) +} + +func TestEscapeOtherAlone(t *testing.T) { + runFixture(t, "escape_other_alone", defaultSettings()) +} + +func TestDerivedCtxDone(t *testing.T) { + runFixture(t, "derived_ctx_done", defaultSettings()) +} + +func TestFakeDoneCounter(t *testing.T) { + runFixture(t, "fake_done_counter", defaultSettings()) +} + +func TestSendInCaseBody(t *testing.T) { + runFixture(t, "send_in_case_body", defaultSettings()) +} + +func TestIgnoreChannelsByName(t *testing.T) { + runFixture(t, "ignore_by_name", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestIgnoreSuppressesEmptyDefault(t *testing.T) { + runFixture(t, "ignore_empty_default", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestIgnoreSuppressesCtxDone(t *testing.T) { + runFixture(t, "ignore_ctx_done", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestIgnorePartialStillFires(t *testing.T) { + runFixture(t, "ignore_partial", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestBufferMaxDisabledByDefault(t *testing.T) { + // Default has CheckBufferAmount=0 which disables the check. The + // buffer_too_large fixture's // want marker requires the check on, so a + // no-want fixture is used here. + runFixture(t, "buffer_disabled", Settings{CheckBlockingSends: false}) +} diff --git a/linters/rules/channelcheck/go.mod b/linters/rules/channelcheck/go.mod new file mode 100644 index 00000000000..4e5308adb7c --- /dev/null +++ b/linters/rules/channelcheck/go.mod @@ -0,0 +1,11 @@ +module github.com/wormhole-foundation/wormhole/linters/rules/channelcheck + +go 1.25.10 + +require golang.org/x/tools v0.30.0 + +require ( + github.com/golangci/plugin-module-register v0.1.1 // indirect + golang.org/x/mod v0.23.0 // indirect + golang.org/x/sync v0.11.0 // indirect +) diff --git a/linters/rules/channelcheck/go.sum b/linters/rules/channelcheck/go.sum new file mode 100644 index 00000000000..5ac45bec971 --- /dev/null +++ b/linters/rules/channelcheck/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= +github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM= +golang.org/x/mod v0.23.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.30.0 h1:BgcpHewrV5AUp2G9MebG4XPFI1E2W41zU1SaqVA9vJY= +golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= diff --git a/linters/rules/channelcheck/testdata/src/blocking_disabled/blocking_disabled.go b/linters/rules/channelcheck/testdata/src/blocking_disabled/blocking_disabled.go new file mode 100644 index 00000000000..8cd4b8c890b --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_disabled/blocking_disabled.go @@ -0,0 +1,6 @@ +package fixture + +func blockingSend() { + c := make(chan int, 1) + c <- 1 +} diff --git a/linters/rules/channelcheck/testdata/src/blocking_disabled_ctx_done/blocking_disabled_ctx_done.go b/linters/rules/channelcheck/testdata/src/blocking_disabled_ctx_done/blocking_disabled_ctx_done.go new file mode 100644 index 00000000000..c470e24a128 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_disabled_ctx_done/blocking_disabled_ctx_done.go @@ -0,0 +1,11 @@ +package fixture + +import "context" + +func ctxDoneOnly(ctx context.Context) { + c := make(chan int, 1) + select { + case c <- 1: + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/blocking_disabled_empty_default/blocking_disabled_empty_default.go b/linters/rules/channelcheck/testdata/src/blocking_disabled_empty_default/blocking_disabled_empty_default.go new file mode 100644 index 00000000000..24ee0a91fa1 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_disabled_empty_default/blocking_disabled_empty_default.go @@ -0,0 +1,9 @@ +package fixture + +func emptyDefault() { + c := make(chan int, 1) + select { + case c <- 1: + default: + } +} diff --git a/linters/rules/channelcheck/testdata/src/blocking_send/blocking_send.go b/linters/rules/channelcheck/testdata/src/blocking_send/blocking_send.go new file mode 100644 index 00000000000..c2299b749dd --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_send/blocking_send.go @@ -0,0 +1,7 @@ +package fixture + +// Single match +func blockingSend() { + c := make(chan int, 1) + c <- 1 // want `Blocking send` +} diff --git a/linters/rules/channelcheck/testdata/src/buffer_disabled/buffer_disabled.go b/linters/rules/channelcheck/testdata/src/buffer_disabled/buffer_disabled.go new file mode 100644 index 00000000000..4be2f5bb2ee --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/buffer_disabled/buffer_disabled.go @@ -0,0 +1,5 @@ +package fixture + +func tooLarge() { + _ = make(chan int, 100) +} diff --git a/linters/rules/channelcheck/testdata/src/buffer_too_large/buffer_too_large.go b/linters/rules/channelcheck/testdata/src/buffer_too_large/buffer_too_large.go new file mode 100644 index 00000000000..52393e7c421 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/buffer_too_large/buffer_too_large.go @@ -0,0 +1,6 @@ +package fixture + +// Single match for buffer too large +func tooLarge() { + _ = make(chan int, 100) // want `buffer size exceeds` +} diff --git a/linters/rules/channelcheck/testdata/src/ctx_done_only/ctx_done_only.go b/linters/rules/channelcheck/testdata/src/ctx_done_only/ctx_done_only.go new file mode 100644 index 00000000000..3df21df308f --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ctx_done_only/ctx_done_only.go @@ -0,0 +1,12 @@ +package fixture + +import "context" + +// Single match for having a 'Done()' without anything else that was useful. +func ctxDoneOnly(ctx context.Context) { + c := make(chan int, 1) + select { + case c <- 1: // want `ctx\.Done\(\)` + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/default_safe/default_safe.go b/linters/rules/channelcheck/testdata/src/default_safe/default_safe.go new file mode 100644 index 00000000000..fbc70229e03 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/default_safe/default_safe.go @@ -0,0 +1,14 @@ +// OK +package fixture + +import "fmt" + +// Channel send with default and a println. This is "safe" according to the code. +func defaultSafe() { + c := make(chan int, 1) + select { + case c <- 1: + default: + fmt.Println("dropped") + } +} diff --git a/linters/rules/channelcheck/testdata/src/derived_ctx_done/derived_ctx_done.go b/linters/rules/channelcheck/testdata/src/derived_ctx_done/derived_ctx_done.go new file mode 100644 index 00000000000..d8e40340f68 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/derived_ctx_done/derived_ctx_done.go @@ -0,0 +1,20 @@ +package fixture + +import ( + "context" + "time" +) + +// A derived context (timeoutCtx) — its Done() method still resolves to the +// "context" package, so the rule must still recognize it as EscapeContextDone +// and emit the ctx.Done() diagnostic. +func derivedCtxDone(ctx context.Context) { + timeoutCtx, cancel := context.WithTimeout(ctx, time.Second) + defer cancel() + + c := make(chan int, 1) + select { + case c <- 1: // want `ctx\.Done\(\)` + case <-timeoutCtx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/empty_default/empty_default.go b/linters/rules/channelcheck/testdata/src/empty_default/empty_default.go new file mode 100644 index 00000000000..926dd4774d9 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/empty_default/empty_default.go @@ -0,0 +1,10 @@ +package fixture + +// Match - empty default case +func emptyDefault() { + c := make(chan int, 1) + select { + case c <- 1: // want `empty default in channel select silently drops the send` + default: + } +} diff --git a/linters/rules/channelcheck/testdata/src/escape_other_alone/escape_other_alone.go b/linters/rules/channelcheck/testdata/src/escape_other_alone/escape_other_alone.go new file mode 100644 index 00000000000..e74ef7254f8 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/escape_other_alone/escape_other_alone.go @@ -0,0 +1,11 @@ +package fixture + +// Match for no default or timeout +func escapeOtherAlone() { + c := make(chan int, 1) + other := make(chan struct{}, 1) + select { + case c <- 1: // want `Blocking send` + case <-other: + } +} diff --git a/linters/rules/channelcheck/testdata/src/fake_done_counter/fake_done_counter.go b/linters/rules/channelcheck/testdata/src/fake_done_counter/fake_done_counter.go new file mode 100644 index 00000000000..38b27891106 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/fake_done_counter/fake_done_counter.go @@ -0,0 +1,19 @@ +package fixture + +// Non-match. Only the ctx.Done() function should trigger this. +type fakeDone struct { + done chan struct{} +} + +func (f *fakeDone) Done() <-chan struct{} { + return f.done +} + +func fakeDoneCounter() { + c := make(chan int, 1) + f := &fakeDone{done: make(chan struct{}, 1)} + select { + case c <- 1: // want `Blocking send` + case <-f.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_by_name/ignore_by_name.go b/linters/rules/channelcheck/testdata/src/ignore_by_name/ignore_by_name.go new file mode 100644 index 00000000000..8ad8d986194 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_by_name/ignore_by_name.go @@ -0,0 +1,17 @@ +package fixture + +type holder struct { + ignoreMe chan int +} + +// ignoreMe and h.ignoreMe are not matched based on ignore channel rules. +// tracked is still matched. +func ignoreByName() { + ignoreMe := make(chan int, 1) + tracked := make(chan int, 1) + h := &holder{ignoreMe: make(chan int, 1)} + + ignoreMe <- 1 // ignored by name + h.ignoreMe <- 2 // ignored by selector field name + tracked <- 3 // want `Blocking send` +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_ctx_done/ignore_ctx_done.go b/linters/rules/channelcheck/testdata/src/ignore_ctx_done/ignore_ctx_done.go new file mode 100644 index 00000000000..3ae29d47c2b --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_ctx_done/ignore_ctx_done.go @@ -0,0 +1,11 @@ +package fixture + +import "context" + +func ignoreCtxDone(ctx context.Context) { + ignoreMe := make(chan int, 1) + select { + case ignoreMe <- 1: + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_empty_default/ignore_empty_default.go b/linters/rules/channelcheck/testdata/src/ignore_empty_default/ignore_empty_default.go new file mode 100644 index 00000000000..b612543254a --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_empty_default/ignore_empty_default.go @@ -0,0 +1,9 @@ +package fixture + +func ignoreEmptyDefault() { + ignoreMe := make(chan int, 1) + select { + case ignoreMe <- 1: + default: + } +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_partial/ignore_partial.go b/linters/rules/channelcheck/testdata/src/ignore_partial/ignore_partial.go new file mode 100644 index 00000000000..49864694f06 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_partial/ignore_partial.go @@ -0,0 +1,15 @@ +package fixture + +import "context" + +// Mixed sends in one select: one ignored, one tracked. The select must still +// produce the ctx.Done() diagnostic because not every send is ignored. +func ignorePartial(ctx context.Context) { + ignoreMe := make(chan int, 1) + tracked := make(chan int, 1) + select { + case ignoreMe <- 1: + case tracked <- 2: // want `ctx\.Done\(\)` + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/send_in_case_body/send_in_case_body.go b/linters/rules/channelcheck/testdata/src/send_in_case_body/send_in_case_body.go new file mode 100644 index 00000000000..93743da0d53 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/send_in_case_body/send_in_case_body.go @@ -0,0 +1,18 @@ +package fixture + +import "time" + +// A send inside a case BODY is unrelated to the select's escape mechanism — +// it executes synchronously when that branch is chosen and blocks just like +// any other bare send. Even with a real timer escape on the select, the rule +// must still flag the in-body sends. +func sendInCaseBody() { + c := make(chan int, 1) + other := make(chan int, 1) + select { + case <-time.After(time.Second): + c <- 1 // want `Blocking send` + case <-other: + c <- 2 // want `Blocking send` + } +} diff --git a/linters/rules/channelcheck/testdata/src/timer_safe/timer_safe.go b/linters/rules/channelcheck/testdata/src/timer_safe/timer_safe.go new file mode 100644 index 00000000000..fa04cd687bb --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/timer_safe/timer_safe.go @@ -0,0 +1,12 @@ +// OK +package fixture + +import "time" + +func timerSafe() { + c := make(chan int, 1) + select { + case c <- 1: + case <-time.After(time.Second): + } +} diff --git a/linters/rules/channelcheck/testdata/src/unbuffered_chan/unbuffered_chan.go b/linters/rules/channelcheck/testdata/src/unbuffered_chan/unbuffered_chan.go new file mode 100644 index 00000000000..dea6fd1066d --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/unbuffered_chan/unbuffered_chan.go @@ -0,0 +1,5 @@ +package fixture + +func unbuffered() { + _ = make(chan int) // want `unbuffered channel` +} diff --git a/linters/rules/channelcheck/testdata/src/unbuffered_disabled/unbuffered_disabled.go b/linters/rules/channelcheck/testdata/src/unbuffered_disabled/unbuffered_disabled.go new file mode 100644 index 00000000000..5075016ec15 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/unbuffered_disabled/unbuffered_disabled.go @@ -0,0 +1,5 @@ +package fixture + +func unbuffered() { + _ = make(chan int) +} diff --git a/linters/rules/example_linter/example.go b/linters/rules/example_linter/example.go new file mode 100644 index 00000000000..08388a6b5c0 --- /dev/null +++ b/linters/rules/example_linter/example.go @@ -0,0 +1,66 @@ +// Package example is a no-op reference linter. It demonstrates the minimum +// structure needed to plug a new analyzer into both the wormhole-lint +// aggregator (cmd/wormhole-lint) and the custom golangci-lint binary +// (.custom-gcl.yml). Copy this package as a starting point when adding a +// real linter under rules//. +package example + +import ( + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +// Settings is the per-invocation configuration block passed in via +// golangci-lint's `linters.settings.custom.example.settings:` map. Fields +// here become user-facing tunables; an empty struct is fine for a no-op. +type Settings struct{} + +// Analyzer is the standalone entrypoint consumed by cmd/wormhole-lint. +// Keep Name unique across all rules// analyzers — golangci-lint +// addresses the plugin by this name in .golangci.yml. +var Analyzer = &analysis.Analyzer{ + Name: "example", + Doc: "no-op reference linter; never reports diagnostics", + Run: run, +} + +func run(_ *analysis.Pass) (any, error) { + // Intentionally empty: this linter never flags anything. A real linter + // walks pass.Files and calls pass.Reportf / pass.Report on offenders. + return nil, nil +} + +// Plugin is the type returned to golangci-lint's module-plugin loader. One +// plugin can expose multiple analyzers via BuildAnalyzers. +type Plugin struct { + settings Settings +} + +// New is the constructor registered with golangci-lint. It receives the +// raw settings map from .golangci.yml and decodes it into Settings. +func New(raw any) (register.LinterPlugin, error) { + s, err := register.DecodeSettings[Settings](raw) + if err != nil { + return nil, err + } + return &Plugin{settings: s}, nil +} + +// BuildAnalyzers returns the analyzers this plugin contributes. Add more +// entries here if your linter ships multiple checks. +func (p *Plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{Analyzer}, nil +} + +// GetLoadMode tells golangci-lint how much type information to load. +// LoadModeSyntax is enough for AST-only checks; LoadModeTypesInfo is +// required when you need full type resolution (recommended default). +func (p *Plugin) GetLoadMode() string { + return register.LoadModeTypesInfo +} + +func init() { + // The string passed here is the name used in .golangci.yml under + // linters.enable and linters.settings.custom. + register.Plugin("example", New) +} diff --git a/linters/rules/example_linter/example_test.go b/linters/rules/example_linter/example_test.go new file mode 100644 index 00000000000..63dcbb4e700 --- /dev/null +++ b/linters/rules/example_linter/example_test.go @@ -0,0 +1,13 @@ +package example + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestExample(t *testing.T) { + // The fixture contains no `// want "..."` comments — the linter is a + // no-op, so analysistest should see zero diagnostics. + analysistest.Run(t, analysistest.TestData(), Analyzer, "./...") +} diff --git a/linters/rules/example_linter/go.mod b/linters/rules/example_linter/go.mod new file mode 100644 index 00000000000..163f7ebc698 --- /dev/null +++ b/linters/rules/example_linter/go.mod @@ -0,0 +1,13 @@ +module github.com/wormhole-foundation/wormhole/linters/rules/example_linter + +go 1.25.10 + +require ( + github.com/golangci/plugin-module-register v0.1.1 + golang.org/x/tools v0.30.0 +) + +require ( + golang.org/x/mod v0.23.0 // indirect + golang.org/x/sync v0.11.0 // indirect +) diff --git a/linters/rules/example_linter/go.sum b/linters/rules/example_linter/go.sum new file mode 100644 index 00000000000..5ac45bec971 --- /dev/null +++ b/linters/rules/example_linter/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= +github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM= +golang.org/x/mod v0.23.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.30.0 h1:BgcpHewrV5AUp2G9MebG4XPFI1E2W41zU1SaqVA9vJY= +golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= diff --git a/linters/rules/example_linter/testdata/example.go b/linters/rules/example_linter/testdata/example.go new file mode 100644 index 00000000000..02f55ef4b9d --- /dev/null +++ b/linters/rules/example_linter/testdata/example.go @@ -0,0 +1,9 @@ +package testdata + +// Fixture for the example linter. It contains no offenders because the +// example linter never flags anything; this file exists only to give +// `go test` something to load when running `analysistest`. + +func Hello() string { + return "hello" +} diff --git a/node/Makefile b/node/Makefile index 7fcf7cde8c0..e8eba1525ea 100644 --- a/node/Makefile +++ b/node/Makefile @@ -1,6 +1,7 @@ .PHONY: lint lint: - golangci-lint run -c ../.golangci.yml ./... +# Lints spelling and Go via the custom wormhole-golangci-lint + bash ../scripts/lint.sh lint .PHONY: test test: diff --git a/node/cmd/spy/spy.go b/node/cmd/spy/spy.go index 9a68e0d1232..5dc4a740e88 100644 --- a/node/cmd/spy/spy.go +++ b/node/cmd/spy/spy.go @@ -126,7 +126,7 @@ func (s *spyServer) PublishSignedVAA(vaaBytes []byte) error { return err } } - sub.ch <- message{vaaBytes: vaaBytes} // Note on channel capacity: Don't want to drop incoming VAAs + sub.ch <- message{vaaBytes: vaaBytes} //nolint:channelcheck // Don't want to drop incoming VAAs. So, block on send. continue } @@ -146,7 +146,7 @@ func (s *spyServer) PublishSignedVAA(vaaBytes []byte) error { return err } } - sub.ch <- message{vaaBytes: vaaBytes} // Note on channel capacity: Don't want to drop incoming VAAs + sub.ch <- message{vaaBytes: vaaBytes} //nolint:channelcheck // Don't want to drop incoming VAAs. So, block on send. } } @@ -252,7 +252,7 @@ func newSpyServer(logger *zap.Logger) *spyServer { func DoWithTimeout(f func() error, d time.Duration) error { errChan := make(chan error, 1) go func() { - errChan <- f() // Note on channel capacity: Has timeout below + errChan <- f() //nolint:channelcheck // Has timeout below. If f() takes too long, the rest of the code will continue. close(errChan) }() t := time.NewTimer(d) diff --git a/node/hack/evm_test/wstest.go b/node/hack/evm_test/wstest.go index 2d94760d9e9..7915504758c 100644 --- a/node/hack/evm_test/wstest.go +++ b/node/hack/evm_test/wstest.go @@ -24,6 +24,7 @@ import ( "go.uber.org/zap" + "github.com/certusone/wormhole/node/pkg/common" ethAbi "github.com/certusone/wormhole/node/pkg/watchers/evm/connectors/ethabi" ethBind "github.com/ethereum/go-ethereum/accounts/abi/bind" ethCommon "github.com/ethereum/go-ethereum/common" @@ -77,7 +78,7 @@ func main() { case <-ctx.Done(): return case err := <-headerSubscription.Err(): - errC <- fmt.Errorf("block subscription failed: %w", err) // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("block subscription failed: %w", err), "wstest_errc") return case block := <-headSink: // These two pointers should have been checked before the event was placed on the channel, but just being safe. @@ -114,7 +115,7 @@ func main() { case <-ctx.Done(): return case err := <-messageSub.Err(): - errC <- fmt.Errorf("message subscription failed: %w", err) // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("message subscription failed: %w", err), "wstest_errc") return case ev := <-messageC: logger.Info("Received a log event from the contract", zap.Any("ev", ev)) diff --git a/node/pkg/adminrpc/adminserver.go b/node/pkg/adminrpc/adminserver.go index a9f6a0c443a..9807f3847c7 100644 --- a/node/pkg/adminrpc/adminserver.go +++ b/node/pkg/adminrpc/adminserver.go @@ -1063,7 +1063,7 @@ func (s *nodePrivilegedService) InjectGovernanceVAA(ctx context.Context, req *no vaaInjectionsTotal.Inc() - s.injectC <- &common.MessagePublication{ // Note on channel capacity: Only blocks this command + s.injectC <- &common.MessagePublication{ //nolint:channelcheck // Only blocks this command TxID: ethcommon.Hash{}.Bytes(), Timestamp: v.Timestamp, Nonce: v.Nonce, @@ -1165,7 +1165,7 @@ func (s *nodePrivilegedService) fetchMissing( // Inject into the gossip signed VAA receive path. // This has the same effect as if the VAA was received from the network // (verifying signature, storing in local DB...). - s.signedInC <- &gossipv1.SignedVAAWithQuorum{ // Note on channel capacity: Only blocks this command + s.signedInC <- &gossipv1.SignedVAAWithQuorum{ //nolint:channelcheck // Only blocks this command Vaa: vaaBytes, } diff --git a/node/pkg/common/scissors.go b/node/pkg/common/scissors.go index aab8b61febe..a0e9b877e31 100644 --- a/node/pkg/common/scissors.go +++ b/node/pkg/common/scissors.go @@ -67,7 +67,7 @@ func StartRunnable(ctx context.Context, errC chan error, catchPanics bool, name } // We don't want this to hang if the listener has already gone away. select { - case errC <- err: + case errC <- err: //nolint:channelcheck // intentional best-effort: silently drop if the listener has already gone away default: } ScissorsPanicsCaught.WithLabelValues(name).Inc() @@ -85,7 +85,7 @@ func startRunnable(ctx context.Context, errC chan error, name string, runnable s if err != nil { // We don't want this to hang if the listener has already gone away. select { - case errC <- err: + case errC <- err: //nolint:channelcheck // intentional best-effort: silently drop if the listener has already gone away default: } ScissorsErrorsCaught.WithLabelValues(name).Inc() diff --git a/node/pkg/governor/governor_prices.go b/node/pkg/governor/governor_prices.go index be6d9a359a9..236c0dd581a 100644 --- a/node/pkg/governor/governor_prices.go +++ b/node/pkg/governor/governor_prices.go @@ -157,7 +157,7 @@ func (gov *ChainGovernor) queryCoinGecko(ctx context.Context) error { for { select { case <-ticker.C: - throttle <- 1 // Note on channel capacity: We want this to block for throttling + throttle <- 1 //nolint:channelcheck // Blocking used for throttling CoinGecko API requests below. case <-ctx.Done(): return } diff --git a/node/pkg/node/options.go b/node/pkg/node/options.go index c4c2f7f074c..ed5c0382504 100644 --- a/node/pkg/node/options.go +++ b/node/pkg/node/options.go @@ -460,7 +460,7 @@ func GuardianOptionWatchers(watcherConfigs []watchers.WatcherConfig, ibcWatcherC zap.String("txID", msg.TxIDString()), zap.Time("timestamp", msg.Timestamp)) } else { - g.msgC.writeC <- msg // Note on channel capacity: The channel to the processor is buffered and shared across chains, if it backs up we should stop processing new observations + g.msgC.writeC <- msg //nolint:channelcheck // The channel to the processor is buffered and shared across chains, if it backs up we should stop processing new observations } } } @@ -486,7 +486,7 @@ func GuardianOptionWatchers(watcherConfigs []watchers.WatcherConfig, ibcWatcherC zap.Stringer("watcherChainId", chainId), ) } - g.queryResponseC.writeC <- response // Note on channel capacity: This channel is buffered, if it backs up we'll stop processing queries until it clears + g.queryResponseC.writeC <- response //nolint:channelcheck // This channel is buffered, if it backs up we'll stop processing queries until it clears. } } }(chainQueryResponseC[chainId], chainId) diff --git a/node/pkg/node/publicwebRunnable.go b/node/pkg/node/publicwebRunnable.go index 3c2a68440c2..d152b1494fb 100644 --- a/node/pkg/node/publicwebRunnable.go +++ b/node/pkg/node/publicwebRunnable.go @@ -164,13 +164,13 @@ func publicwebServiceRunnable( } supervisor.Signal(ctx, supervisor.SignalHealthy) - errC := make(chan error) + errC := make(chan error, 1) go func() { logger.Info("publicweb server listening", zap.String("addr", srv.Addr)) if tlsHostname != "" { - errC <- srv.ServeTLS(listener, "", "") // Note on channel capacity: Only does one write + errC <- srv.ServeTLS(listener, "", "") //nolint:channelcheck // Only does one write and one receive. } else { - errC <- srv.Serve(listener) // Note on channel capacity: Only does one write + errC <- srv.Serve(listener) //nolint:channelcheck // Only does one write and one receive. } }() select { diff --git a/node/pkg/p2p/p2p.go b/node/pkg/p2p/p2p.go index 7f8ba41071b..cd20a9bfbe7 100644 --- a/node/pkg/p2p/p2p.go +++ b/node/pkg/p2p/p2p.go @@ -923,7 +923,7 @@ func Run(params *RunParams) func(ctx context.Context) error { for { envelope, err := controlSubscription.Next(ctx) // Note: sub.Next(ctx) will return an error once ctx is canceled if err != nil { - errC <- fmt.Errorf("failed to receive pubsub message on control topic: %w", err) // Note on channel capacity: The runnable will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to receive pubsub message on control topic: %w", err), "p2p_control") return } @@ -1081,7 +1081,7 @@ func Run(params *RunParams) func(ctx context.Context) error { for { envelope, err := attestationSubscription.Next(ctx) // Note: sub.Next(ctx) will return an error once ctx is canceled if err != nil { - errC <- fmt.Errorf("failed to receive pubsub message on attestation topic: %w", err) // Note on channel capacity: The runnable will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to receive pubsub message on attestation topic: %w", err), "p2p_attestation") return } @@ -1163,7 +1163,7 @@ func Run(params *RunParams) func(ctx context.Context) error { for { envelope, err := delegatedAttestationSubscription.Next(ctx) // Note: sub.Next(ctx) will return an error once ctx is canceled if err != nil { - errC <- fmt.Errorf("failed to receive pubsub message on delegated attestation topic: %w", err) // Note on channel capacity: The runnable will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to receive pubsub message on delegated attestation topic: %w", err), "p2p_delegated_attestation") return } @@ -1307,7 +1307,7 @@ func Run(params *RunParams) func(ctx context.Context) error { for { envelope, err := vaaSubscription.Next(ctx) // Note: sub.Next(ctx) will return an error once ctx is canceled if err != nil { - errC <- fmt.Errorf("failed to receive pubsub message on vaa topic: %w", err) // Note on channel capacity: The runnable will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to receive pubsub message on vaa topic: %w", err), "p2p_vaa") return } @@ -1370,7 +1370,7 @@ func Run(params *RunParams) func(ctx context.Context) error { for { envelope, err := managerSubscription.Next(ctx) // Note: sub.Next(ctx) will return an error once ctx is canceled if err != nil { - errC <- fmt.Errorf("failed to receive pubsub message on manager topic: %w", err) //nolint:channelcheck // The runnable will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to receive pubsub message on manager topic: %w", err), "p2p_manager") return } diff --git a/node/pkg/suiclient/suigrpc.go b/node/pkg/suiclient/suigrpc.go index 510dc518780..40fa3216729 100644 --- a/node/pkg/suiclient/suigrpc.go +++ b/node/pkg/suiclient/suigrpc.go @@ -258,7 +258,7 @@ func (s *SuiGrpcClient) SubscribeToTransactionEvents(ctx context.Context, eventT } // An RPC communication error occurred. Note that this terminates the goroutine. - errorChannel <- err + errorChannel <- err //nolint:channelcheck // A single write occurs and then exits. Because it's buffered above, this is safe. return } @@ -323,7 +323,7 @@ func (s *SuiGrpcClient) SubscribeToTransactionEvents(ctx context.Context, eventT // Writing to eventWriteChannel could be blocking if there are no readers. Use a select to include // a simultaneous check to bail out if the context is cancelled. select { - case eventWriteChannel <- SuiTransactionEvent{TxDigest: txDigest, Event: *suiEvent}: + case eventWriteChannel <- SuiTransactionEvent{TxDigest: txDigest, Event: *suiEvent}: //nolint:channelcheck // Don't want to drop RPC events. Continuous producer should block when the consumer isn't able to handle more. case <-ctx.Done(): return } diff --git a/node/pkg/supervisor/supervisor.go b/node/pkg/supervisor/supervisor.go index 94a1ac453f4..ad1c2cc7cf4 100644 --- a/node/pkg/supervisor/supervisor.go +++ b/node/pkg/supervisor/supervisor.go @@ -110,7 +110,7 @@ func New(ctx context.Context, logger *zap.Logger, rootRunnable Runnable, opts .. go sup.processor(ctx) - sup.pReq <- &processorRequest{ // Note on channel capacity: Only does one write + sup.pReq <- &processorRequest{ //nolint:channelcheck // One-shot send to the processor goroutine started above; blocking is the intended startup handshake. schedule: &processorRequestSchedule{dn: "root"}, } diff --git a/node/pkg/supervisor/supervisor_node.go b/node/pkg/supervisor/supervisor_node.go index 5079969c26a..d7755662ab9 100644 --- a/node/pkg/supervisor/supervisor_node.go +++ b/node/pkg/supervisor/supervisor_node.go @@ -238,7 +238,7 @@ func (n *node) runGroup(runnables map[string]Runnable) error { // Schedule execution of group members. go func() { for name := range runnables { - n.sup.pReq <- &processorRequest{ // Note on channel capacity: Will only block this go routine + n.sup.pReq <- &processorRequest{ //nolint:channelcheck // Will only block this go routine schedule: &processorRequestSchedule{ dn: dns[name], }, diff --git a/node/pkg/supervisor/supervisor_processor.go b/node/pkg/supervisor/supervisor_processor.go index 8fe0b1379f7..0e0af7be9dd 100644 --- a/node/pkg/supervisor/supervisor_processor.go +++ b/node/pkg/supervisor/supervisor_processor.go @@ -134,7 +134,7 @@ func (s *supervisor) processSchedule(r *processorRequestSchedule) { if !s.propagatePanic { defer func() { if rec := recover(); rec != nil { - s.pReq <- &processorRequest{ // Note on channel capacity: Will only block this go routine + s.pReq <- &processorRequest{ //nolint:channelcheck // Will only block this go routine's defer handler died: &processorRequestDied{ dn: r.dn, err: fmt.Errorf("panic: %v, stacktrace: %s", rec, string(debug.Stack())), @@ -146,7 +146,7 @@ func (s *supervisor) processSchedule(r *processorRequestSchedule) { res := n.runnable(n.ctx) - s.pReq <- &processorRequest{ // Note on channel capacity: Will only block this go routine + s.pReq <- &processorRequest{ //nolint:channelcheck // Will only block this go routine. died: &processorRequestDied{ dn: r.dn, err: res, @@ -387,7 +387,7 @@ func (s *supervisor) processGC() { // Reschedule node runnable to run after backoff. go func(n *node, bo time.Duration) { time.Sleep(bo) //nolint:forbidigo // TODO: This code should be refactored to not use time.Sleep - s.pReq <- &processorRequest{ // Note on channel capacity: Will only block this go routine + s.pReq <- &processorRequest{ //nolint:channelcheck // Will only block this go routine. schedule: &processorRequestSchedule{dn: n.dn()}, } }(n, bo) diff --git a/node/pkg/supervisor/supervisor_support.go b/node/pkg/supervisor/supervisor_support.go index 99570eb55f3..a86b33c27a9 100644 --- a/node/pkg/supervisor/supervisor_support.go +++ b/node/pkg/supervisor/supervisor_support.go @@ -16,9 +16,9 @@ import ( func GRPCServer(srv *grpc.Server, lis net.Listener, graceful bool) Runnable { return func(ctx context.Context) error { Signal(ctx, SignalHealthy) - errC := make(chan error) + errC := make(chan error, 1) go func() { - errC <- srv.Serve(lis) // Note on channel capacity: Will only block this go routine + errC <- srv.Serve(lis) //nolint:channelcheck // A single write occurs on a buffered channel. Safe pattern that can't deadlock. }() select { case <-ctx.Done(): diff --git a/node/pkg/telemetry/loki.go b/node/pkg/telemetry/loki.go index bf0059b6961..85536523fba 100644 --- a/node/pkg/telemetry/loki.go +++ b/node/pkg/telemetry/loki.go @@ -208,8 +208,9 @@ func logWriter(ctx context.Context, logger *zap.Logger, localC chan api.Entry, w } // Write to Loki in a blocking manner unless we are signaled to shutdown. + // The write to `localC` above skips the write if the buffered channel is full. So, we can block on this channel write. select { - case c.Chan() <- entry: // Note on channel capacity: We want to block on the Loki client. + case c.Chan() <- entry: //nolint:channelcheck // We want to block on the Loki client write once we have received the log. pendingEntry = nil case <-ctx.Done(): // Time to shutdown. We probably failed to write this message, save it so we can try to flush it. @@ -240,7 +241,7 @@ func flushLogsWithTimeout(localC chan api.Entry, c client.Client, pendingEntry * if pendingEntry != nil { select { - case c.Chan() <- *pendingEntry: // Note on channel capacity: We want to block on the Loki client. The timeout will interrupt us. + case c.Chan() <- *pendingEntry: //nolint:channelcheck // We want to block on the Loki client. The timeout will interrupt this if it's taking too long. case <-timeout.Done(): // If we timeout, we didn't write the pending one, so count that as remaining. return (1 + len(localC)), errors.New("timeout writing pending entry") @@ -250,7 +251,12 @@ func flushLogsWithTimeout(localC chan api.Entry, c client.Client, pendingEntry * for len(localC) > 0 { select { case entry := <-localC: - c.Chan() <- entry // Note on channel capacity: We want to block on the Loki client. The timeout will interrupt us. + select { + case c.Chan() <- entry: //nolint:channelcheck // Bounded by the timeout context below; we want to block on the Loki client until then. + case <-timeout.Done(): + // If we timeout, we didn't write the one we just pulled, so count that as remaining. + return (1 + len(localC)), errors.New("timeout flushing buffered entry") + } case <-timeout.Done(): // If we timeout, we didn't write the current one, so count that as remaining. return (1 + len(localC)), errors.New("timeout flushing buffered entry") @@ -282,7 +288,7 @@ func stopClientWithTimeout(c client.Client) error { stopExitedC := make(chan struct{}, 1) go func(c client.Client) { c.StopNow() - stopExitedC <- struct{}{} // Note on channel capacity: We only do a single write. + stopExitedC <- struct{}{} //nolint:channelcheck // We only do a single write on a buffered channel. }(c) // Wait for the go routine to exit or the timer to expire. Using `time.After` since this is a one shot and we don't have the context. diff --git a/node/pkg/txverifier/evmtypes.go b/node/pkg/txverifier/evmtypes.go index 1e83f789f77..4a120f8f53a 100644 --- a/node/pkg/txverifier/evmtypes.go +++ b/node/pkg/txverifier/evmtypes.go @@ -13,6 +13,7 @@ import ( "math/big" "time" + node_common "github.com/certusone/wormhole/node/pkg/common" connectors "github.com/certusone/wormhole/node/pkg/watchers/evm/connectors" "github.com/certusone/wormhole/node/pkg/watchers/evm/connectors/ethabi" "github.com/ethereum/go-ethereum" @@ -275,8 +276,8 @@ func (s *Subscription) Subscribe(ctx context.Context) { ) if err != nil { - s.errC <- fmt.Errorf("failed to subscribe to logs: %w", err) // Note on channel capacity: Will only block this subscriber routine - time.Sleep(RECONNECT_DELAY) //nolint:forbidigo // TODO: This code should be refactored to not use time.Sleep; // Wait before retrying + node_common.WriteToChannelWithoutBlocking(s.errC, fmt.Errorf("failed to subscribe to logs: %w", err), "txverifier_errc") + time.Sleep(RECONNECT_DELAY) //nolint:forbidigo // TODO: This code should be refactored to not use time.Sleep; // Wait before retrying continue } @@ -286,7 +287,7 @@ func (s *Subscription) Subscribe(ctx context.Context) { err = s.handleSubscription(ctx, subscription) if err != nil { - s.errC <- err // Note on channel capacity: Will only block this subscriber routine + node_common.WriteToChannelWithoutBlocking(s.errC, err, "txverifier_errc") time.Sleep(RECONNECT_DELAY) //nolint:forbidigo // TODO: This code should be refactored to not use time.Sleep; // Wait before retrying } } diff --git a/node/pkg/watchers/cosmwasm/watcher.go b/node/pkg/watchers/cosmwasm/watcher.go index 07296aec0b4..9e4558c3d03 100644 --- a/node/pkg/watchers/cosmwasm/watcher.go +++ b/node/pkg/watchers/cosmwasm/watcher.go @@ -221,7 +221,7 @@ func (e *Watcher) Run(ctx context.Context) error { blocksBody, err := common.SafeRead(resp.Body) if err != nil { logger.Error("query latest block response read error", zap.String("network", e.networkName), zap.Error(err)) - errC <- err // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, err, "cosmwasm_errc") resp.Body.Close() continue } @@ -321,7 +321,7 @@ func (e *Watcher) Run(ctx context.Context) error { p2p.DefaultRegistry.AddErrorCount(e.chainID, 1) connectionErrors.WithLabelValues(e.networkName, "channel_read_error").Inc() logger.Error("error reading channel", zap.String("network", e.networkName), zap.Error(err)) - errC <- err // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, err, "cosmwasm_errc") return nil } diff --git a/node/pkg/watchers/evm/connectors/batch_poller.go b/node/pkg/watchers/evm/connectors/batch_poller.go index 6978bc053b0..8f09a55513b 100644 --- a/node/pkg/watchers/evm/connectors/batch_poller.go +++ b/node/pkg/watchers/evm/connectors/batch_poller.go @@ -85,11 +85,11 @@ func (b *BatchPollConnector) SubscribeForBlocks(ctx context.Context, errC chan e // Publish the initial finalized and safe blocks so we have a starting point for reobservation requests. for idx, block := range lastBlocks { b.logger.Info(fmt.Sprintf("publishing initial %s block", b.batchData[idx].finality), zap.Uint64("initial_block", block.Number.Uint64())) - sink <- block // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- block //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears if b.generateSafe && b.batchData[idx].finality == Finalized { safe := block.Copy(Safe) b.logger.Info("publishing generated initial safe block", zap.Uint64("initial_block", safe.Number.Uint64())) - sink <- safe // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- safe //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears } } @@ -106,7 +106,7 @@ func (b *BatchPollConnector) SubscribeForBlocks(ctx context.Context, errC chan e errCount++ b.logger.Error("batch polling encountered an error", zap.Int("errCount", errCount), zap.Error(err)) if errCount > 3 { - errC <- fmt.Errorf("polling encountered too many errors: %w", err) // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("polling encountered too many errors: %w", err), "evm_errc") return nil } } else if errCount != 0 { @@ -122,7 +122,7 @@ func (b *BatchPollConnector) SubscribeForBlocks(ctx context.Context, errC chan e b.logger.Error("new latest header block number is nil") continue } - sink <- &NewBlock{ // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- &NewBlock{ //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears Number: ev.Number, Time: ev.Time, Hash: ev.Hash(), @@ -201,9 +201,9 @@ func (b *BatchPollConnector) pollBlocks(ctx context.Context, sink chan<- *NewBlo errorFound = true break } - sink <- block // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- block //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears if b.generateSafe && b.batchData[idx].finality == Finalized { - sink <- block.Copy(Safe) // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- block.Copy(Safe) //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears } lastPublishedBlock = block } @@ -214,9 +214,9 @@ func (b *BatchPollConnector) pollBlocks(ctx context.Context, sink chan<- *NewBlo if !errorFound { // The original value of newBlocks is still good. - sink <- newBlock // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- newBlock //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears if b.generateSafe && b.batchData[idx].finality == Finalized { - sink <- newBlock.Copy(Safe) // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- newBlock.Copy(Safe) //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears } } else { newBlocks[idx] = lastPublishedBlock diff --git a/node/pkg/watchers/evm/connectors/common.go b/node/pkg/watchers/evm/connectors/common.go index c5607800428..65842464ceb 100644 --- a/node/pkg/watchers/evm/connectors/common.go +++ b/node/pkg/watchers/evm/connectors/common.go @@ -91,7 +91,7 @@ func (sub *PollSubscription) Err() <-chan error { func (sub *PollSubscription) Unsubscribe() { sub.errOnce.Do(func() { select { - case sub.quit <- ErrUnsubscribed: // Note on channel capacity: We only do a single write. + case sub.quit <- ErrUnsubscribed: //nolint:channelcheck // We only do a single write. Additionally, the other receive branch will likely have already triggered. <-sub.unsubDone case <-sub.unsubDone: } @@ -109,7 +109,7 @@ func (sub *PollSubscription) Unsubscribe() { // non-blocking: unsubDone is buffered and only ever needs a single value. func (sub *PollSubscription) signalUnsubscribed() { select { - case sub.unsubDone <- struct{}{}: //nolint:channelcheck // Buffered; single signal. + case sub.unsubDone <- struct{}{}: //nolint:channelcheck // Buffered and single signal. If it's already blocked, then we're in the shutdown phase anyway so we can just continue. default: } } diff --git a/node/pkg/watchers/evm/connectors/instant_finality.go b/node/pkg/watchers/evm/connectors/instant_finality.go index a92a7fe0416..9e2779e5ef4 100644 --- a/node/pkg/watchers/evm/connectors/instant_finality.go +++ b/node/pkg/watchers/evm/connectors/instant_finality.go @@ -54,9 +54,9 @@ func (c *InstantFinalityConnector) SubscribeForBlocks(ctx context.Context, errC Hash: ev.Hash(), Finality: Finalized, } - sink <- block // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears - sink <- block.Copy(Safe) // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears - sink <- block.Copy(Latest) // Note on channel capacity: This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- block //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- block.Copy(Safe) //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears + sink <- block.Copy(Latest) //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears } } }) diff --git a/node/pkg/watchers/evm/connectors/poller.go b/node/pkg/watchers/evm/connectors/poller.go index 6c4ad61de38..cc7b575330f 100644 --- a/node/pkg/watchers/evm/connectors/poller.go +++ b/node/pkg/watchers/evm/connectors/poller.go @@ -93,11 +93,11 @@ func (p *PollConnector) SubscribeForBlocks(ctx context.Context, errC chan error, for idx, block := range lastBlocks { p.logger.Info(fmt.Sprintf("publishing initial %s block", p.batchData[idx].finality), zap.Uint64("initial_block", block.Number.Uint64())) - sink <- block + sink <- block //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears if p.generateSafe && p.batchData[idx].finality == Finalized { safe := block.Copy(Safe) p.logger.Info("publishing generated initial safe block", zap.Uint64("initial_block", safe.Number.Uint64())) - sink <- safe + sink <- safe //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears } } @@ -234,7 +234,7 @@ func (p *PollConnector) watchLogMessagePublishedFrom(ctx context.Context, errC c p.logger.Error("log poller failed to parse log", zap.Error(err)) continue } - sink <- ev + sink <- ev //nolint:channelcheck // This channel is buffered, if it backs up, we will just stop polling until it clears } fromBlock = toBlock + 1 diff --git a/node/pkg/watchers/evm/watcher.go b/node/pkg/watchers/evm/watcher.go index 1c4ab4cdb56..d565eff42e8 100644 --- a/node/pkg/watchers/evm/watcher.go +++ b/node/pkg/watchers/evm/watcher.go @@ -388,7 +388,7 @@ func (w *Watcher) Run(parentCtx context.Context) error { return nil case <-t.C: if pollErr := w.fetchAndUpdateGuardianSet(ctx, w.ethConn); pollErr != nil { - errC <- fmt.Errorf("failed to request guardian set: %v", pollErr) // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to request guardian set: %v", pollErr), "evm_errc") return nil } } @@ -414,7 +414,7 @@ func (w *Watcher) Run(parentCtx context.Context) error { return nil case <-t.C: if pollErr := w.fetchAndUpdateDelegatedGuardianConfig(ctx); pollErr != nil { - errC <- fmt.Errorf("failed to request delegated guardian config: %v", pollErr) // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to request delegated guardian config: %v", pollErr), "evm_errc") return nil } } @@ -477,7 +477,7 @@ func (w *Watcher) Run(parentCtx context.Context) error { return nil case subErr := <-messageSub.Err(): ethConnectionErrors.WithLabelValues(w.networkName, "subscription_error").Inc() - errC <- fmt.Errorf("error while processing message publication subscription: %w", subErr) // The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("error while processing message publication subscription: %w", subErr), "evm_errc") p2p.DefaultRegistry.AddErrorCount(w.chainID, 1) return nil case ev := <-messageC: @@ -489,7 +489,7 @@ func (w *Watcher) Run(parentCtx context.Context) error { continue } p2p.DefaultRegistry.AddErrorCount(w.chainID, 1) - errC <- fmt.Errorf("failed to request timestamp for block %d, hash %s: %w", ev.Raw.BlockNumber, ev.Raw.BlockHash.String(), blockErr) // The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to request timestamp for block %d, hash %s: %w", ev.Raw.BlockNumber, ev.Raw.BlockHash.String(), blockErr), "evm_errc") return nil } @@ -517,7 +517,7 @@ func (w *Watcher) Run(parentCtx context.Context) error { case err := <-headerSubscription.Err(): logger.Error("error while processing header subscription", zap.Error(err)) ethConnectionErrors.WithLabelValues(w.networkName, "header_subscription_error").Inc() - errC <- fmt.Errorf("error while processing header subscription: %w", err) // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("error while processing header subscription: %w", err), "evm_errc") p2p.DefaultRegistry.AddErrorCount(w.chainID, 1) return nil case ev := <-headSink: @@ -541,9 +541,9 @@ func (w *Watcher) Run(parentCtx context.Context) error { readiness.SetReady(w.readinessSync) if err := w.processNewBlock(ctx, ev, &stats); err != nil { - errC <- err + common.WriteToChannelWithoutBlocking(errC, err, "evm_errc") p2p.DefaultRegistry.AddErrorCount(w.chainID, 1) - return nil //nolint:nilerr // error propagated via errC + return nil } } } @@ -587,7 +587,11 @@ func (w *Watcher) fetchAndUpdateGuardianSet( w.currentGuardianSet = &idx if w.setC != nil { - w.setC <- common.NewGuardianSet(gs.Keys, idx) // Note on channel capacity: Will only block the guardian set update routine + select { + case w.setC <- common.NewGuardianSet(gs.Keys, idx): //nolint:channelcheck // Shutdown is safe. Should block until consumable or context is closed. + case <-ctx.Done(): + return ctx.Err() + } } return nil @@ -677,7 +681,11 @@ func (w *Watcher) fetchAndUpdateDelegatedGuardianConfig( zap.Uint32("timestamp", cfg.Timestamp)) } - w.dgConfigC <- dgConfig // Note on channel capacity: Will only block the delegated guardian config update routine + select { + case w.dgConfigC <- dgConfig: //nolint:channelcheck // Shutdown is safe. Should block until consumable or context is closed. + case <-ctx.Done(): + return ctx.Err() + } w.logger.Info("sent delegated guardian config update to processor") } @@ -1197,7 +1205,7 @@ func (w *Watcher) waitForBlockTime(ctx context.Context, errC chan error, ev *eth ethConnectionErrors.WithLabelValues(w.networkName, "block_by_number_error").Inc() if !canRetryGetBlockTime(err) { p2p.DefaultRegistry.AddErrorCount(w.chainID, 1) - errC <- fmt.Errorf("failed to request timestamp for block %d, hash %s: %w", ev.Raw.BlockNumber, ev.Raw.BlockHash.String(), err) // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(errC, fmt.Errorf("failed to request timestamp for block %d, hash %s: %w", ev.Raw.BlockNumber, ev.Raw.BlockHash.String(), err), "evm_errc") return } if retries >= MaxRetries { diff --git a/node/pkg/watchers/mock/watcher.go b/node/pkg/watchers/mock/watcher.go index afd6773959b..b6f24d546d2 100644 --- a/node/pkg/watchers/mock/watcher.go +++ b/node/pkg/watchers/mock/watcher.go @@ -29,9 +29,9 @@ func NewWatcherRunnable( return nil case observation := <-c.MockObservationC: logger.Info("message observed", observation.ZapFields(zap.String("digest", observation.CreateDigest()))...) - msgC <- observation // Note on channel capacity: The channel to the processor is buffered and shared across chains, if it backs up we should stop processing new observations + msgC <- observation // Note on channel capacity: Will only block this mock watcher. Shouldn't drop observations anyway. case gs := <-c.MockSetC: - setC <- gs // Note on channel capacity: Will only block this mock watcher + setC <- gs //nolint:channelcheck // Will only block this mock watcher. Shouldn't drop guardian set updates anyway. case o := <-obsvReqC: hash := eth_common.BytesToHash(o.TxHash) logger.Info("Received obsv request", zap.String("log_msg_type", "obsv_req_received"), zap.String("tx_hash", hash.Hex())) @@ -39,7 +39,7 @@ func NewWatcherRunnable( if ok { msg2 := *msg msg2.IsReobservation = true - msgC <- &msg2 // Note on channel capacity: The channel to the processor is buffered and shared across chains, if it backs up we should stop processing new observations + msgC <- &msg2 // Note on channel capacity: Will only block this mock watcher. Shouldn't drop observations anyway. } } } diff --git a/node/pkg/watchers/near/finalizer.go b/node/pkg/watchers/near/finalizer.go index 612146651a1..c36c38c757b 100644 --- a/node/pkg/watchers/near/finalizer.go +++ b/node/pkg/watchers/near/finalizer.go @@ -4,6 +4,7 @@ import ( "context" "errors" + "github.com/certusone/wormhole/node/pkg/common" "github.com/certusone/wormhole/node/pkg/watchers/near/nearapi" lru "github.com/hashicorp/golang-lru" "go.uber.org/zap" @@ -64,7 +65,7 @@ func (f Finalizer) isFinalized(logger *zap.Logger, ctx context.Context, queriedB } logger.Debug("block finalization cache miss", zap.String("method", "isFinalized"), zap.String("parameters", queriedBlockHash)) - f.eventChan <- EVENT_FINALIZED_CACHE_MISS // Note on channel capacity: Only pauses this watcher + common.WriteToChannelWithoutBlocking(f.eventChan, EVENT_FINALIZED_CACHE_MISS, "near_event_chan") // Non-blocking: eventChan is buffered (cap 10) and metrics-only, so dropping an event is preferable to stalling the watcher. queriedBlock, err := f.nearAPI.GetBlock(ctx, queriedBlockHash) if err != nil { diff --git a/node/pkg/watchers/near/poll.go b/node/pkg/watchers/near/poll.go index 0c084f065b5..605f50ed469 100644 --- a/node/pkg/watchers/near/poll.go +++ b/node/pkg/watchers/near/poll.go @@ -4,6 +4,7 @@ import ( "context" "errors" + "github.com/certusone/wormhole/node/pkg/common" "github.com/certusone/wormhole/node/pkg/watchers/near/nearapi" "go.uber.org/zap" ) @@ -50,7 +51,7 @@ func (e *Watcher) recursivelyReadFinalizedBlocks(logger *zap.Logger, ctx context // we want to avoid going too far back because that would increase the likelihood of error somewhere in the recursion stack. // If we go back too far, we just report the error and terminate early. if recursionDepth > maxFallBehindBlocks { - e.eventChan <- EVENT_NEAR_WATCHER_TOO_FAR_BEHIND // Note on channel capacity: Only pauses this watcher + common.WriteToChannelWithoutBlocking(e.eventChan, EVENT_NEAR_WATCHER_TOO_FAR_BEHIND, "near_event_chan") // Non-blocking: eventChan is buffered (cap 10) and metrics-only, so dropping an event is preferable to stalling the watcher. return errors.New("recursivelyReadFinalizedBlocks: maxFallBehindBlocks") } @@ -71,7 +72,11 @@ func (e *Watcher) recursivelyReadFinalizedBlocks(logger *zap.Logger, ctx context chunks := startBlock.ChunkHashes() // process chunks after recursion such that youngest chunks get processed first for i := 0; i < len(chunks); i++ { - chunkSink <- chunks[i] // Note on channel capacity: Only pauses this watcher + select { + case chunkSink <- chunks[i]: //nolint:channelcheck // Blocks on consumer backpressure (chunks are critical data, never dropped); ctx.Done() allows shutdown. + case <-ctx.Done(): + return ctx.Err() + } } return nil } diff --git a/node/pkg/watchers/near/tx_processing.go b/node/pkg/watchers/near/tx_processing.go index 53cb1e6a837..1d7d6099a8d 100644 --- a/node/pkg/watchers/near/tx_processing.go +++ b/node/pkg/watchers/near/tx_processing.go @@ -256,7 +256,7 @@ func (e *Watcher) processWormholeLog(logger *zap.Logger, _ context.Context, job // tell everyone about it job.hasWormholeMsg = true - e.eventChan <- EVENT_NEAR_MESSAGE_CONFIRMED // Note on channel capacity: Only pauses this watcher + common.WriteToChannelWithoutBlocking(e.eventChan, EVENT_NEAR_MESSAGE_CONFIRMED, "near_event_chan") // Non-blocking: eventChan is buffered (cap 10) and metrics-only, so dropping an event is preferable to stalling the watcher. logger.Info("message observed", zap.String("log_msg_type", "wormhole_event_success"), diff --git a/node/pkg/watchers/near/watcher.go b/node/pkg/watchers/near/watcher.go index 344485c093d..27ffb733042 100644 --- a/node/pkg/watchers/near/watcher.go +++ b/node/pkg/watchers/near/watcher.go @@ -266,7 +266,7 @@ func (e *Watcher) runTxProcessor(ctx context.Context) error { if job.hasWormholeMsg { // report how long it took to process this transaction - e.eventChanTxProcessedDuration <- time.Since(job.creationTime) // Note on channel capacity: Only pauses this watcher + common.WriteToChannelWithoutBlocking(e.eventChanTxProcessedDuration, time.Since(job.creationTime), "near_event_chan_tx_processed_duration") // Non-blocking: eventChanTxProcessedDuration is buffered (cap 10) and metrics-only, so dropping a sample is preferable to stalling the watcher. } } @@ -346,7 +346,7 @@ func (e *Watcher) schedule(ctx context.Context, job *transactionProcessingJob, d select { case <-ctx.Done(): return nil - case e.transactionProcessingQueue <- job: // Note on channel capacity: Only blocking this go routine. + case e.transactionProcessingQueue <- job: //nolint:channelcheck // Buffered channel and we drop the message if over the queue size above. } } return nil diff --git a/node/pkg/watchers/solana/client.go b/node/pkg/watchers/solana/client.go index cd85e43f52f..36953be8098 100644 --- a/node/pkg/watchers/solana/client.go +++ b/node/pkg/watchers/solana/client.go @@ -413,7 +413,11 @@ func (s *SolanaWatcher) setupWebSocket(ctx context.Context) error { logger.Error("failed to read from account web socket", zap.Error(err)) return err } else { - s.pumpData <- msg // Note on channel capacity: Only pauses this watcher + select { + case s.pumpData <- msg: //nolint:channelcheck // Pauses on backpressure from consumer. Allows for shutdown if consumer no longer exists. + case <-ctx.Done(): + return nil + } } } } @@ -498,7 +502,7 @@ func (s *SolanaWatcher) Run(ctx context.Context) error { if err != nil { p2p.DefaultRegistry.AddErrorCount(s.chainID, 1) solanaConnectionErrors.WithLabelValues(s.networkName, string(s.commitment), "account_subscription_data").Inc() - s.errC <- err // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(s.errC, err, "solana_errc") return err } case m := <-s.obsvReqC: @@ -537,7 +541,7 @@ func (s *SolanaWatcher) Run(ctx context.Context) error { if err != nil { p2p.DefaultRegistry.AddErrorCount(s.chainID, 1) solanaConnectionErrors.WithLabelValues(s.networkName, string(s.commitment), "get_slot_error").Inc() - s.errC <- err // Note on channel capacity: The watcher will exit anyway + common.WriteToChannelWithoutBlocking(s.errC, err, "solana_errc") return err } diff --git a/node/pkg/watchers/solana/tx_for_addr.go b/node/pkg/watchers/solana/tx_for_addr.go index e9e84ba0d0b..26ad6ddce7b 100644 --- a/node/pkg/watchers/solana/tx_for_addr.go +++ b/node/pkg/watchers/solana/tx_for_addr.go @@ -17,6 +17,7 @@ import ( "slices" "time" + "github.com/certusone/wormhole/node/pkg/common" "github.com/gagliardetto/solana-go" "github.com/gagliardetto/solana-go/rpc" "go.uber.org/zap" @@ -39,7 +40,7 @@ func (s *SolanaWatcher) transactionProcessor(ctx context.Context) error { s.pollPrevWormholeSignature, err = s.getPrevWormholeSignature() if err != nil { s.logger.Error("failed to get the last wormhole signature on start up", zap.Error(err)) - s.errC <- err + common.WriteToChannelWithoutBlocking(s.errC, err, "solana_errc") return err } } @@ -58,7 +59,7 @@ func (s *SolanaWatcher) transactionProcessor(ctx context.Context) error { err := s.processNewTransactions() if err != nil { s.logger.Error("failed to get transactions", zap.Error(err)) - s.errC <- err + common.WriteToChannelWithoutBlocking(s.errC, err, "solana_errc") return err } } diff --git a/node/pkg/watchers/xrpl/watcher.go b/node/pkg/watchers/xrpl/watcher.go index 9a772ddf254..88c9056196f 100644 --- a/node/pkg/watchers/xrpl/watcher.go +++ b/node/pkg/watchers/xrpl/watcher.go @@ -257,7 +257,7 @@ func (w *Watcher) Run(ctx context.Context) error { if msg != nil { msg.IsReobservation = true - w.msgChan <- msg + w.msgChan <- msg // Note on channel capacity: We never want to drop messages. watchers.ReobservationsByChain.WithLabelValues("xrpl", "std").Inc() } } @@ -351,7 +351,7 @@ func (w *Watcher) processTransaction(tx *streamtypes.TransactionStream) error { } // Send to processor - w.msgChan <- msg + w.msgChan <- msg // Note on channel capacity: We never want to drop messages. xrplMessagesConfirmed.Inc() w.logger.Info("message observed", msg.ZapFields()...) diff --git a/scripts/Dockerfile.lint b/scripts/Dockerfile.lint index a66f32d0648..231de7e1628 100644 --- a/scripts/Dockerfile.lint +++ b/scripts/Dockerfile.lint @@ -1,13 +1,21 @@ # syntax=docker.io/docker/dockerfile:1.3@sha256:42399d4635eddd7a9b8a24be879d2f9a930d0ed040a61324cfdf59ef1357b3b2 FROM docker.io/golang:1.25.10-bookworm@sha256:154bd7001b6eb339e88c964442c0ad6ed5e53f09844cc818a41ce4ecb3ce3b43 +# make: drives the custom linter build via linters/Makefile. +RUN apt-get update && apt-get install -y --no-install-recommends make \ + && rm -rf /var/lib/apt/lists/* + RUN useradd -u 1000 -U -m -d /home/lint lint USER 1000 WORKDIR /home/lint -# install goimports +# install goimports (used by `format`) RUN go install golang.org/x/tools/cmd/goimports@latest -# install golangci-lint -RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | \ - sh -s -- -b $(go env GOPATH)/bin v1.52.2 +# Build the custom wormhole-golangci-lint (golangci-lint with our linters/ module +# plugins) from source into the image and put it on PATH, so `-c lint` runs it +# without writing to the read-only repo mount. Rebuilt when anything under +# linters/ changes (Docker layer cache). +COPY --chown=1000:1000 linters/ /home/lint/linters/ +RUN make -C /home/lint/linters clean && make -C /home/lint/linters build-golangci-lint +ENV PATH="/home/lint/linters/bin:${PATH}" diff --git a/scripts/lint.sh b/scripts/lint.sh index da82a8b4b2e..ef569475ce3 100755 --- a/scripts/lint.sh +++ b/scripts/lint.sh @@ -6,6 +6,14 @@ set -eo pipefail -o nounset ROOT="$(dirname "$(dirname "$(realpath "$0")")")" DOCKERFILE="$ROOT/scripts/Dockerfile.lint" +# The custom golangci-lint (with our linters/ module plugins baked in) is built +# from source under linters/ via `make -C linters build-golangci-lint`. The +# golangci-lint version is owned by linters/ (GOLANGCI_LINT_HASH in +# linters/Makefile, whose tag comment must match `version:` in +# linters/.custom-gcl.yml). The Go build cache makes rebuilds a near-no-op after +# the first build, so we just rebuild every run rather than caching the binary. +LINTERS_DIR="$ROOT/linters" + VALID_COMMANDS=("lint" "format") SELF_ARGS_WITHOUT_DOCKER="" @@ -43,7 +51,7 @@ format(){ fi # Use -exec because of pitfall #1 in http://mywiki.wooledge.org/BashPitfalls - GOFMT_OUTPUT="$(find "./sdk" "./node" "./wormchain" -type f -name '*.go' -not -path '*.pb.go' -print0 | xargs -r -0 goimports $GOIMPORTS_ARGS 2>&1)" + GOFMT_OUTPUT="$(find "./sdk" "./node" "./wormchain" "./linters" -type f -name '*.go' -not -path '*.pb.go' -print0 | xargs -r -0 goimports $GOIMPORTS_ARGS 2>&1)" if [ -n "$GOFMT_OUTPUT" ]; then if [ "$GITHUB_ACTION" == "true" ]; then @@ -54,6 +62,22 @@ format(){ fi } +ensure_wormhole_golangci_lint() { + # In the -c docker image the custom linter is already built from source and + # placed on PATH; run that instead of rebuilding (the repo mount is read-only). + if command -v wormhole-golangci-lint >/dev/null 2>&1; then + command -v wormhole-golangci-lint + return + fi + + # Build the custom golangci-lint. The Go build cache makes this a near-no-op + # once it's been built once, so we don't cache the binary ourselves. + # (stderr only — stdout is the binary path.) + echo "Building wormhole-golangci-lint..." >&2 + make -C "$LINTERS_DIR" build-golangci-lint >&2 + echo "$LINTERS_DIR/bin/wormhole-golangci-lint" +} + lint(){ # === Spell check if ! command -v cspell >/dev/null 2>&1; then @@ -61,19 +85,26 @@ lint(){ else cspell "*/**.*md" fi - - # === Go linting - # Check for dependencies - if ! command -v golangci-lint >/dev/null 2>&1; then - printf "%s\n" "Require golangci-lint. You can run this command in a docker container instead with '-c' and not worry about it or install it: https://golangci-lint.run/usage/install/" - fi - - # Do the actual linting! - cd "$ROOT"/node - golangci-lint run --timeout=10m $GOLANGCI_LINT_ARGS ./... - cd "${ROOT}/sdk" - golangci-lint run --timeout=10m $GOLANGCI_LINT_ARGS ./... + # === Go linting (custom wormhole-golangci-lint) + local LINT_BIN + LINT_BIN="$(ensure_wormhole_golangci_lint)" + + # Lint node, sdk, and linters/ is the custom linter's own code; its + # rules// are separate modules (mirroring `make -C linters test`), so + # lint each one too. Run in a subshell so a failure still halts under + # `set -e` without leaking the working directory between modules. + # NOTE this does NOT lint the wormchain directory. + lint_module() { + ( cd "$1" && "$LINT_BIN" run --timeout=10m $GOLANGCI_LINT_ARGS ./... ) + } + + lint_module "$ROOT/node" + lint_module "$ROOT/sdk" + lint_module "$ROOT/linters" + while IFS= read -r gomod; do + lint_module "$(dirname "$gomod")" + done < <(find "$ROOT/linters/rules" -name go.mod) } DOCKER="false"