fix(provider/client): forward service filter and tail to gateway#310
Open
gosuri wants to merge 4 commits into
Open
fix(provider/client): forward service filter and tail to gateway#310gosuri wants to merge 4 commits into
gosuri wants to merge 4 commits into
Conversation
- LeaseLogs: ?services= → ?service= (gateway reads singular).
- LeaseLogs: forward tailLines as ?tail=N (was dropped).
- LeaseEvents: forward services as ?service= (was dropped).
The interface in client.go:70-71 already named these parameters, but
the implementations bound them to `_` and silently dropped the values.
This makes the implementation honor the published contract.
Cross-verified against akash-network/provider main,
gateway/rest/middleware.go requestStreamParams: the gateway reads
vars.Get("service") and vars.Get("tail").
Adds httptest-based unit tests that pin the wire format (singular
service key, tail forwarding, no legacy plural services key) and
guard the omit-when-empty/zero branches.
closes akash-network/support#523
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFixes client websocket query encoding: LeaseEvents now includes the service filter when non-empty; LeaseLogs accepts tailLines and includes tail when >0. Tests and helpers added to capture and assert the gateway upgrade query parameters. ChangesLease Streaming Query Parameters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
go/provider/client/client_test.go (2)
99-103: ⚡ Quick winCapture only the first websocket query in the helper.
The helper contract says it records the first upgrade request, but it currently overwrites
qon each request. That can make retries/reconnects clobber the asserted query and introduce flakiness.Proposed refactor
func captureQueryServer(t *testing.T) (*httptest.Server, func() url.Values) { t.Helper() var ( mu sync.Mutex + once sync.Once q url.Values ) upgrader := websocket.Upgrader{ CheckOrigin: func(*http.Request) bool { return true }, } srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - mu.Lock() - q = r.URL.Query() - mu.Unlock() + once.Do(func() { + mu.Lock() + q = r.URL.Query() + mu.Unlock() + }) conn, err := upgrader.Upgrade(w, r, nil) if err != nil { return } _ = conn.Close() })) return srv, func() url.Values { mu.Lock() defer mu.Unlock() - return q + out := make(url.Values, len(q)) + for k, v := range q { + out[k] = append([]string(nil), v...) + } + return out } }Also applies to: 110-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/provider/client/client_test.go` around lines 99 - 103, The helper's HTTP handler currently overwrites the recorded websocket query `q` on every request, causing flaky tests; change the handler used with `httptest.NewTLSServer` (the closure that reads `r.URL.Query()`) to record only the first upgrade request by guarding the assignment with a check (e.g., use a sync.Once or only assign if `len(q) == 0`) while still protecting with `mu.Lock()`/`mu.Unlock()`; apply the same single-capture change to the second similar handler referenced around the 110-114 region so retries/reconnects don't clobber the originally asserted query.
154-154: ⚡ Quick winAvoid unbounded waits on stream close channels in tests.
Direct receives on
OnClosecan hang the suite indefinitely on regressions. Use a bounded wait helper with timeout for deterministic failure.Proposed refactor
import ( "context" "crypto/tls" "errors" "net/http" "net/http/httptest" "net/url" "sync" "testing" + "time" @@ ) + +func waitOnClose(t *testing.T, ch <-chan struct{}) { + t.Helper() + select { + case <-ch: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for stream close") + } +} @@ - <-out.OnClose // wait for reader goroutine to drain after server close + waitOnClose(t, out.OnClose) // wait for reader goroutine to drain after server close @@ - <-out.OnClose + waitOnClose(t, out.OnClose) @@ - <-out.OnClose + waitOnClose(t, out.OnClose) @@ - <-out.OnClose + waitOnClose(t, out.OnClose)Also applies to: 176-176, 195-195, 213-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/provider/client/client_test.go` at line 154, The test currently does a direct receive from the stream close channel (<-out.OnClose) which can hang; replace these unbounded receives in client_test.go (instances referencing out.OnClose at the noted locations) with a bounded wait helper that times out and fails the test (e.g., use a helper like waitForClose(t, out.OnClose, timeout) or require.Eventually with a short timeout) so the test deterministically fails on regressions instead of hanging; add the helper near the tests and use it at each occurrence (lines referencing out.OnClose) to assert the channel closed within the timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go/provider/client/client_test.go`:
- Around line 215-217: The test at getQuery()/q.Get("service") only asserts
service is empty; strengthen it by also asserting that the follow parameter is
explicitly "false" and that the "services" parameter is omitted/empty for
symmetry with other query-forwarding tests — add assertions like
require.Equal(t, "false", q.Get("follow")) and require.Empty(t,
q.Get("services")) alongside the existing require.Empty(t, q.Get("service")) in
the LeaseEvents-related test.
---
Nitpick comments:
In `@go/provider/client/client_test.go`:
- Around line 99-103: The helper's HTTP handler currently overwrites the
recorded websocket query `q` on every request, causing flaky tests; change the
handler used with `httptest.NewTLSServer` (the closure that reads
`r.URL.Query()`) to record only the first upgrade request by guarding the
assignment with a check (e.g., use a sync.Once or only assign if `len(q) == 0`)
while still protecting with `mu.Lock()`/`mu.Unlock()`; apply the same
single-capture change to the second similar handler referenced around the
110-114 region so retries/reconnects don't clobber the originally asserted
query.
- Line 154: The test currently does a direct receive from the stream close
channel (<-out.OnClose) which can hang; replace these unbounded receives in
client_test.go (instances referencing out.OnClose at the noted locations) with a
bounded wait helper that times out and fails the test (e.g., use a helper like
waitForClose(t, out.OnClose, timeout) or require.Eventually with a short
timeout) so the test deterministically fails on regressions instead of hanging;
add the helper near the tests and use it at each occurrence (lines referencing
out.OnClose) to assert the channel closed within the timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a594591-f9e0-4708-9bc4-e113e2b2f3d2
📒 Files selected for processing (2)
go/provider/client/client.gogo/provider/client/client_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
chalabi2
approved these changes
May 29, 2026
Contributor
chalabi2
left a comment
chalabi2
left a comment
There was a problem hiding this comment.
Gateway-side verified, mocks unaffected, callers unchanged, tests pass. The tailLines > 0 guard is fine, --tail 0 is pathological and the default-coercion is a reasonable choice. If you want to be strict about user-intent preservation you can flip to >= 0, but I don't think it matters in practice.
GHA down but local review LGTM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Description
Three wire-level bugs in the provider gateway websocket client meant every consumer's
--service/--tailflag was silently dropped. The interface inclient.go:70-71advertisesservicesandtailLines, but the implementations bind both to_and never write them to the query string. This PR makes the implementation honor the published interface contract.LeaseLogs:?services=→?service=(gateway uses singular).LeaseLogs: forwardtailLinesas?tail=N(was dropped).LeaseEvents: forwardservicesas?service=(was dropped).Cross-verified against
akash-network/providermain,gateway/rest/middleware.gorequestStreamParams: the gateway readsvars.Get("service")andvars.Get("tail").🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
client.go:70-71already named these parameters. Today's_placeholders are an implementation drift from the published interface — not a behavior callers can be relying on.tailLinesguard. Forwarded only when> 0. The gateway acceptstail >= -1(default-1), so we treat0/negative as "leave gateway default". Open to flipping to>= 0if reviewers prefer.httptestusers in this package — seecaptureQueryServer+newTestClienthelpers inclient_test.go. The helpers spin uphttptest.NewTLSServerwith awebsocket.Upgraderand capture the query string at upgrade time; same-package access lets the helper replacec.tlsCfgdirectly so no production API surface is added for the test.