fix: canceled should always be set to true when cancel a watch request

[nic-6443]

Anytime the server attempts to cancel a watch request, it should set the canceled field to true, regardless of whether err is empty or not. Let err only affect the reason field.

[brandond]

Thanks! Can you provide any information on what issue this is causing so that we can validate the fix?

[nic-6443]

Thanks! Can you provide any information on what issue this is causing so that we can validate the fix?

Sure, the problem I encountered is as follows: I used kine as a replacement for etcd in the https://github.com/apache/apisix project, using postgres as the backend database. When performing maintenance operations on postgres, it needs to be restarted. If kine happens to receive a watch request at this time, this request will be canceled by kine due to SQL execution failure:

kine/pkg/logstructured/logstructured.go

Line 430 in 73df6c7

rev, kvs, err := l.log.After(ctx, prefix, revision, 0)

error log:

time="2024-09-24T20:20:23Z" level=error msg="Failed to list / for revision 3095: FATAL: terminating connection due to administrator command (SQLSTATE 57P01)"

Although kine cancels this request, it does not set the canceled field to true, causing abnormal client behavior. The response returned by kine is:

{"result":{"header":{},"watch_id":"1"}}

I have checked the native etcd code and found that when canceling a watch request, they always set canceled to true. I think we should stay consistent with etcd.

[brandond]

That is interesting. This suggests that the error is not being propagated up from the failed list call, since err != nil is false and the cancelReason is empty. Are you able to run kine with --debug and find the output of the WATCH CANCEL id= log when this happens?

[nic-6443]

That is interesting. This suggests that the error is not being propagated up from the failed list call, since err != nil is false and the cancelReason is empty. Are you able to run kine with --debug and find the output of the WATCH CANCEL id= log when this happens?

this is the log:

TRAC[0022] WATCH CANCEL id=1, reason=, compactRev=0

I have considered whether to return the cause of the error to the client, such as FATAL: terminating connection due to administrator command (SQLSTATE 57P01), but considering that this is internal server information, it seems inappropriate to return it to the client. If you think it is necessary to pass this error message through to the client, I can consider recording the error in server.WatchResult and returning it in the cancel response.

[brandond]

I don't know if we need to expose the raw error message from the sql client to the etcd client, but it does seem correct to return AN error.

[nic-6443]

Understood, then I will return a generic database error message.

[nic-6443]

@brandond send a error message to client now, since this error only occurs when there is an exception in the database, it is difficult to write automated tests.
Therefore, I verified that the change works locally by injecting errors into the query function.

❯ curl https://localhost:7943/v3/watch --cert /tmp/tls.crt --key /tmp/tls.key -k -X POST --data '
{"create_request":{"key":"L2FwaXNpeC8=","start_revision":61,"range_end":"L2FwaXNpeDA="}}
'
{"result":{"header":{},"watch_id":"1","created":true}}
{"result":{"header":{},"watch_id":"1","canceled":true,"cancel_reason":"failed to execute query in database"}}

[brandond]

lgtm, one nit!

[brandond]

nit: Add ErrWatchCancelled = rpctypes.ErrGRPCWatchCanceled in pkg/server/types.go and just return that here as a generic watch failure message. We try to return the same errors as upstream whenever possible, even if the cause isn't necessarily the same.

If you can think of a better error from https://pkg.go.dev/go.etcd.io/etcd/api/v3/v3rpc/rpctypes to use please feel free.

[nic-6443]

oh, great, I think ErrGRPCUnhealthy is better to express the failure come from system level.

{"result":{"header":{},"watch_id":"1","created":true}}
{"result":{"header":{},"watch_id":"1","canceled":true,"cancel_reason":"rpc error: code = Unavailable desc = etcdserver: unhealthy cluster"}}