There are few things that i'm uncomfortable with:

First, removing the may-replicate flag may go unnoticed, nothing will seem to break (no test).
at first, when i was looking at this, for a moment i thought maybe there's a serious bug and all the read commands need that flag, since they all call expireIfNeeded, but i later remembered that expireIsNeeded has an explicit check for write pause.

So in fact the reason we're setting the flag here is not because this command may replicate (unlike EVAL and PUBLISH), but because we wanna make sure that when it'll be called, it'll never skip deleting an expired key.
I.e. this call for expireIfNeeded is different than the others, in the other places we're interested in the return value of the function, and don't care much about the fact that it deletes the key or not.
But here, we don't care about the return value, we only care about the deletion.

Maybe some of this concern can be solved by separating isKeyExpired and expireIfNeeded (one returns a value, and the other returns void).
But as noted in the opening comment (about replicas), it may be problematic to rely on the fact the key is deleted on time, maybe instead we need to create some other mechanism to avoid failing EXEC if the key logical state didn't change between WATCH and EXEC?

p.s. correct me if i'm wrong. regarding WATCH on replicas, this PR doesn't create any new issue with that combination, it's just that the problem it come to fix isn't fixed on replicas, right?

suggested course of action:

1. rename expireIsNeeded which is misleading to IsKeyExpired (or isKeyExpiredAndMaybeDeleteIt 😄)
2. WATCH can call that function on the keys, and record the result (one bit) somewhere.
3. when the key is deleted either by lazy-expire, active-expire, or the expiration check in EXEC, if that bit is set, we don't flag the transaction as dirty.

this way we know if the key changed logical state between the watch and exec.

note that a similar issue this PR aims to fix (wrongly failing EXEC) already existed (before the recent change in #9194) for active expire. i.e. consider the following test:

    test {WATCH stale keys should not fail EXEC} {
        r flushdb
        r set x foo px 1
        wait_for_condition {[r dbsize] == 0} else { fail ...}
        r watch x
        r multi
        r ping
        assert_equal {PONG} [r exec]
    } {OK} {needs:debug}

1. rename expireIsNeeded which is misleading to IsKeyExpired (or isKeyExpiredAndMaybeDeleteIt 😄)
2. WATCH can call that function on the keys, and record the result (one bit) somewhere.
3. when the key is deleted either by lazy-expire, active-expire, or the expiration check in EXEC, if that bit is set, we don't flag the transaction as dirty.

Yes, I know this can work, and actually at first I wanna implement it, but it's a bit complex(not only the EXPIRED flag but also need modify touchWatchedKey function and calls to check if it is a stale deletion) and it's just a corner case, and seems we are near the next release, so I wanna fix it in an easy way.

If you all think we need a new WATCH mechanism, we can just do it.

i don't see an urgency to fix it. it existed since forever, and the only thing recently changed is just to prevent a race condition (increases the chances the transaction will fail, but the chance existed before), and the outcome of the problem this PR comes to fix is not severe (failing a transaction, something an application is suppose to handle).
i rather take our time and solve it properly.

ok, I will try to implement the new mechanism.

BTW, since this PR is still WIP, I just reopen issue #9068

1. rename expireIsNeeded which is misleading to IsKeyExpired (or isKeyExpiredAndMaybeDeleteIt 😄)
2. WATCH can call that function on the keys, and record the result (one bit) somewhere.
3. when the key is deleted either by lazy-expire, active-expire, or the expiration check in EXEC, if that bit is set, we don't flag the transaction as dirty.

I'm trying to redesign, but meet some problem that I didn't figure it out.

A problem is about replication, for example about point 3, it works well on master, but expiration doesn't work on replica, replica depends on master's replication stream to delete stale data (DEL or UNLINK command), so how to handle stale data on replica (especially the replication may delay)?

I'd love to have your suggestions @oranagra @zuiderkwast

trying to follow, i need some recap:

1. the proper use case for WATCH is: WATCH x + GET x + MULTI + SET x + EXEC.
2. but since we're discussing a (read-only) replica, maybe our (a bit odd) scenario is WATCH x + GET x + MULTI + GET y + EXEC
3. however, the case in 1 doesn't need this PR to be fixed, so this PR is discussing a case with no GET after WATCH, like WATCH x + MULTI + SET x + EXEC.
4. so for a replica are we discussing WATCH x + MULTI + GET y + EXEC?

now when i actually think of case 2 (which wasn't the case for this PR till now in my understanding) in a replica, since GET wouldn't expire the key, it'll just return nil, we do indeed have a problem.
so @soloestoy is that what you where talking about?
so you want the DEL the master sends, to avoid invalidating the WATCH since the client already viewed it as expired (got nil without deleting the key).

I think the right way to solve it is that the DEL command (from a master client) deletes a logically expired key, we avoid invalidating the WATCH (if that cached bit i described in my 1,2,3 was set).
i.e. in masters when DEL command arrives on a logically expired key, the expireIfNeeded check will delete it before the DEL command does, and that will be reflected in the DEL command response (and keyspace notification).
in the replica, that DEL proceeds and does the actual deletion, that's what we wanna change.

so that means:
delGenericCommand needs needs to look at the return value of the new isKeyExpiredAndMaybeDeleteIt, and if the client is the master client, call the same logic that active/lazy expire is using?
or alternatively it can pass a flag to isKeyExpiredAndMaybeDeleteIt telling it that it's a DEL request (so it should behave differently than how it normally behaves for other commands that are received from the master).

now you'll probably argue that there's a problem with the above suggestion since it'll have a false positive in case the user sent a DEL command to the master before the key expired, and it arrived to the replica after it was logically expired, and the replica will still execute the expiration path.
As far as i can tell, it doesn't matter for WATCH and EXEC. am i right?
it does however matter if we also wanna solve keyspcae notifications and module hooks better, but if we wanna do that, the only way i can see is add a new new command which will let the master send the reason for DEL (i.e propagate a different DEL for expiration, eviction, and anything else)... maybe that's ok too.

closing in favor of #10256