Skip to content

Fix scanning wrong dict entry in activeExpireCycle#8089

Closed
ShooterIT wants to merge 1 commit intoredis:unstablefrom
ShooterIT:expire-dict-fix
Closed

Fix scanning wrong dict entry in activeExpireCycle#8089
ShooterIT wants to merge 1 commit intoredis:unstablefrom
ShooterIT:expire-dict-fix

Conversation

@ShooterIT
Copy link
Member

Fix #8087

@oranagra
Copy link
Member

@ShooterIT i'm not sure this fix is wise, or at all needed.
first, going back to re-scan the entire bucket after each successful expiration doesn't seems like a good idea.
secondly, i'm not sure anyone meant for a module to mess with the keyspace from inside the keyspace notification hook.

@guybe7 @MeirShpilraien WDYT?

@ShooterIT
Copy link
Member Author

@oranagra Yes, re-scanning the entire bucket is worth to discuss. I am not sure it's right. I worry about we may not scan next dict entries that have expired key.

I don't sure users' intentions, but i think redis shouldn't core dump because of this thing. We can describe this problem to avoid it if we do nothing.

@oranagra
Copy link
Member

@ShooterIT they way i look at it, it's not redis that core dumps, it's the module.
i.e. as soon as there's a module loaded, any crash or memory smear could be it's fault.
in this case the module is doing something we didn't expect or intend for it to do and is not valid, and that sometimes leads redis to crash. but i'm not sure we can document all the things we don't allow.

i think it's clear that from within a module command, a module can open any key it wants, but interacting with other keys from within callbacks is risky business.

for example, i've seen modules open keys, keep the pointer to the data and use it at various points in time, without re-opening the key. they can usually get away unpunished because redis can't really do anything to that pointer without the module's involvement, for instance on free. but saying we support this kind of thing would be obviously wrong.

so what i'm saying is that some modules authors read the redis code and conclude they can do things we didn't mean for them to do, it may work for them, until the day redis internals change and it stops working. i think we wanna let them keep that freedom, but then anything bad that happens is their responsibility.

the only thing we can do about it, is maybe avoid passing a working context to a callback, in which case they won't be able to open key or do certain actions at all (even if they read the code and conclude it's safe at that point).
i rather let them keep the freedom to do funny things, but they should be responsible for the implications.

@yossigo @guybe7 anything to add or contradict?

@yossigo
Copy link
Collaborator

yossigo commented Dec 8, 2020

@oranagra I think that ideally the API should reflect the real contract and limit interactions that violate this contract wherever it is possible, at least for naive implementations. This is also why I was not happy with exposing a full context in some hooks.

I guess what we can do here is add some dirty flag to indicate we need to re-scan, but I suspect this is just one special case out of many others.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2024

CLA assistant check
All committers have signed the CLA.

@ShooterIT ShooterIT closed this Jan 6, 2025
@ShooterIT ShooterIT deleted the expire-dict-fix branch January 6, 2025 02:55
alonre24 added a commit to alonre24/redis that referenced this pull request Jan 26, 2026
**Bug Fixes:**

* [redis#7385](RediSearch/RediSearch#7385) Fix high temporary memory consumption when loading multiple search indexes from RDB
* [redis#7430](RediSearch/RediSearch#7430) Fix a potential deadlock in `FT.HYBRID` in cluster mode during updates.
* [redis#7454](RediSearch/RediSearch#7454) Fix a garbage collection performence regression
* [redis#7460](RediSearch/RediSearch#7460) Fix potential double-free in Fork GC error paths
* [redis#7455](RediSearch/RediSearch#7455) Fix internal cursors not being deleted promptly in cluster mode
* [redis#7667](RediSearch/RediSearch#7667) Fix a cursor logical leak upon dropping the index
* [redis#7796](RediSearch/RediSearch#7796) Fix a potential use-after-free when removing connections
* [redis#7792](RediSearch/RediSearch#7792) Fix string comparison for binary data with embedded NULLs in TOLIST reducer in FT.AGGREGATE
* [redis#7823](RediSearch/RediSearch#7823) Update `FT.HYBRID` to accept vector blobs only via parameters
* [redis#7903](RediSearch/RediSearch#7903) Fix a memory leak in Hybrid ASM
* [redis#8052](RediSearch/RediSearch#8052) Fix `FT.HYBRID` behavior when used with `LOAD *`
* [redis#8082](RediSearch/RediSearch#8082) Fix incorrect FULLTEXT field metric counts
* [redis#8089](RediSearch/RediSearch#8089) Fix an edge case in `CLUSTERSET` handling
* [redis#8152](RediSearch/RediSearch#8152) Fix configuration registration issues

**Improvements:**

* [redis#7427](RediSearch/RediSearch#7427) Enhance `FT.PROFILE` with vector search execution details
* [redis#7431](RediSearch/RediSearch#7431) Ensure full `FT.PROFILE` output is returned on timeout with RETURN policy
* [redis#7507](RediSearch/RediSearch#7507) Track timeout warnings and errors in INFO
* [redis#7576](RediSearch/RediSearch#7576) Track OOM warnings and errors in INFO
* [redis#7612](RediSearch/RediSearch#7612) Track `maxprefixexpansions` warnings and errors in INFO
* [redis#7960](RediSearch/RediSearch#7960) Persist query warnings across cursor reads
* [redis#7551](RediSearch/RediSearch#7551), [redis#7616](RediSearch/RediSearch#7616), [redis#7622](RediSearch/RediSearch#7622), [redis#7625](RediSearch/RediSearch#7625) Add runtime thread and pending-jobs metrics
* [redis#7589](RediSearch/RediSearch#7589) Support multiple slot ranges in `search.CLUSTERSET`
* [redis#7707](RediSearch/RediSearch#7707) Add `WITHCOUNT` support to `FT.AGGREGATE`
* [redis#7862](RediSearch/RediSearch#7862) Add support for subquery `COUNT` in `FT.HYBRID`
* [redis#8087](RediSearch/RediSearch#8087) Add warnings when cursor results may be affected by ASM and expose ASM warnings in `FT.PROFILE`
* [redis#8049](RediSearch/RediSearch#8049) Add logging for index-related commands
* [redis#8150](RediSearch/RediSearch#8150) Fix shard total profile time reporting in `FT.PROFILE`
YaacovHazan pushed a commit that referenced this pull request Jan 26, 2026
**Bug Fixes:**

* [#7385](RediSearch/RediSearch#7385) Fix high
temporary memory consumption when loading multiple search indexes from
RDB
* [#7430](RediSearch/RediSearch#7430) Fix a
potential deadlock in `FT.HYBRID` in cluster mode during updates.
* [#7454](RediSearch/RediSearch#7454) Fix a
garbage collection performence regression
* [#7460](RediSearch/RediSearch#7460) Fix
potential double-free in Fork GC error paths
* [#7455](RediSearch/RediSearch#7455) Fix
internal cursors not being deleted promptly in cluster mode
* [#7667](RediSearch/RediSearch#7667) Fix a
cursor logical leak upon dropping the index
* [#7796](RediSearch/RediSearch#7796) Fix a
potential use-after-free when removing connections
* [#7792](RediSearch/RediSearch#7792) Fix string
comparison for binary data with embedded NULLs in TOLIST reducer in
FT.AGGREGATE
* [#7704](RediSearch/RediSearch#7704) Use
asynchronous jobs for GC in SVS to accelerate execution
* [#7823](RediSearch/RediSearch#7823) Update
`FT.HYBRID` to accept vector blobs only via parameters
* [#7903](RediSearch/RediSearch#7903) Fix a
memory leak in Hybrid ASM
* [#8052](RediSearch/RediSearch#8052) Fix
`FT.HYBRID` behavior when used with `LOAD *`
* [#8082](RediSearch/RediSearch#8082) Fix
incorrect FULLTEXT field metric counts
* [#8089](RediSearch/RediSearch#8089) Fix an
edge case in `CLUSTERSET` handling
* [#8152](RediSearch/RediSearch#8152) Fix
configuration registration issues

**Improvements:**

* [#7427](RediSearch/RediSearch#7427) Enhance
`FT.PROFILE` with vector search execution details
* [#7431](RediSearch/RediSearch#7431) Ensure
full `FT.PROFILE` output is returned on timeout with RETURN policy
* [#7507](RediSearch/RediSearch#7507) Track
timeout warnings and errors in INFO
* [#7576](RediSearch/RediSearch#7576) Track OOM
warnings and errors in INFO
* [#7612](RediSearch/RediSearch#7612) Track
`maxprefixexpansions` warnings and errors in INFO
* [#7960](RediSearch/RediSearch#7960) Persist
query warnings across cursor reads
* [#7551](RediSearch/RediSearch#7551),
[#7616](RediSearch/RediSearch#7616),
[#7622](RediSearch/RediSearch#7622),
[#7625](RediSearch/RediSearch#7625) Add runtime
thread and pending-jobs metrics
* [#7589](RediSearch/RediSearch#7589) Support
multiple slot ranges in `search.CLUSTERSET`
* [#7707](RediSearch/RediSearch#7707) Add
`WITHCOUNT` support to `FT.AGGREGATE`
* [#7862](RediSearch/RediSearch#7862) Add
support for subquery `COUNT` in `FT.HYBRID`
* [#8087](RediSearch/RediSearch#8087) Add
warnings when cursor results may be affected by ASM and expose ASM
warnings in `FT.PROFILE`
* [#8049](RediSearch/RediSearch#8049) Add
logging for index-related commands
* [#8150](RediSearch/RediSearch#8150) Fix shard
total profile time reporting in `FT.PROFILE`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CRASH] activeExpireCycle crash

4 participants