Flush propagation list in active-expire of writable replicas to fix an assertion

[guybe7]

Make sure to flush also_propagate after expiring from expireSlaveKeys to avoid an assertion in beforeSleep.

Note that this is an edge case that only happens in case volatile keys were created directly on a writable replica, and that anyway nothing is propagated to sub-replicas.

For Redis 7.2, We need to honor the post-execution-unit API and call it after each KSN.

[oranagra]

@guybe7 unless i'm missing something, the top comment is misleading.
sub-replicas don't really propagate anything to their replicas (it'll mess up the replication offset).

see:

void replicationFeedSlaves(list *slaves, int dictid, robj **argv, int argc) {

    /* If the instance is not a top level master, return ASAP: we'll just proxy
     * the stream of data we receive from our master instead, in order to
     * propagate *identical* replication stream. In this way this slave can
     * advertise the same replication ID as the master (since it shares the
     * master replication history and has the same backlog and offsets). */
    if (server.masterhost != NULL) return;

so this fix is needed for the sake of modules maybe, but not for replication.
please ack.

[guybe7]

@oranagra so when a writable replica actively expires a key it doesn't send that DEL? but it does emit a KSN? This could be problematic for modules, the sub-replica will not receive what the module propagated from within the PEJ (post execution-unit job)

maybe we want to keep the code as-is and just document somewhere that PEJ doesn't work in case of writable-replica active-expire? or better to leave the code and just document that in this case nothing is propagated?

@MeirShpilraien ^

[oranagra]

i think we rather keep honoring the module API (current fix in the PR), since i don't know what the module will do.

writable replicas, and specifically ones with local expiry are a mess anyway, i hope the use cases for that don't involve modules, and if they do, the module should probably be explicitly designed for that.

[guybe7]

@oranagra updated the top comment

[oranagra]

marking this to be backported for 7.0 with a twist, see #11778 (comment)