Hey, thanks for trying to fix this, but I'm not sure this is the right fix. We should attempt to write out pending data from a client that has been marked as close asap, since there are certain places we do it intentionally. I'm also wondering what RedisGraph is doing, since the SIGPIPE signal should be ignored. You might want to follow up with them:

ref:

This is indeed odd that you're getting that signal although suppose to be ignored.
@jeffreylovitz is there any chance RedisGraph is doing something that overrides that?
@yossigo maybe you have some other idea why this can happen?

Regarding the fix, there's actually already a PR with a similar fix and a similar title: #7202 (attempting to fix a different issue), it looks like Salvatore was actually ok with this specific change, but not about another one in that other PR.

@madolson i suppose you're referring to clientAcceptHandler, but there we're writing before setting the flag, and anyway, we're writing directly to the socket, not using the write handler.
is there anything i'm missing?

Ohh sorry, i now realize it's not the same change as in the other PR (it's the same as the one Salvatore suggested instead of one of the changes there).

@oranagra no idea, unless it gets overridden elsewhere but it doesn't seem so.

@oranagra We override the SIGUSR signals and, on some versions, inject our own logs upon receiving the SIGSEGV signal before calling the Redis handler. Other than that, RedisGraph shouldn't interfere at all.

@jeffreylovitz I can confirm creating similar situations where there's a lot of buffered output and a client drops, connSocketWrite() does get an EPIPE response as expected (i.e. SIGPIPE is ignored). Something in RedisGraph must be messing up with the handler...

@yossigo To get more context, I added an explicit EPIPE signal handler to RedisGraph, and it fails to catch the signal or have any impact on this crash.

@oranagra I was actually thinking about ACLs, but thinking about it more this PR looks correct, since we have another functionality to achieve the mechanism I was considering.

@madolson i guess you meant #7202 right?

@jeffreylovitz I think your change should now pass the tests since we fixed the underlying issue, would you mind doing an empty push to your branch to kickoff the CI again?

This was actually already resolved in #7202 (has exactly this change and more. I forgot to close this one).
@madolson FYI, there's a button that can rerun a failed CI, but in this case a rebase was required (not an empty push)