dragonfly icon indicating copy to clipboard operation
dragonfly copied to clipboard

fix(search): remove invalid DCHECK in FT.INFO for concurrent index operations

Open vyavdoshenko opened this issue 1 month ago • 3 comments

Fixes: https://github.com/dragonflydb/dragonfly/issues/6181

  • Fix crash in FT.INFO when called concurrently with FT.CREATE/FT.DROPINDEX
  • Remove invalid DCHECK assertion that assumed index state is always consistent across shards

Details: The DCHECK(num_notfound == 0u || num_notfound == shard_set->size()) assertion in FtInfo() was incorrect because FT.INFO doesn't use a global transaction, so it can observe partial index state during concurrent FT.CREATE or FT.DROPINDEX operations.

The existing code already handles this case gracefully by returning an error when num_notfound > 0, so the DCHECK was simply removed.

vyavdoshenko avatar Dec 11 '25 16:12 vyavdoshenko

I do not know if performance is important here, let's remove idempotent.

Roman Gershman CTO

www.dragonflydb.io http://www.dragonflydb.io

On Thu, Dec 11, 2025, 8:57 PM Vladislav @.***> wrote:

@.**** commented on this pull request.

In src/server/search/search_family.cc https://github.com/dragonflydb/dragonfly/pull/6204#discussion_r2611722479 :

  • DCHECK(num_notfound == 0u || num_notfound == shard_set->size()); auto* rb = static_cast<RedisReplyBuilder*>(cmd_cntx.rb);
  • // If index is not found on at least one shard, return error.
  • // This can happen during concurrent FT.CREATE/FT.DROPINDEX operations
  • // where some shards may have the index while others don't. if (num_notfound > 0u) return rb->SendError(IndexNotFoundMsg(idx_name));

If you remove the IDEMPOTENT flag the issue will disappear, but it will also reduce performance, so its better to make it safe for the callback to be called multiple times

— Reply to this email directly, view it on GitHub https://github.com/dragonflydb/dragonfly/pull/6204#discussion_r2611722479, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4BFCAFPSWXXCOVGCPIOLT4BG5A7AVCNFSM6AAAAACOYFXLESVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNRYHA3TMOJVG4 . You are receiving this because your review was requested.Message ID: @.***>

romange avatar Dec 11 '25 19:12 romange

I do not know if performance is important here, let's remove idempotent.

Because...? And if you remove it for Ft.Info, why not for Ft.Search which have the same schematics? Its just a few lines of code that need to be updated

dranikpg avatar Dec 11 '25 19:12 dranikpg

Ok, ok. Search is called thousands of times, ft.info not. But whatever works for you

Roman Gershman CTO

www.dragonflydb.io http://www.dragonflydb.io

On Thu, Dec 11, 2025, 9:13 PM Vladislav @.***> wrote:

dranikpg left a comment (dragonflydb/dragonfly#6204) https://github.com/dragonflydb/dragonfly/pull/6204#issuecomment-3643402406

I do not know if performance is important here, let's remove idempotent.

Because...? And if you remove it for Ft.Info, why not for Ft.Search which have the same schematics? Its just a few lines of code that need to be updated

— Reply to this email directly, view it on GitHub https://github.com/dragonflydb/dragonfly/pull/6204#issuecomment-3643402406, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4BFCBTSUUCPIHKXM3M6Z34BG66XAVCNFSM6AAAAACOYFXLESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNBTGQYDENBQGY . You are receiving this because your review was requested.Message ID: @.***>

romange avatar Dec 11 '25 20:12 romange