Skip to content

fix: implement more robust retries for Watch #535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
miparnisari merged 1 commit into from
Sep 12, 2025
Merged

fix: implement more robust retries for Watch #535

miparnisari merged 1 commit into from
Sep 12, 2025

Conversation

@miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Aug 26, 2025 •
edited
Loading

Description

Similar to authzed/authzed-java#137, I'm modifying the implementation of the zed relationships watch command so that

  1. we request checkpoints to keep the stream alive in case spicedb is behind a proxy that closes idle connection
  2. we add manual retries in case the error code is UNAVAILABLE

Testing

Locally against a locally running instance that is behind a Contour/Envoy reverse proxy.

  1. Tested the scenario of Watch API not returning anything (being idle) for at least 30 seconds:

before

 $ zed watch --log-level trace
Command "watch" is deprecated, deprecated; please use `zed watch relationships` instead
7:07PM DBG configured logging async=false format=auto log_level=trace provider=zerolog
starting watch stream over types [] and revision 
7:07PM TRC context={"APIToken":"......","CACert":null,"Endpoint":"maria-ps-6fc3f6.cloud.authzed.dev:443","Insecure":false,"Name":"us-east-1","NoVerifyCA":false} context-override-via-cli=false

(nothing happens)

7:08PM ERR retrying gRPC call error="rpc error: code = Unavailable desc = stream timeout" attempt=1

with this PR

./zed watch --log-level trace           
Command "watch" is deprecated, please use `zed relationships watch` instead
4:06PM DBG configured logging async=false format=auto log_level=trace provider=zerolog
starting watch stream over types [] and revision 
4:06PM TRC context={"APIToken":".....","CACert":null,"Endpoint":"maria-ps-6fc3f6.cloud.authzed.dev:443","Insecure":false,"Name":"us-east-1","NoVerifyCA":false} context-override-via-cli=false
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
CHECKPOINT REACHED
  1. Tested the scenario of Watch API continually returning data

References

@miparnisari miparnisari force-pushed the retry-watch branch 2 times, most recently from 07c540b to e3696bb Compare August 26, 2025 23:56
@miparnisari miparnisari force-pushed the retry-watch branch 5 times, most recently from 481580d to 979db72 Compare September 10, 2025 02:26
@miparnisari miparnisari marked this pull request as ready for review September 11, 2025 03:23
tstirrat15
tstirrat15 reviewed Sep 11, 2025
View reviewed changes
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments, otherwise LGTM

internal/commands/watch_test.go Outdated
}()
t.Cleanup(s.Stop)

conn, err := grpc.NewClient("passthrough://bufnet",
Copy link
Contributor

@tstirrat15 tstirrat15 Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

Copy link
Contributor Author

@miparnisari miparnisari Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is establishing a connection to my custom-made Watch server.

I see that other tests do it differently:

// Set up client
	ctx := t.Context()
	srv := zedtesting.NewTestServer(ctx, t)
	go func() {
		require.NoError(srv.Run(ctx))
	}()
	conn, err := srv.GRPCDialContext(ctx)
	require.NoError(err)

	originalClient := client.NewClient
	defer func() {
		client.NewClient = originalClient
	}()

	client.NewClient = zedtesting.ClientFromConn(conn)

	c, err := zedtesting.ClientFromConn(conn)(cmd)
	require.NoError(err)

but in my case i don't want to rely on a real SpiceDB server; I want to have full control on the response given by Watch

Copy link
Contributor

@tstirrat15 tstirrat15 Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah in the tests that I was working on I used a fake client and then supplied responses directly through it. Is there a reason to prefer one approach to the other in this case?

Copy link
Contributor Author

@miparnisari miparnisari Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@miparnisari miparnisari force-pushed the retry-watch branch 2 times, most recently from a440855 to 69f8f1e Compare September 11, 2025 20:21
tstirrat15
tstirrat15 approved these changes Sep 12, 2025
View reviewed changes
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM

@miparnisari miparnisari merged commit 525f8be into main Sep 12, 2025
11 checks passed
@miparnisari miparnisari deleted the retry-watch branch September 12, 2025 18:03
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tstirrat15 tstirrat15 tstirrat15 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@miparnisari @tstirrat15