Skip to content

fix(search): Fix integration test failures #5991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 5, 2025
+38 −1
Merged

fix(search): Fix integration test failures #5991

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53b9f6c
fix(search): Fix integration test failures
vyavdoshenko Oct 31, 2025
5a3f3ce
fix: unit test
vyavdoshenko Oct 31, 2025
48aeb5b
fix: review comments
vyavdoshenko Nov 2, 2025
e7d11ac
Merge branch 'main' into bobik/integration_tests_update
vyavdoshenko Nov 5, 2025
0bf58f3
fix: review comments
vyavdoshenko Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/server/acl/acl_family.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class AclFamily final {
{"SCRIPTING", SCRIPTING},
{"BLOOM", BLOOM},
{"FT_SEARCH", FT_SEARCH},
{"SEARCH", FT_SEARCH}, // Alias for FT_SEARCH
{"THROTTLE", THROTTLE},
{"JSON", JSON},
{"ALL", ALL}};
Expand Down
1 change: 1 addition & 0 deletions src/server/main_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,6 +1014,7 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>
config_registry.RegisterMutable("send_timeout");
config_registry.RegisterMutable("managed_service_info");
#ifdef WITH_SEARCH
config_registry.RegisterMutable("search_info_developer_visible");
config_registry.RegisterMutable("MAXSEARCHRESULTS");
#endif

Expand Down
34 changes: 33 additions & 1 deletion src/server/search/search_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
#include "src/core/overloaded.h"

ABSL_FLAG(bool, search_reject_legacy_field, true, "FT.AGGREGATE: Reject legacy field names.");
ABSL_FLAG(bool, search_info_developer_visible, true,
"Enable developer-visible fields in INFO SEARCH");

ABSL_FLAG(size_t, MAXSEARCHRESULTS, 1000000, "Maximum number of results from ft.search command");
namespace dfly {
Expand Down Expand Up @@ -1844,6 +1846,35 @@ void SearchFamily::FtSynUpdate(CmdArgList args, const CommandContext& cmd_cntx)
cmd_cntx.rb->SendOk();
}

void SearchFamily::FtDebug(CmdArgList args, const CommandContext& cmd_cntx) {
// FT._DEBUG command stub for test compatibility
// This command is used by integration tests to control internal behavior
CmdArgParser parser{args};
auto* rb = static_cast<RedisReplyBuilder*>(cmd_cntx.rb);

if (args.empty() || parser.Check("HELP")) {
rb->SendSimpleString("FT._DEBUG - Debug command stub (not fully implemented)");
return;
}

// Handle CONTROLLED_VARIABLE subcommand used by tests
if (parser.Check("CONTROLLED_VARIABLE")) {
if (parser.Check("SET")) {
// Consume variable name and value
if (parser.HasNext())
Copy link
Collaborator

@romange romange Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these optional? if not please change the code to be more strict.
@BorysTheDev can you help with the advice on how to improve parsing?

Copy link
Contributor

@BorysTheDev BorysTheDev Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked Valkey search code.

 if (keyword == "SHOW_INFO") {
    return vmsdk::info_field::ShowInfo(ctx, itr, options);
  } else if (keyword == "PAUSEPOINT") {
    return PausePointControlCmd(ctx, itr);
  } else if (keyword == "CONTROLLED_VARIABLE") {
    return ControlledCmd(ctx, itr);
  } else if (keyword == "HELP") {
    return HelpCmd(ctx, itr);
  } else {
    return absl::InvalidArgumentError(absl::StrCat(
        "Unknown subcommand: ", *itr.GetStringView(), " try HELP subcommand"));
  }

absl::Status ControlledCmd(ValkeyModuleCtx *ctx, vmsdk::ArgsIterator &itr) {
  std::string keyword;
  VMSDK_RETURN_IF_ERROR(vmsdk::ParseParamValue(itr, keyword));
  keyword = absl::AsciiStrToUpper(keyword);
  if (keyword == "LIST") {
    VMSDK_RETURN_IF_ERROR(CheckEndOfArgs(itr));
    auto results = vmsdk::debug::ControlledGetValues();
    ValkeyModule_ReplyWithArray(ctx, 2 * results.size());
    for (auto &r : results) {
      ValkeyModule_ReplyWithCString(ctx, r.first.data());
      ValkeyModule_ReplyWithCString(ctx, r.second.data());
    }
    return absl::OkStatus();
  }
  std::string test_control_name;
  std::string value;
  VMSDK_RETURN_IF_ERROR(vmsdk::ParseParamValue(itr, test_control_name));
  if (keyword == "GET") {
    VMSDK_ASSIGN_OR_RETURN(value,
                           vmsdk::debug::ControlledGet(test_control_name));
    VMSDK_RETURN_IF_ERROR(CheckEndOfArgs(itr));
    ValkeyModule_ReplyWithCString(ctx, value.data());
  } else if (keyword == "SET") {
    VMSDK_RETURN_IF_ERROR(vmsdk::ParseParamValue(itr, value));
    VMSDK_RETURN_IF_ERROR(CheckEndOfArgs(itr));
    VMSDK_RETURN_IF_ERROR(
        vmsdk::debug::ControlledSet(test_control_name, value));
    ValkeyModule_ReplyWithSimpleString(ctx, "OK");
  } else {
    ValkeyModule_ReplyWithError(
        ctx, absl::StrCat("Unknown keyword", keyword).data());
  }
  return absl::OkStatus();
}

So I think we can do the next:

auto cmd = parser.MapNext("HELP", &HelpCmd, "CONTROLLED_VARIABLE", &ControlledCmd,...);
if (parser.TakeError()) {
  rb.sendError()
}
return cmd(...);
}

void ControlledCmd(...) {
  auto action = parser.MapNext("SET", SET, "GET", GET ...)
    switch (action) {
        case SET: 
          parser.Next();
          break:
        case GET:
          ....
    }
    if (parser.TakeError()) {
       rb.sendError()
     }
  }
}

Copy link
Contributor Author

@vyavdoshenko vyavdoshenko Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, this approach adds more bolerplate. I suggest leave the current approach, I guess we will update this command soon fixing failed tests.

Copy link
Contributor

@BorysTheDev BorysTheDev Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok please add error checking

parser.Next(); // variable name
if (parser.HasNext())
parser.Next(); // variable value
// Just acknowledge the command
rb->SendOk();
return;
}
}

// For any other subcommand, just return OK
rb->SendOk();
}

#define HFUNC(x) SetHandler(&SearchFamily::x)

// Redis search is a module. Therefore we introduce dragonfly extension search
Expand Down Expand Up @@ -1874,7 +1905,8 @@ void SearchFamily::Register(CommandRegistry* registry) {
<< CI{"FT.TAGVALS", kReadOnlyMask, 3, 0, 0, acl::FT_SEARCH}.HFUNC(FtTagVals)
<< CI{"FT.SYNDUMP", kReadOnlyMask, 2, 0, 0, acl::FT_SEARCH}.HFUNC(FtSynDump)
<< CI{"FT.SYNUPDATE", CO::WRITE | CO::GLOBAL_TRANS, -4, 0, 0, acl::FT_SEARCH}.HFUNC(
FtSynUpdate);
FtSynUpdate)
<< CI{"FT._DEBUG", kReadOnlyMask, -1, 0, 0, acl::FT_SEARCH}.HFUNC(FtDebug);
}

} // namespace dfly
1 change: 1 addition & 0 deletions src/server/search/search_family.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class SearchFamily {
static void FtSynDump(CmdArgList args, const CommandContext& cmd_cntx);
static void FtSynUpdate(CmdArgList args, const CommandContext& cmd_cntx);
static void FtConfig(CmdArgList args, const CommandContext& cmd_cntx);
static void FtDebug(CmdArgList args, const CommandContext& cmd_cntx);

public:
static void Register(CommandRegistry* registry);
Expand Down
9 changes: 6 additions & 3 deletions src/server/search/search_family_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3330,11 +3330,14 @@ TEST_F(SearchFamilyTest, MAXSEARCHRESULTS) {
"Maximum number of results from ft.search command", "Value", "1"));

resp = Run({"FT.CONFIG", "GET", "*"});
EXPECT_THAT(resp, IsArray("MAXSEARCHRESULTS", "1"));
EXPECT_THAT(resp, IsUnordArray("MAXSEARCHRESULTS", "1", "search_info_developer_visible", "true"));

resp = Run({"FT.CONFIG", "HELP", "*"});
EXPECT_THAT(resp, IsArray("MAXSEARCHRESULTS", "Description",
"Maximum number of results from ft.search command", "Value", "1"));
EXPECT_THAT(resp, IsUnordArray(
IsArray("search_info_developer_visible", "Description",
"Enable developer-visible fields in INFO SEARCH", "Value", "true"),
IsArray("MAXSEARCHRESULTS", "Description",
"Maximum number of results from ft.search command", "Value", "1")));

// restore normal value for other tests
Run({"FT.CONFIG", "SET", "MAXSEARCHRESULTS", "1000000"});
Expand Down
70 changes: 70 additions & 0 deletions src/server/server_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ ABSL_DECLARE_FLAG(int32_t, port);
ABSL_DECLARE_FLAG(bool, cache_mode);
ABSL_DECLARE_FLAG(int32_t, hz);
ABSL_DECLARE_FLAG(bool, tls);
#ifdef WITH_SEARCH
ABSL_DECLARE_FLAG(bool, search_info_developer_visible);
#endif
ABSL_DECLARE_FLAG(string, tls_ca_cert_file);
ABSL_DECLARE_FLAG(string, tls_ca_cert_dir);
ABSL_DECLARE_FLAG(int, replica_priority);
Expand Down Expand Up @@ -3246,11 +3249,78 @@ string ServerFamily::FormatInfoMetrics(const Metrics& m, std::string_view sectio
"errors]");
}

#ifdef WITH_SEARCH
if (should_enter("SEARCH", true)) {
append("search_memory", m.search_stats.used_memory);
append("search_num_indices", m.search_stats.num_indices);
append("search_num_entries", m.search_stats.num_entries);

// Developer-visible fields - controlled by search_info_developer_visible flag
// These fields provide detailed internal statistics useful for debugging and monitoring
bool show_developer_fields = absl::GetFlag(FLAGS_search_info_developer_visible);

if (show_developer_fields) {
// Queue and worker pool statistics
append("search_query_queue_size", 0);
append("search_writer_queue_size", 0);
append("search_worker_pool_suspend_cnt", 0);
append("search_writer_resumed_cnt", 0);
append("search_reader_resumed_cnt", 0);
append("search_writer_suspension_expired_cnt", 0);

// RDB save/load statistics
append("search_rdb_load_success_cnt", 0);
append("search_rdb_load_failure_cnt", 0);
append("search_rdb_save_success_cnt", 0);
append("search_rdb_save_failure_cnt", 0);

// Request statistics
append("search_successful_requests_count", 0);
append("search_failure_requests_count", 0);
append("search_hybrid_requests_count", 0);
append("search_inline_filtering_requests_count", 0);

// HNSW exception counters
append("search_hnsw_add_exceptions_count", 0);
append("search_hnsw_remove_exceptions_count", 0);
append("search_hnsw_modify_exceptions_count", 0);
append("search_hnsw_search_exceptions_count", 0);
append("search_hnsw_create_exceptions_count", 0);

// String interning statistics
append("search_string_interning_store_size", 0);
append("search_string_interning_memory_bytes", 0);
append("search_string_interning_memory_human", "0B");

// Vector externing statistics
append("search_vector_externing_entry_count", 0);
append("search_vector_externing_hash_extern_errors", 0);
append("search_vector_externing_generated_value_cnt", 0);
append("search_vector_externing_num_lru_entries", 0);
append("search_vector_externing_lru_promote_cnt", 0);
append("search_vector_externing_deferred_entry_cnt", 0);

// Cancel/timeout statistics
append("search_cancel-timeouts", 0);
}

// Always visible fields - public API
append("search_index_reclaimable_memory", 0);
append("search_number_of_attributes", 0);
append("search_number_of_indexes", m.search_stats.num_indices);
append("search_total_indexed_documents", m.search_stats.num_entries);
append("search_number_of_active_indexes", 0);
append("search_number_of_active_indexes_running_queries", 0);
append("search_number_of_active_indexes_indexing", 0);
append("search_total_active_write_threads", 0);
append("search_total_indexing_time", 0);
append("search_used_memory_bytes", m.search_stats.used_memory);
append("search_used_memory_human", HumanReadableNumBytes(m.search_stats.used_memory));
append("search_background_indexing_status", "idle");
append("search_used_read_cpu", 0.0);
append("search_used_write_cpu", 0.0);
}
#endif

if (should_enter("ERRORSTATS", true)) {
for (const auto& k_v : m.facade_stats.reply_stats.err_count) {
Expand Down
Loading