Repository: valkey-io/valkey
File: src/t_hash.c (C)

Code before:
        server.stat_expiredfields++;
        entryFree(entry);
        return 1;
    }
    return 0;
}

/* Extract expired entries from a hash object's volatile set.
 * Returns number of expired entries, populates `out_entries`. */
size_t hashTypeDeleteExpiredFields(robj *o, mstime_t now, unsigned long max_fields, robj **out_entries) {
    serverAssert(o->encoding == OBJ_ENCODING_HASHTABLE);

    /* skip TTL checks temporarily (to allow hashtable lookup) */
    hashTypeIgnoreTTL(o, 1);

    vset *vset = hashTypeGetVolatileSet(o);
    if (!vset || vsetIsEmpty(vset)) {
        hashTypeIgnoreTTL(o, 0);
        return 0;
    }

    expiryContext ctx = {.key = o, .fields = out_entries, .n_fields = 0};
    size_t expired = vsetRemoveExpired(vset, entryGetExpiry, hashTypeExpireEntry, now, max_fields, &ctx);
    serverAssert(ctx.n_fields <= max_fields);
    hashTypeIgnoreTTL(o, 0);
    if (!hashTypeHasVolatileFields(o)) {
        hashTypeFreeVolatileSet(o);
    }
    return expired;
}

/* Hashtable scan callback for hash datatype */
static void defragHashTypeEntry(void *privdata, void *element_ref) {
    entry **entry_ref = (entry **)element_ref;
    entry *old_entry = *entry_ref;

    entry *new_entry = entryDefrag(old_entry, activeDefragAlloc, activeDefragSds);
    if (new_entry) {
        long long expiry = entryGetExpiry(new_entry);
        /* In case the entry is tracked we need to update it in the volatile set */
        if (expiry != EXPIRY_NONE) {
            // We don't need to pass the db because db-level tracking isn't going to change for this update.
            hashTypeTrackUpdateEntry(privdata, old_entry, new_entry, expiry, expiry);
        }
        *entry_ref = new_entry;
    }
}

size_t hashTypeScanDefrag(robj *ob, size_t cursor, void *(*defragAllocfn)(void *)) {
    if (ob->encoding == OBJ_ENCODING_LISTPACK) {
        unsigned char *newzl;

Review comment (bug):
The logic looks correct. The bug is that we didn't free the empty volatile set after removing all the expired fields, right?

I can see in the source code of `hashTypeFreeVolatileSet()` that it implies `hashTypeIgnoreTTL(o, true)`, but the naming and documentation of these functions don't indicate that it's OK to skip restoring `hashTypeIgnoreTTL` afterwards if the volatile set is freed. It looks unbalanced to me, as if leaving it as `true` here sets a flag that keeps TTL ignored forever even after adding more volatile fields. Ideally this should be obvious from the function names alone.

Code after:
        return 1;
    }
    return 0;
}

/* Extract expired entries from a hash object's volatile set.
 * Returns number of expired entries, populates `out_entries`. */
size_t hashTypeDeleteExpiredFields(robj *o, mstime_t now, unsigned long max_fields, robj **out_entries) {
    serverAssert(o->encoding == OBJ_ENCODING_HASHTABLE);

    vset *vset = hashTypeGetVolatileSet(o);
    if (!vset) {
        return 0;
    }

    serverAssert(!vsetIsEmpty(vset));
    /* skip TTL checks temporarily (to allow hashtable pops) */
    hashTypeIgnoreTTL(o, true);
    expiryContext ctx = {.key = o, .fields = out_entries, .n_fields = 0};
    size_t expired = vsetRemoveExpired(vset, entryGetExpiry, hashTypeExpireEntry, now, max_fields, &ctx);
    serverAssert(ctx.n_fields <= max_fields);
    if (vsetIsEmpty(vset)) {
        hashTypeFreeVolatileSet(o);
    } else {
        hashTypeIgnoreTTL(o, false);
    }
    return expired;
}

/* Hashtable scan callback for hash datatype */
static void defragHashTypeEntry(void *privdata, void *element_ref) {
    entry **entry_ref = (entry **)element_ref;
    entry *old_entry = *entry_ref;

    entry *new_entry = entryDefrag(old_entry, activeDefragAlloc, activeDefragSds);
    if (new_entry) {
        long long expiry = entryGetExpiry(new_entry);
        /* In case the entry is tracked we need to update it in the volatile set */
        if (expiry != EXPIRY_NONE) {
            // We don't need to pass the db because db-level tracking isn't going to change for this update.
            hashTypeTrackUpdateEntry(privdata, old_entry, new_entry, expiry, expiry);
        }
        *entry_ref = new_entry;
    }
}

size_t hashTypeScanDefrag(robj *ob, size_t cursor, void *(*defragAllocfn)(void *)) {
    if (ob->encoding == OBJ_ENCODING_LISTPACK) {
        unsigned char *newzl;
        if ((newzl = activeDefragAlloc(ob->ptr))) ob->ptr = newzl;
        return 0;