Repository navigation
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit ac7ebfb. Configure here.
|
|
||
| /* Create entry for lookup and potential insertion */ | ||
| entry = idmpEntryCreate(iid_str, iid_len, &s->alloc_size); | ||
| entry->recorded_ms = server.mstime; |
There was a problem hiding this comment.
IDMP timestamp not replicated
High Severity
XADD with IDMP stores recorded_ms from the local clock and never propagates that timestamp. Replicas apply the same command later and stamp a later time, so IDMP expiry diverges by replication lag. After the primary drops a mapping, a retry of the same IID can append a new stream entry on the primary while the replica still treats it as a duplicate and skips it, silently diverging stream contents. RDB, AOF rewrite, and COPY preserve recorded_ms, but live replication does not.
Reviewed by Cursor Bugbot for commit ac7ebfb. Configure here.
| while (producer->idmp_head != NULL) { | ||
| idmpEntry *entry = producer->idmp_head; | ||
| if (entry->id.ms <= expire_time) { | ||
| if (entry->recorded_ms <= expire_time) { |
There was a problem hiding this comment.
IDMP expiry assumes sorted timestamps
Medium Severity
Expiry and RDB filtering still only drop a prefix of the IDMP linked list and assume recorded_ms never decreases along it. Inserts always append, while recorded_ms is wall-clock time and XIDMPRECORD can supply an older stamp. A later entry with an earlier recorded_ms is then skipped until the head expires, so mappings can outlive IDMP-DURATION. Stream IDs used to keep this list ordered even when the clock stepped backward; recorded_ms does not.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ac7ebfb. Configure here.
Summary
IDMP expiration currently derives entry age from the stream ID millisecond component. An explicit future-dated stream ID can therefore retain the IDMP mapping indefinitely.
This change stores a separate insertion timestamp for each IDMP entry and uses it for expiration and persistence filtering. The timestamp is preserved across RDB loading, AOF rewrite/replay, and stream COPY.
Testing
The patch is based on current unstable.
Note
Medium Risk
Changes stream IDMP TTL semantics and on-disk/AOF formats (new RDB type and extended XIDMPRECORD); backward-compatible load paths exist but persistence and replication behavior shift for future-dated IDs.
Overview
IDMP entries now expire by insertion time, not by the stream ID’s millisecond field, so future-dated message IDs can’t keep dedup mappings alive past
IDMP-DURATION.Each
idmpEntrygainsrecorded_ms, set onXADDwith IDMP toserver.mstimeand copied on stream duplication. Expiration, RDB save filtering, and background cleanup all comparerecorded_msto the TTL window instead ofid.ms.Persistence: new RDB stream type
STREAM_LISTPACKS_6storesrecorded_msper IDMP entry; older stream RDB types load without it (defaulting to current time). AOF rewrite emits an optional sixth argument on internalXIDMPRECORDso replay restores the timestamp. Command metadata moves to arity-5with optionalrecorded-ms.A Tcl test asserts that a future-dated stream ID still lets IDMP state expire after the configured duration.
Reviewed by Cursor Bugbot for commit ac7ebfb. Bugbot is set up for automated code reviews on this repo. Configure here.