nodebuilder/rpc: Make maxConcurrentConns configurable

[mohsenm4]

Summary

Fixes #5071. After the RPC hardening in #4909 shipped in v0.31.3, the connLimit middleware caps all incoming HTTP connections at a hardcoded 500. Websocket subscriptions occupy a slot for the lifetime of the connection, so a bridge with many long-lived subscribers exhausts the pool; the next WS upgrade gets HTTP 503 back, which surfaces client-side as websocket: bad handshake. Operators had no way to raise the cap without a custom build.

This PR:

• Adds MaxConcurrentConns to nodebuilder/rpc.Config (default 500, must be > 0).
• Adds --rpc.max-concurrent-conns flag (0 = fall back to config value).
• Threads the value through api/rpc.NewServer into connLimit. A zero value passed to NewServer falls back to DefaultMaxConcurrentConns so callers that leave the field unset keep the pre-change behavior.
• Tests: a wiring test that constructs the server with limit=1, blocks one request, and asserts the second gets 503; a fallback test for the zero-value path; validation-rejection tests for ≤ 0.

Out of scope (issue questions Q3 and Q5): a separate WS-only limit / exempting WS upgrades from the shared cap, and surfacing the HTTP reject reason on failed upgrades. Both are worth doing as follow-ups but are larger design changes.

Test plan

• go build ./...
• go vet ./... (no new warnings introduced)
• golangci-lint run --new-from-rev=origin/main — 0 new issues
• go test ./api/rpc/... ./nodebuilder/rpc/...
• go test -short ./nodebuilder/... — full nodebuilder suite passes
• Manual: set MaxConcurrentConns to a low value in config.toml, confirm the new cap is honored and default value still works when unset

[greptile-apps]

Greptile Summary

This PR makes the RPC connection cap configurable. The main changes are:

• Adds MaxConcurrentConns to nodebuilder/rpc.Config with a default of 500.
• Adds --rpc.max-concurrent-conns as a CLI override.
• Threads the configured value into api/rpc.NewServer and connLimit.
• Adds tests for configured limits, zero-value fallback, and config validation.

Confidence Score: 4/5

Mostly safe to merge after fixing the CLI validation issue.

Core server wiring and config defaulting are simple and covered by tests. One contained bug remains: negative CLI overrides are accepted and ignored instead of rejected.

Files Needing Attention: nodebuilder/rpc/flags.go

T-Rex Logs

What T-Rex did

• I reproduced that setting --rpc.max-concurrent-conns to -1 is ignored, because ParseFlags returns nil and the MaxConcurrentConns value stays at 128.
• A focused Go test source was generated to exercise the negative-override scenario in the nodebuilder/rpc package.
• Runtime tests across configurations showed that a configured limit of 0 yields an effective limit of 500 with a second request returning 200 OK, and a configured limit of 1 yields an effective limit of 1 with a second request returning 503 while the first held request completes 200 OK after release.
• The tests TestServer_MaxConcurrentConns_Configurable and TestServer_MaxConcurrentConns_ZeroFallsBackToDefault passed.
• Artifacts were created and reviewed to verify the results, including the repro test source, its output, and the runtime logs.

_{Ran code and verified through T-Rex}

Important Files Changed

Filename	Overview
api/rpc/server.go	Adds the exported default connection cap and stores the configured limit on Server.
api/rpc/server_test.go	Updates NewServer call sites and tests configured limiting plus zero-value fallback.
nodebuilder/rpc/config.go	Adds MaxConcurrentConns to RPC config defaults and validation.
nodebuilder/rpc/config_test.go	Covers the new config default and invalid non-positive values.
nodebuilder/rpc/constructors.go	Passes the configured connection cap into api/rpc.NewServer.
nodebuilder/rpc/flags.go	Adds the CLI override, but negative values are silently ignored.

Sequence Diagram

sequenceDiagram
participant Operator
participant CLI as nodebuilder/rpc.ParseFlags
participant Config as nodebuilder/rpc.Config
participant Constructor as nodebuilder/rpc.server
participant Server as api/rpc.NewServer
participant Middleware as connLimit

Operator->>CLI: "--rpc.max-concurrent-conns=N"
CLI->>Config: "set MaxConcurrentConns when N > 0"
Config->>Config: "Validate MaxConcurrentConns > 0"
Config->>Constructor: pass cfg.MaxConcurrentConns
Constructor->>Server: NewServer(..., maxConcurrentConns)
Server->>Server: "fallback to DefaultMaxConcurrentConns if <= 0"
Server->>Middleware: connLimit(maxConcurrentConns, handler)
Middleware-->>Operator: 503 when concurrent slots are exhausted

Loading

Prompt To Fix All With AI

### Issue 1
nodebuilder/rpc/flags.go:207-211
**Reject negative flag values**
`--rpc.max-concurrent-conns=-1` is accepted by `pflag.GetInt`, but this branch ignores it because only `val > 0` is applied. With a valid config already loaded, startup continues using the old config value even though the operator supplied an invalid override; `Validate()` never sees the negative value. Returning an error here keeps CLI overrides from being silently misapplied.

```suggestion
	if val, err := cmd.Flags().GetInt(maxConcurrentConnsFlag); err != nil {
		return err
	} else if val < 0 {
		return fmt.Errorf("%s must be >= 0", maxConcurrentConnsFlag)
	} else if val > 0 {
		cfg.MaxConcurrentConns = val
	}
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (3): Last reviewed commit: "fix(nodebuilder/rpc): fail on invalid Ma..." | Re-trigger Greptile}

[mohsenm4]

@vgonkivs could you take a look at this?

[vgonkivs]

Thanks for the PR. Have a few comments before we proceed with it:

1. Please verify MaxConcurrentConn but dont mutate them(cfg.Validate() in rpc module). The node should fail out loud on startup in case MaxConcurrentConn <= 0
2. The PR contains a lot of AI-generated comments. Please keep comments short and only where a reader would ask why

[mohsenm4]

Both addressed in the amended commit:

1. Validate() now rejects MaxConcurrentConns <= 0 instead of backfilling. Error message points at celestia config update so upgrading operators know how to recover.

2. Trimmed the AI-flavored comments.

Release note: configs predating this field need celestia config update before startup.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.69%. Comparing base (2469e7a) to head (67fb4db).
⚠️ Report is 888 commits behind head on main.

Files with missing lines	Patch %	Lines
nodebuilder/rpc/flags.go	55.55%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5124      +/-   ##
==========================================
- Coverage   44.83%   37.69%   -7.15%     
==========================================
  Files         265      307      +42     
  Lines       14620    21373    +6753     
==========================================
+ Hits         6555     8056    +1501     
- Misses       7313    12304    +4991     
- Partials      752     1013     +261     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[greptile-apps]

Reject negative flag values
--rpc.max-concurrent-conns=-1 is accepted by pflag.GetInt, but this branch ignores it because only val > 0 is applied. With a valid config already loaded, startup continues using the old config value even though the operator supplied an invalid override; Validate() never sees the negative value. Returning an error here keeps CLI overrides from being silently misapplied.

Suggested change

	if val, err := cmd.Flags().GetInt(maxConcurrentConnsFlag); err != nil {
	return err
	} else if val > 0 {
	cfg.MaxConcurrentConns = val
	}
	if val, err := cmd.Flags().GetInt(maxConcurrentConnsFlag); err != nil {
	return err
	} else if val < 0 {
	return fmt.Errorf("%s must be >= 0", maxConcurrentConnsFlag)
	} else if val > 0 {
	cfg.MaxConcurrentConns = val
	}

Context Used: CLAUDE.md (source)

Artifacts

Repro: generated focused Go test source

• Evidence file captured while the check ran.

Repro: focused Go test output showing nil error and unchanged MaxConcurrentConns

• The full command output behind this check.

_{Ran code and verified through T-Rex}

Prompt To Fix With AI

This is a comment left during a code review.
Path: nodebuilder/rpc/flags.go
Line: 207-211

Comment:
**Reject negative flag values**
`--rpc.max-concurrent-conns=-1` is accepted by `pflag.GetInt`, but this branch ignores it because only `val > 0` is applied. With a valid config already loaded, startup continues using the old config value even though the operator supplied an invalid override; `Validate()` never sees the negative value. Returning an error here keeps CLI overrides from being silently misapplied.

```suggestion
	if val, err := cmd.Flags().GetInt(maxConcurrentConnsFlag); err != nil {
		return err
	} else if val < 0 {
		return fmt.Errorf("%s must be >= 0", maxConcurrentConnsFlag)
	} else if val > 0 {
		cfg.MaxConcurrentConns = val
	}
```

**Context Used:** CLAUDE.md ([source](https://app.greptile.com/celestia/github/celestiaorg/celestia-node/-/custom-context?memory=f5c7a6d3-9be6-4d68-83d6-efd8541b6403))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.