Conversation
…gnostics Strengthen the synonym dictionary for Japanese workloads by improving loading diagnostics and adding comprehensive test coverage across both unit and search-pipeline levels. Synonym dictionary improvements (synonym_dictionary.cpp / .h): - Preserve raw tokens before normalization to enable meaningful warning messages when tokens collapse or conflict - Emit structured log event "synonym_group_collapsed" when all tokens in a group normalize to the same value, including the raw term preview and source line number - Emit "synonym_group_term_conflict" when too few non-conflicting terms remain to form a usable group, listing the conflicting terms - Add ForEachTerm() method (shared-lock safe) so callers can inspect every loaded term without exposing internal data structures Startup diagnostics (server_orchestrator.cpp): - After loading a synonym dictionary, call ForEachTerm() to identify terms that are too short to produce any n-grams given the table's ngram_size / kanji_ngram_size configuration - Emit "synonym_variant_unreachable" warning for each such term so operators are alerted to silently dead entries at startup rather than at query time Unit tests (tests/query/synonym_dictionary_test.cpp): - Add RealNormalizerSynonymDictionaryTest fixture backed by a real Index normalizer (NFKC + width=half + lower=true) to match production - Add ForEachTermIteratesAllTerms to cover the new ForEachTerm API - Add seven Japanese normalization tests (TC-SD-01 through TC-SD-07) covering half-width kana collapse, hiragana/katakana distinctness, kana+kanji three-element groups, full-width ASCII folding, dedup after partial collapse, multiple independent groups, and first-wins conflict resolution E2E pipeline tests (tests/server/search_pipeline_synonym_jp_test.cpp): - New JapaneseSynonymPipelineTest fixture using ExecuteFullPipeline with ngram_size=3 and kanji_ngram_size=2 - TC-SP-01: Hiragana query expands to katakana and kanji documents - TC-SP-02: Kanji query returns the same set via expansion - TC-SP-03: Half-width kana normalizes to full-width and reaches the matching synonym group (kana + ASCII alternative) - TC-SP-04: Kanji bigrams (kanji_ngram_size=2) do not break expansion when synonyms share a leading bigram - TC-SP-05: OR within synonym group, AND across multiple query terms - TC-SP-06: verify_text="all" post-filter accepts synonym-matched docs via PostFilterByTextWithSynonyms - CMakeLists.txt wired up as search_pipeline_synonym_jp_test target
…/CRLF/port bugs Refactor mygram-cli.cpp to delegate socket I/O, hostname resolution, response framing, and timeouts to mygramdb::client::MygramClient, removing ~500 lines of duplicate implementation. Fix several correctness bugs discovered during the rewrite. Bug fixes: - Hostname resolution: replace inet_pton (IPv4-literal only) with getaddrinfo via MygramClient, so -h localhost now works - Response truncation: replace single 64 KB recv() with the client library's full-response loop using IsResponseComplete; large SEARCH/INFO responses no longer get cut off - REPLICATION STATUS CRLF: old code searched for literal escape sequences "\\r\\n" instead of the actual two-byte sequence, leaving raw CR/LF on screen; replaced with a shared NormalizeCrlf helper - INFO/REPLICATION/CACHE_STATS: now strip the trailing END sentinel and the leading blank line from the OK <TYPE>\r\n\r\n... framing - Port validation: static_cast<uint16_t>(stoi(...)) silently truncated out-of-range values (port 70000 -> 4464); now rejects values outside [1, 65535], non-numeric input, and trailing-garbage parses - SIGPIPE: process now ignores SIGPIPE so a closed server connection raises EPIPE on send() instead of killing the CLI - Arg joining: whitespace-containing args are now quoted before being sent over the wire (SEARCH articles "hello world" -> two tokens) - SO_SNDTIMEO / Unix-socket RCVTIMEO: set on both TCP and Unix paths via the client library (was TCP-only before) New response handlers (previously fell through to "Unknown response"): - OK CONFIG, OK FACET, OK CACHE_STATS / CACHE_CLEARED / CACHE_ENABLED / CACHE_DISABLED, OK SYNC STARTED|STOPPED, OK DUMP_STARTED / DUMP_VERIFIED / DUMP_INFO / DUMP_STATUS, OK OPTIMIZED, +OK body Extracted helpers: - ToUpper, Trim, NormalizeCrlf, StripTrailingEndMarker, QuoteArgIfNeeded, StartsWith, JoinArgsForCommand, PrintHeaderAndBody, FormatMultiLineBody, PrintSearchOrCountResponse, PrintConnectionHints - ParseArguments now returns a testable ParseResult struct - Prefix-length constants imported from server/protocol_constants.h UX improvements: - Tab completion adds FACET, CACHE, DUMP, SYNC, OPTIMIZE; removes literal placeholder candidates like "<column_name>" - Help text uses SORT (actual protocol keyword) instead of ORDER BY - New --version / -V flag - Trivial commands (help/quit/exit) no longer pollute readline history - Connection error hints cover hostname-resolution failures and missing Unix socket files Testing: - ~50 new/rewritten test cases in mygram_cli_test.cpp - New: string-helper unit tests (ToUpper, Trim, NormalizeCrlf, StripTrailingEndMarker, QuoteArgIfNeeded, JoinArgsForCommand) - New: ParsePort tests (valid range, out-of-range, non-numeric, trailing garbage) - New: ParseArguments tests for every flag including --version / --help - New: response pretty-print coverage for CONFIG, FACET, CACHE_*, SYNC_*, DUMP_*, OPTIMIZED, +OK - New: regression test for the REPLICATION CRLF bug - New: INFO normalization and END-marker stripping test - src/cli/CMakeLists.txt: link mygramclient_static + mygramdb_utils - tests/cli/CMakeLists.txt: same, plus CMAKE_BINARY_DIR/src include for the generated version.h header - All 2867 fast-tier tests pass (ctest -LE "SLOW|LOAD")
SendCommand hung until the per-socket timeout for CACHE_STATS, DUMP_INFO, and DUMP_STATUS responses because IsResponseComplete treated their "OK CACHE_STATS\r\n...\r\nEND\r\n" wire format as unrecognised and kept accumulating data waiting for the double-CRLF that never arrives. - Add CACHE_STATS, DUMP_INFO, DUMP_STATUS to the END-marker branch in IsResponseComplete (src/client/protocol_detection.h) - DUMP_INFO first-line carries an optional filepath suffix, so detection uses prefix matching (first_crlf >= 12) rather than exact length - Extract EndsWith helper to eliminate repeated bounds-check boilerplate for both the "END\r\n" and "\r\n\r\n" terminal patterns - Update function-level doc comment to name all six END-terminated commands
…earchExpression SimplifySearchExpression returned false for any expression that had no required terms (+ prefix), even when the expression was a valid OR sub-expression such as "python OR ruby" or "(a OR b)". Callers that relied on this helper (e.g. the CLI's smart-query mode) silently fell back to a raw string, bypassing term splitting and NOT handling. - When required_terms is empty but raw_expression is non-empty, surface the OR sub-expression as main_term wrapped in parentheses so it is valid when AND-composed by the caller; already-parenthesized inputs are not double-wrapped - Fix docstring examples: ToQueryString always wraps bare OR expressions in parentheses; the old example claimed unparenthesized output - Add four new tests: SimplifyOrOnly, SimplifyParenthesizedOnly, SimplifyMixed, ToQueryStringWrapsOrInParens
Addresses eight independent bugs found in code review. All 3091 fast tests pass with these fixes. DEBUG response parsing (SearchWithDebugInfo, CountWithDebugInfo): - Server emits "key: value\r\n" lines; client expected whitespace- tokenised "key=value" pairs, so all debug fields were always zero - The literal "#" from "# DEBUG" leaked into resp.results as a spurious primary key entry - Replace token-based ParseDebugInfo with a line-based parser using the new ParseColonKeyValueLines helper; split Search/Count main body from debug section via SplitDebugBlock before parsing either GetReplicationStatus parsing (GetReplicationStatus): - Same colon-vs-equals mismatch; running was always false, gtid always empty; switch to ParseColonKeyValueLines and map current_gtid -> gtid - Add processed_events and queue_size fields to ReplicationStatus struct INFO key mapping (Info, InfoMultiLineComplete): - active_connections was always 0 because the client looked for key "active_connections" but the server emits "connected_clients" - index_size_bytes was always 0 because the server key is "used_memory_bytes" Connect timeout (ConnectTimeoutOnUnreachableHost): - SO_RCVTIMEO/SO_SNDTIMEO only govern send/recv, not connect(); a connection to an unreachable host blocked for the full OS default (~75s) ignoring timeout_ms - Add ConnectWithTimeout helper: set O_NONBLOCK, call connect(), poll() for POLLOUT with the configured timeout, then restore blocking mode; ApplySocketTimeouts sets SO_RCVTIMEO/SO_SNDTIMEO after connect Identifier validation (RejectsWhitespace/Empty* tests): - Table names, primary keys, sort columns, and filter keys are sent unquoted on the wire; embedded whitespace breaks the protocol by splitting a single identifier into multiple tokens - Add ValidateIdentifier helper and apply it to Search, Count, and Get OFFSET-only emission (SearchWithOffsetOnlyAppliesOffset): - offset > 0 with limit == 0 silently dropped the offset; callers expecting the server to skip N results got the first page instead - Emit bare "OFFSET <n>" when only offset is non-zero Mutex serialization (ConcurrentSendCommandsSerialize): - The class header claimed thread-safety but Impl had no mutex; concurrent threads interleaved send/recv bytes and corrupted the protocol stream - Add mutable std::mutex command_mutex_ to Impl; lock in SendCommand() C API array validation (CApiSearchNullTermsCrashGuard): - search_advanced / count_advanced with count > 0 but NULL array pointer dereferenced NULL and segfaulted; validate up front and return -1 - Move MygramClient construction inside the try block in mygramclient_create so a throw during construction is covered by unique_ptr and cannot leak Error message improvements (ConnectInvalidHostnameIncludesGaiError, SearchEmptyQueryReturnsError): - DNS errors now include gai_strerror() text and the offending hostname - EscapeQueryString emits explicit "" for empty strings so the server can parse a well-formed token instead of seeing a malformed command C API replication status (CApiReplicationStatus): - Expose GetReplicationStatus through the C API as mygramclient_replication_status / mygramclient_free_replication_status - Add MygramReplicationStatus_C struct with running, gtid, status_str, processed_events, queue_size fields Refactoring (no behaviour change): - SendAndExpectPrefix helper consolidates the SendCommand -> error check -> prefix assert pattern used in every command method - ParseColonKeyValueLines replaces ParseKeyValuePairs for colon-delimited multi-line responses; ParseKeyValuePairs is kept for "key=value" tokens - CArrayToVector / CFilterArraysToVector / ForwardVoid helpers reduce copy-paste in mygramclient_c.cpp - Protocol constants accessed via namespace alias (proto::) instead of inline constexpr copies - Impl marked non-movable (std::mutex is not movable); MygramClient move semantics are preserved via its unique_ptr<Impl> member
…help OPTIMIZE is a top-level command handled by the query parser; it has no DEBUG subcommand form. The readline completion table offered "DEBUG OPTIMIZE" as a valid sequence, misleading users into sending a malformed command that the server would reject. - Remove "OPTIMIZE" from the DEBUG subcommand completion list; only "ON" and "OFF" are valid DEBUG subcommands - Add "OPTIMIZE [table]" to the inline help text so users can discover it - Remove the "OK CONFIG" branch from the multi-line response display path: CONFIG SHOW emits "+OK\r\n..." (handled by the existing +OK branch) and the server never emits a literal "OK CONFIG" prefix; the dead branch added noise and misleading documentation - Remove the now-defunct kOkInfoPrefixLength and kOkReplicationPrefixLength re-export aliases (constants removed from protocol_constants.h in the dead-code cleanup) and their corresponding prefix-length tests
FormatConfigResponse in ResponseFormatter was never called from any handler; CONFIG SHOW is served by a separate code path that formats the YAML directly. kOkInfoPrefixLen and kOkReplicationPrefixLen in protocol_constants.h were only referenced by the now-deleted CLI re-export aliases. - Remove FormatConfigResponse declaration from response_formatter.h and its ~60-line implementation from response_formatter.cpp - Remove kOkInfoPrefixLen and kOkReplicationPrefixLen from protocol_constants.h; the only caller (mygram-cli.cpp re-exports) was also removed in the preceding CLI commit - Remove FormatConfigResponse and FormatConfigResponseUsesCRLFLineEndings unit tests from tests/server/response_formatter_test.cpp
StartSync held sync_mutex_ via lock_guard while calling join() on the previous sync thread, but BuildSnapshotAsync's terminal update_state lambda re-acquires the same mutex. If the prior thread was waiting on update_state at that moment, the two paths deadlocked. This matched the pattern that ~SyncOperationManager already documents and avoids. Switch StartSync to unique_lock, move the previous thread out of sync_threads_ under the lock, then unlock before join() and re-lock to continue. Adds a regression test that re-runs StartSync after the prior sync drained through the failure path, guarded by a 5-second timeout so a future deadlock surfaces as a test failure instead of a hang.
The Init* helpers in ServerLifecycleManager wrapped make_unique calls in try/catch and converted std::exception into Expected<...> errors. None of the constructed objects throw, and catching std::bad_alloc only hides OOM that should terminate the process. CLAUDE.md mandates Expected<T,Error> instead of exceptions, so the wrappers were both dead code and a regression in error fidelity. RequestDispatcher had the same pattern around handler->Handle(); handlers return strings via ResponseFormatter::FormatError and never throw. Both have been removed. SnapshotScheduler's constructor previously logged an error and kept going when catalog_ was null, leaving the object in a state that would crash on the next TakeSnapshot. Move the precondition check into ServerLifecycleManager::InitScheduler so a null catalog produces an Expected error before construction, and document the constructor's non-null contract. Adds a regression test covering the InitScheduler optional path (interval_sec == 0) and tightens the success path to verify IsRunning.
…race reactor_handler_ (std::function) was assigned by the main thread via TcpServer::Start->SetReactorHandler after the accept thread had already been spawned by ServerLifecycleManager::InitAcceptor. The accept loop read reactor_handler_ on every connection, producing a data race that TSan flagged as UB. Split Start() into two phases. Start() now only binds and listens. StartAccepting() spawns the accept thread and is the new place where TcpServer wires it up - after SetReactorHandler has run, so the std::thread launch creates the happens-before edge that synchronizes the handler write with the worker. StartAccepting requires both Start() and a non-null handler and reports kNetworkAcceptorNoHandler (new error 6025) or kNetworkServerNotStarted otherwise. Stop() resets the thread slot so Start->Stop->Start->StartAccepting is reusable. Existing acceptor tests were updated to call StartAccepting after SetReactorHandler. Five new cases cover the precondition matrix and restart-after-stop.
ConnectionAcceptor::server_fd_ was a plain int read by the accept loop and written by Stop, which is a data race that TSan flags as UB. Promote it to std::atomic<int>. Stop() now exchanges the fd out before shutdown/close so accept callers either see the live fd or -1, never an in-flight transition. AcceptLoop snapshots the fd with acquire ordering before each accept call so the loop terminates promptly when Stop runs. KqueueMultiplexer::Add/Modify/Remove released interest_mutex_ between reading the old interest mask, issuing the kevent syscall, and writing the new mask back. Concurrent Modify calls on one fd could read the same old mask, both issue diff-syscalls, then race to write the map - the kernel ended up with one filter set while interest_ recorded another, leaking EVFILT_WRITE filters and producing event storms. Hold interest_mutex_ across the kevent syscall and the map update so the kernel and map advance together. Add a fast-path early-return when the new mask matches the old, and a regression test that hammers Modify from 8 threads to verify no spurious failures or divergence.
IoReactor::Register dropped connections_mutex_ between inserting the fd and arming the multiplexer, so a concurrent Stop() could clear connections_ in that gap and leave the entry stranded. Restructure Register to take mux_lifecycle_ (shared) first, then connections_mutex_ (unique), and run the running_ check, map insert, and mux_->Add inside that single window with proper rollback on failure. ReactorConnection::OnReadable made its close decision across two separate critical sections - it sampled pending_frames_, dropped frame_mutex_, then sampled write_queue_ under write_mutex_. A drain task finishing between the two could push a response after the first sample saw the queue empty, and the connection would close before the response went out. Hold both mutexes (frame->write order) for the empty-and-eof check so the close transition is atomic with the work queues. DrainTask cleared drain_scheduled_ before re-checking whether to requeue, opening a window where another path could schedule a second drain and break the at-most-one-task invariant. Keep drain_scheduled_ true while the task decides to reschedule; only clear it on the no-more-work path inside frame_mutex_. The follow-up Submit happens outside the lock and inherits the still-true flag. IoReactor Start/Stop could race on the event_loop_thread_ slot: if Stop ran between running_=true and the std::thread assignment, Stop saw a non-joinable thread and returned, leaking the loop that Start was about to spawn. Add start_stop_mutex_ and serialise both phases. Documents the lock order start_stop_mutex_ -> mux_lifecycle_ -> connections_mutex_. New regression coverage: rapid and concurrent Start/Stop cycles for IoReactor, Register racing Stop without stale entries, and a peer half-close path that dispatches a frame and verifies the response reaches the peer before the connection closes.
Start could spawn the scheduler thread after Stop had already flipped running_ to false and missed the join, leaking the thread that SchedulerLoop was about to enter. Add start_stop_mutex_ around the running_ flip plus thread construction in Start and notify_all/join in Stop. SchedulerLoop never takes the mutex, so the wait inside the loop stays correct. TakeSnapshot wrote the dump without pausing the binlog reader, so the auto-snapshot path could observe Index/DocumentStore being mutated concurrently and emit a torn dump - DumpHandler already guards against this via replication_paused_for_dump, but the scheduler did not. Mirror that guard: stop the reader, set the flag, capture GTID, and let a ScopeGuard restart replication on every exit path. The ReplicationHandler check that already rejects manual REPLICATION START when the flag is set now applies to scheduled snapshots too. Threads through the new replication_paused_for_dump reference via the SnapshotScheduler constructor and ServerLifecycleManager::InitScheduler. Adds a stub-based test that verifies the flag pulses around WriteDump and a concurrent Start/Stop test that fails without the lifecycle mutex.
HTTP /search and /count concatenated the URL-derived table name and the JSON q field straight into the parser command. q could carry parser clause keywords like "foo LIMIT 0 OFFSET 999999" that silently overrode the JSON-supplied limit/offset, and a table name containing whitespace or punctuation could split the parser tokens entirely. Add IsValidTableName (alphanumerics, underscore, hyphen, dot, plus any byte >= 0x80 to keep UTF-8 names working) and ValidateQueryTextNoReservedClauses (rejects unquoted LIMIT/OFFSET/ ORDER/FILTER/SORT/HIGHLIGHT/FUZZY tokens, case-insensitive, while still permitting them inside single- or double-quoted phrases and keeping AND/OR/NOT as first-class search syntax). Both helpers run before the string concatenation in HandleSearch and HandleCount and return HTTP 400 with a message naming the offending input. SearchHandler::HandleSearch had one path that returned sorted_result.error().to_string() directly instead of going through ResponseFormatter::FormatError, so a sort/pagination failure produced "[Error 1004 ...] message" on the wire instead of the expected ERROR prefix. Route it through FormatError like every other failure. Adds smuggling regression tests for both endpoints, table-name validation cases, and a TCP-side test that asserts the ERROR-prefixed shape for sort failures.
Many StructuredLog().Event("server_error").Field("error", ...).Error()
sites lacked the numeric error_code, so log queries and Prometheus
alerts could not pivot on it. Add StructuredLog::FieldError(const
Error&) that emits message and error_code together as one chained
call. Apply it across dump/admin/io_reactor/sync_operation_manager
sites that were missing error_code, and promote reactor_poll_failed
from Warn to Error since epoll/kevent failures are not warnings.
CONFIG VERIFY failures were logging at Error level under server_error,
which is wrong for a client-input mistake. Rename the event to
config_verify_failed and downgrade to Warn so it stops triggering
operational alerts.
OK responses across handlers were a mix of "+OK", "+OK ...", and "OK
...", with each handler hand-rolling the literal. Add
ResponseFormatter::FormatOk(body) for the Redis-style status reply
("+OK [body]") and FormatStatus(body) for the result-style reply
("OK <body>"). Apply them in admin/cache/debug/dump/variable handlers
so the byte sequences on the wire are unchanged but the format lives
in one place.
Adds unit tests for both helpers.
Path validation lived in two places. dump_handler had a robust
canonical-path-plus-lexically_relative implementation; admin_handler's
HandleConfigVerify rolled its own with rfind('.') extension matching
and could be tricked through a directory-on-path symlink. Pull both
into mygram::utils::ResolveSafePath in src/utils/safe_path.{h,cpp},
exposing an Expected<string, Error> API with an optional allowed-
extensions filter, and call it from both handlers. dump_handler's
ResolveDumpPath stays as a thin wrapper to keep its callers unchanged.
The same SYNC-in-progress-block message was hand-built in
HandleDumpSave, HandleDumpLoad, REPLICATION_START, and DEBUG OPTIMIZE.
Add SyncOperationManager::CheckNoSyncInProgress(operation) that
returns Expected<void, Error> with the unified
"Cannot {operation} while SYNC is in progress for tables: a b c"
message, and call it from those four sites. variable_handler's
per-variable check stays as-is because its message format differs.
New tests cover the safe_path edge cases (.., absolute escapes,
extension allowlist, symlink-on-path) and the SYNC conflict path.
…earch prep The "server is loading" guard was open-coded in three handlers (search, document, facet) and the facet path duplicated it twice. Add CommandHandler::CheckNotLoading() so each call site is one line and the response stays byte-for-byte identical. HttpServer split request counting between stats_ and tcp_stats_ asymmetrically: HandleInfo branched on tcp_stats_ != nullptr while HandleSearch and HandleCount always wrote to stats_, so an operator who supplied tcp_stats expecting unified counters got silently inconsistent numbers. Add a single RecordRequest() helper and route every handler entry point through it. HandleSearch and HandleCount opened with ~100 lines of identical preamble - loading check, table-name validation, table lookup, JSON parse, q-field presence/type/control-char checks, reserved-keyword filter, QueryParser, default limit, and JSON filters parse. Extract PrepareHttpSearchQuery into HttpServer; both handlers now call it and diverge only at the actual SEARCH/COUNT execution. Pagination handling is gated by an apply_pagination flag so COUNT keeps skipping LIMIT/ OFFSET and the reserved-keyword error text still names only the fields each endpoint accepts. Adds a CommandHandler unit test for the loading-check contract and a two-mode HttpServer test that locks in stats_ vs tcp_stats_ routing.
ExecuteSearchPipeline was a 290-line function with three near- duplicated debug+timing blocks for the fuzzy, synonym, and regular paths and inline orchestration of cache lookup, NOT/filter/verify, and cache insertion. Split it into a 53-line orchestrator that delegates to search_pipeline::ExecuteFullPipeline plus four small private helpers (BuildPipelineParams, PopulateInputDebugInfo, PopulateCacheHitDebugInfo, PopulateEmptyTermDebugInfo, PopulatePostPipelineDebugInfo). Routing both the HTTP HandleSearch path and the TCP SearchHandler through ExecuteFullPipeline removes the duplicated orchestration glue that previously diverged between the two protocols. To keep TCP debug output byte-for-byte the same, surface the path tag, empty-term detection, and cache age/saved metrics from FullPipelineOutput so the TCP layer can recover the labels it used to compute itself. Cache- lookup latency is now measured inside the pipeline and returned as query_time_ms on hits, matching the previous TCP behavior. The pipeline also skips cache insertion when empty_term_detected; HTTP gains this conservative behavior alongside TCP and the rationale is documented in the code. Adds cross-protocol consistency tests that send the same SEARCH and COUNT queries through both HTTP and TCP and assert the result counts match.
Drop unused #include <spdlog/spdlog.h> from admin/debug/replication/ sync handlers and response_formatter, where logging now goes through StructuredLog and spdlog is no longer referenced directly. Mark the unused query parameter on SyncHandler::HandleSyncStatus so it no longer trips the unused-parameter warning, matching the rest of the codebase's no-name-on-unused-args convention. Add a const overload to TableCatalog::GetTable so a const TableCatalog can be queried; the non-const overload still returns a mutable TableContext* for callers that need it. Adds a const-overload test. GetTableContext returned kInternalError for a missing catalog and the generic kNotFound (range 0-999) for a missing table. Per CLAUDE.md the business-logic range is 4000-4999, so add kTableNotFound (4007) and kCatalogNotInitialized (4008) and route the two error paths through them. The wire message stays "Table not found: <name>" so client behavior is unchanged. ConnectionAcceptor's BUSY response used write() with the return value silenced via NOLINT, which let SIGPIPE be raised when the peer had already closed and gave us no signal that the notice was even attempted. Switch to send(MSG_NOSIGNAL) on platforms that have it (send(0) elsewhere - macOS already gets SO_NOSIGPIPE from SetSocketOptions), keep the result intentionally unused with a comment explaining why best-effort is correct here. RateLimiter::AllowRequest's std::string parameter and ResolveDumpPath's std::string args were considered but not converted: the former uses std::string keys in an unordered_map without heterogeneous lookup, so string_view would force a copy at the lookup site instead of saving one; the latter is now a thin wrapper over ResolveSafePath which already takes string_view.
- CacheManager::Clear/ClearTable now serialize via mutex_ so concurrent Insert can no longer leave phantom InvalidationManager metadata after QueryCache is emptied (P0-B). - QueryCache::Clear now invokes eviction_callback_ for every entry, ensuring InvalidationManager state stays in sync when callers bypass CacheManager (P0-G). - LookupInternal dedupes decompression_failures via the pending-keys set so multiple concurrent failed lookups of the same entry count once (P0-E). - RemoveEntryLocked updates stats_.current_memory_bytes inline so GetStatistics reflects evictions immediately instead of after RefreshLRU (P0-H). - CacheKey hash combiner switched from XOR to Fibonacci-mixed step, eliminating swapped-pair collisions. - Disable() now drains the invalidation queue and clears all entries, giving a deterministic cold cache after Enable() rather than silently retaining stale entries. - InvalidationQueue replaces hex-string composite keys with a typed PendingKey pair, removing per-event O(k) string allocations on the invalidation hot path. - InvalidationManager::ClearTable is now O(k) via a table_to_cache_keys_ reverse index instead of scanning all metadata. - CacheStatistics gains rejection_count, forced_clears, max_memory_bytes, min_query_cost_ms, ttl_seconds, and compression_enabled for operator visibility into cache configuration and eviction patterns. - LRUEviction test now asserts the strict-LRU victim is gone; new no-compression fixture covers the previously untested code path.
- DUMP LOAD now restores replication on every error path. Filepath validation runs before binlog stop so bad input fails fast, and a ScopeGuard ensures replication restart and flag clearance even on ReadDump failures. Previously a bad path or read failure left the server permanently in loading mode with replication stopped (P0-A). - HttpServer::Start replaces check-then-set on running_ with compare_exchange_strong, eliminating the race where two concurrent Start calls could both spawn a server thread or where Stop could observe running_=true and skip the join (P0-C). Internal failure paths use store(false) only after holding the gate via successful CAS. - SyncOperationManager::StartSync now uses a three-phase pattern (validate-and-claim → join-previous → spawn) with an explicit JOINING_PREVIOUS transitional status. The previous code dropped sync_mutex_ across the previous_thread.join(), exposing a window where concurrent racers could observe partially-modified state (P0-D). Crucially, the slot is now claimed unconditionally during Phase 1 — earlier code only claimed when a previous thread existed, letting a fresh burst of concurrent StartSync calls all reach the spawn step. - AdminHandler null-catalog guard scoped to INFO so CONFIG HELP/SHOW/ VERIFY remain usable without a catalog (matches existing test fixtures). - SyncHandler/ServerLifecycleManager error codes moved to the correct ranges (Index 4xxx for kSyncManagerNull, Server 6xxx for kServerInitMissingDependency).
- Set TCP_NODELAY on accepted sockets (TCP only, not UDS) to remove Nagle delay on the request/response protocol. - HTTP server enforces a configurable max body size via set_payload_max_length, returning 413 for oversize requests. api.http.max_body_bytes added to config. - Reactor connections track last_active_ on every read/write event; IoReactor periodically reaps connections idle longer than idle_timeout_sec, complementing TCP keepalive whose defaults can be hours. Slow-loris-style stalls no longer hold connection slots indefinitely. - TcpServer and HttpServer now share a single RateLimiter so a client cannot get 2x the configured quota by spreading load across protocols. - IoReactor::Register adds the fd to the multiplexer first, then to connections_, removing the rollback path and the window where Lookup could return a fd not yet registered with the kernel poll set. - ThreadPool shutdown with timeout uses a condition_variable instead of 10ms sleep polling, returning promptly when workers drain. - RateLimiter cleanup sweep moved to a dedicated background thread with cv-based shutdown, eliminating O(n) latency spikes on the request hot path. - Remove direct <spdlog/spdlog.h> includes from server source — all logging goes through StructuredLog. Documented at file top. - Comment expansions: kqueue thread-safety contract, MSG_NOSIGNAL portability guard, RemoveConnection idempotency post-Stop, TokenBucket double-precision rationale.
- FacetHandler now routes through search_pipeline::ExecuteFullPipeline instead of calling Execute directly. Synonym expansion, fuzzy matching, and result caching now apply identically to facet-scoped searches and SearchHandler — previously the two diverged silently. - Extract BuildPipelineParamsFromContext as a free function so HTTP HandleSearch/HandleCount and the TCP path build params identically. - New HttpServer::ResolveHttpTableContext consolidates the IsValidTableName + table lookup + index/doc_store null-check pattern duplicated across HandleSearch, HandleCount, and HandleGet. HandleGet also now validates table names like the other handlers. - RequestDispatcher drops the redundant TableExists pre-check since every handler that needs the catalog re-validates via GetTableContext. - ResponseFormatter cache-debug section unified between SEARCH and COUNT via a shared WriteCacheDebugLines helper. - ResolveSafePath gains a base_dir_label parameter so DumpHandler no longer needs a string-rewriting wrapper. - Shared utilities: src/utils/roaring_bitmap_ptr.h provides RoaringBitmapPtr / MakeRoaringFromVector / MakeEmptyRoaring, replacing the local typedefs in search_pipeline and the manual bitmap construction in facet_handler. - New src/server/operation_names.h centralizes operation strings passed to CheckNoSyncInProgress. - DebugHandler::OPTIMIZE drops its local OptimizationGuard struct in favor of mygram::utils::AtomicFlagResetGuard. CACHE ENABLE/DISABLE and OPTIMIZE responses now route through ResponseFormatter::FormatStatus. - ServerStats covers every QueryType — uncommon commands accumulate into cmd_other_, and RequestDispatcher::Dispatch now calls IncrementRequests so total_requests is non-zero (P0-I). - SearchHandler emits a structured search_completed event with query_time_ms / cache_hit / result_count for slow-query analysis. - DumpHandler emits dedicated dump_load_failed / dump_verify_failed event names instead of the generic server_error, and dump_save_started now includes the captured GTID. - AdminHandler::CONFIG VERIFY adds an O_NOFOLLOW probe to narrow the TOCTOU window between symlink check and LoadConfig. - TableCatalog: documented as immutable-post-construction; mutex removed since it served no purpose for the read-only access pattern. - HttpServer adds GetEffectiveStats so callers always read the active counter source whether stats are local or shared with TcpServer. - SyncStatus IDLE response now goes through ResponseFormatter for protocol framing consistency. - Cross-cutting: structured_log.h promotes kMaxQueryLogLength to namespace scope; RequestDispatcher truncates the request field and emits a separate request_full_length so log injection / unbounded log volume from untrusted client input is bounded. - spdlog direct include removed from snapshot_scheduler and request_dispatcher (server-source convention is StructuredLog only). - Comment expansions on intentional patterns (facet double CheckNotLoading, replication flag-clear ordering, snapshot scheduler cv discipline, dump info filepath disclosure) to immunize against future review re-flagging. - New tests: roaring_bitmap_ptr_test, facet_handler_test, cache_handler_test, admin_handler_test, plus extensions to search_pipeline / response_formatter / server_stats / request_dispatcher / variable_handler / sync_handler tests.
…imeout config Batch of correctness fixes and dead-code removal identified in Phase 0 review: Bug fixes: - CR-1: Cast consumed offset to std::ptrdiff_t (was ssize_t) in ReactorConnection::ExtractFramesLocked to satisfy vector::iterator arithmetic requirements and prevent UB on the erase call. - CR-8: Register SYNC_STOP in ServerLifecycleManager::InitDispatcher alongside SYNC and SYNC_STATUS; omission caused "Unknown query type" for every "SYNC STOP <table>" client command despite the handler logic being fully implemented in SyncHandler. - H-N7: Remove RecordRequest() from all four /health/* handlers (HandleHealth, HandleHealthLive, HandleHealthReady, HandleHealthDetail) so orchestrator liveness/readiness probes do not inflate total_requests and distort application QPS metrics. New feature: - H-N8: Expose api.http.read_timeout_sec / write_timeout_sec as YAML- configurable fields. Added fields to ApiConfig::HttpConfig (config.h), ParseConfigFromJson (config.cpp), config-schema.json, and wired into HttpServerConfig::FromConfig. Non-positive values are ignored to keep the existing default (kHttpTimeoutSec = 5s). Dead-code removal: - H-N6: Delete TcpServer::StartSync, TcpServer::GetSyncStatus, and the accompanying friend class SyncHandler declaration; no caller remained after the SyncHandler refactor in the previous commit. - H-N6: Remove atomic<bool> shutdown_requested_ from TcpServer; the Stop() path never read it and reactor connections use their own shutdown signaling. - Drop unused constexpr kDefaultConnectionRecvTimeoutSec (tcp_server.cpp) and kSyncPollIntervalMs (sync_operation_manager.cpp). Startup completeness check (CR-8): - Add RequestDispatcher::HasHandler() query method. - Add a compile-time-sized array of all required QueryTypes in InitDispatcher and iterate it after registration; any unregistered entry returns kServerInitMissingDependency (fail-fast) to prevent silent handler-table drift when new QueryType enum values are added. Testing (6 new, 2 revised, all 2976 fast tests pass): - HasHandlerReturnsTrueForRegisteredTypes: pins positive/negative cases for RequestDispatcher::HasHandler. - Initialize_DispatcherRegistersAllSyncQueryTypes: regression test for SYNC/SYNC_STATUS/SYNC_STOP wiring (server_lifecycle_manager_test.cpp). - HttpTimeoutsParsedFromYaml: verifies read_timeout_sec/write_timeout_sec round-trip through LoadConfig (config_test.cpp). - FromConfigPropagatesHttpTimeouts / FromConfigIgnoresNonPositiveTimeouts: pin HttpServerConfig::FromConfig wiring (http_server_test.cpp). - HealthChecksAreNotCountedInTotalRequests: replaces HealthChecksTrackedSeparately; asserts zero delta on total_requests after hitting all four /health/* endpoints (health_endpoint_test.cpp). - Revise MultipleRequests in http_server_basic_test.cpp to use /info (counted) vs /health (not counted) for the request-counter assertion.
Lifecycle, dump, and replication: - Replace load()+store(true) in HandleDumpSave and HandleDumpLoad with compare_exchange_strong to close the TOCTOU window where two concurrent clients could both observe false and spawn duplicate workers. - Move dump_save_in_progress release into a ScopeGuard at the top of DumpSaveWorker so the flag becomes false strictly after Complete()/Fail() notifications; previously the explicit store(false) ran before Complete, leaving a window where a post-Complete observer saw the slot still busy. - Restructure TcpServer::Stop() into four ordered phases: (1) set shutdown_in_progress_ flag, (2) join dump worker, stop sync manager, and stop snapshot scheduler before the network stack tears down, (3) stop reactor then acceptor, (4) drain thread pool last so in-flight close callbacks can complete cleanly. - DumpSaveWorker checks shutdown_in_progress_ before calling binlog_reader_->Start() after a dump completes; skip restart when server is tearing down to avoid racing the binlog_reader destructor. - Document the synchronous contract of IBinlogReader::Stop() in Doxygen -- implementations must join their worker thread before returning; add new test binary binlog_reader_stop_contract_test verifying mock compliance. - Add HandlerContext::shutdown_flag (std::atomic<bool>*) and TcpServer::shutdown_in_progress_ to wire the signal from Stop() to workers. Multiplexer: - Rewrite KqueueMultiplexer::ApplyInterest to issue one kevent() syscall per change record instead of batching both EVFILT_READ and EVFILT_WRITE in a single call; track applied_interest incrementally so interest_ stays in lockstep with the kernel on partial failures where the first filter succeeds and the second fails. - Remove() also serialised per-filter to avoid the same partial-application hazard on teardown; ENOENT/EBADF are still silently tolerated for idempotent teardown. - Clarify thread-safety contracts in EventMultiplexer, KqueueMultiplexer, and EpollMultiplexer headers: Add/Modify/Remove are safe for concurrent callers; Poll is event-loop thread only. Cache: - Set max_load_factor(0.5) and reserve() in QueryCache constructor to suppress rehash under the steady-state working set; add comment explaining why iterator stability supports the shared_lock contract in LookupInternal. - Introduce QueryCache::EraseWithoutCallback; use it in InvalidationQueue::ProcessBatch and the immediate-erase path in Enqueue so UnregisterCacheEntry fires exactly once per affected key (via the queue), never via the eviction callback, closing the double-unregister race that could corrupt table_to_cache_keys_ / ngram_to_cache_keys_ auxiliary indexes on concurrent re-registration. - In CacheManager::Disable, store(false) on enabled_ before Clear() (was after); concurrent Insert observing the post-flip state now short- circuits at the enabled_ check and cannot re-populate the cache after Clear() completes; Clear body inlined with serialize_mutex_ to avoid the short-circuit in the public Clear() method. - In CacheManager::Enable, start the invalidation queue before setting enabled_=true so that an Invalidate() arriving immediately after Enable finds the queue already running. - InvalidationQueue::Start() resets stopped_=false before launching the worker thread so a Stop()/Start() cycle (Disable/Enable) does not leave stopped_ permanently set and silently drop all subsequent Enqueues. Tests: - New tests in tcp_server_lifecycle_test.cpp, binlog_reader_stop_contract_test.cpp, dump_handler_test.cpp, event_multiplexer_test.cpp, cache_manager_test, invalidation_queue_test, and query_cache_test cover the regressions above. - All 2997 fast tests pass (ctest -LE "SLOW|LOAD" -j 4).
Network hardening: - HttpServer::Start(): remove promise/future startup handshake and its join-deadlock window. bind_to_port is now called synchronously on the calling thread; the worker thread owns only the listen_after_bind accept loop. server_->wait_until_ready() ensures the accept loop is entered before Start() returns, so a racing Stop() cannot leak the thread. - IoReactor wakeup: add EventMultiplexer::Wake() virtual method with a default no-op. EpollMultiplexer implements it via eventfd(2) registered on the epoll instance during Open(); KqueueMultiplexer implements it via EVFILT_USER sentinel (ident kWakeIdent=0xFFFFFFFE). IoReactor::Stop() calls Wake() before joining the event-loop thread, so Poll() returns immediately instead of waiting up to poll_timeout_ms. Poll() drains/drops the wake event from its output vector so callers never observe it as a ready fd. - ConnectionAcceptor EMFILE/ENFILE backoff: replace std::this_thread::sleep_for with condition_variable::wait_for guarded by should_stop_. Stop() acquires stop_mutex_ to prevent the missed-notify race, then calls stop_cv_.notify_all(), so a sleeping accept loop returns in microseconds instead of up to 100 ms. - RateLimiter::GetStats() lock scope: hold mutex_ only for the client_buckets_.size() snapshot; atomic counters are read outside the lock. Total is derived as allowed + blocked rather than loaded from total_requests_ directly to preserve the invariant across concurrent fetch_add pairs without a wider critical section. - ReactorConnection write accounting: add underflow guard in DrainWriteQueueLocked around the write_queue_bytes_ decrement. Debug builds assert; release builds log the anomaly via structured log and clamp write_queue_bytes_ to sent_bytes before subtracting, keeping the connection live and the bug visible to operators. - IoReactor::Register rollback logging: change (void)mux_->Remove(fd) to a checked call; failure is logged at Error severity with the fd and error fields so operators can correlate kernel poll-set inconsistencies. Dump/sync shutdown correctness: - replication_pause_counter.h (NEW): process-wide atomic reference counter for replication-pause requests. RequestPause() returns true only on the 0->1 transition (first pauser calls binlog Stop()); ReleasePause() returns true only on the 1->0 transition (last releaser calls Start()). Header-only with a function-local static; includes ResetForTesting() for test fixtures. DumpSaveWorker, HandleDumpLoad, and SnapshotScheduler:: TakeSnapshot all migrated to use RequestPause()/ReleasePause() so concurrent operations cannot double-Stop() or prematurely Start() the binlog reader. - SyncOperationManager::StartSync() shutdown guard: check shutdown_requested_ (acquire) at the top of StartSync(). Returns kServerShuttingDown (new ErrorCode 6027) immediately, preventing a new SYNC from claiming the sync slot and spawning a worker that RequestShutdown would immediately cancel and race-join. RequestShutdown() updated to release-store shutdown_requested_ so the StartSync acquire fence sees it. - ErrorCode::kServerShuttingDown (6027) added to error.h with string table entry "Server is shutting down". Tests: - Regression tests added for all six fixes: HttpServer bind-on-caller- thread, IoReactor wakeup promptness, ConnectionAcceptor EMFILE shutdown response, RateLimiter lock-minimization consistency, dump_handler / snapshot_scheduler counter coordination, and SyncOperationManager shutdown rejection. - 3003/3004 tests pass; one failure (HttpServerStartupTest. StartOnOccupiedPortReturnsError) confirmed as a pre-existing port- collision flake in parallel ctest runs (three consecutive solo runs pass).
…ster Implement cache correctness and performance improvements (Phase 3 HIGH): Memory accounting (H-M1): - Add kSharedPtrControlBlockOverhead (24 B) to CacheEntry::MemoryUsage() so every compressed entry's control block is counted against the budget; addresses ~5-10% RSS under-report seen in fleet memory profiles - Add kHashMapNodeOverhead (32 B) constant in query_cache.cpp and apply it symmetrically in Insert, Erase, EraseWithoutCallback, and RemoveEntryLocked so total_memory_bytes_ tracks actual heap cost including map node headers - Adjust LRUEviction test kCacheBytes (3500 -> 4096) to account for the ~56 B per-entry increase from the two new constants Lock-order deadlock fix (H-M3): - RemoveEntryLocked no longer calls eviction_callback_ inline; instead it appends the key to an out-parameter evicted_keys vector - Insert, Erase, Clear, ClearTable, and RefreshLRU all collect evicted keys under mutex_ and call FireEvictionCallbacks() after releasing the lock, eliminating the QueryCache::mutex_ -> InvalidationManager::mutex_ ordering that conflicted with the reverse order taken by InvalidateAffectedEntries Batch eviction callback (H-M7): - Add BatchEvictionCallback type and SetBatchEvictionCallback() to QueryCache - Add FireEvictionCallbacks() helper that prefers BatchEvictionCallback over the per-key fallback when a bulk path fires - CacheManager wires SetBatchEvictionCallback to InvalidationManager::UnregisterCacheEntries so Clear/ClearTable/EvictForSpace/ RefreshLRU take InvalidationManager::mutex_ exactly once per bulk operation instead of once per key Batch unregister (H-M7): - Add InvalidationManager::UnregisterCacheEntries(const vector<CacheKey>&) that acquires mutex_ once and loops UnregisterCacheEntryUnlocked; idempotent on missing and duplicate keys filter_columns_changed O(k) scan (H-M2): - Replace O(N) full walk of cache_metadata_ with O(k) lookup via table_to_cache_keys_ reverse index in InvalidationManager::InvalidateAffectedEntries Stale LRU observability (H-M6): - EvictForSpace now emits query_cache_stale_lru structured log and bumps CacheStatistics::stale_lru_entries atomic counter when a key is present in lru_list_ but missing from cache_map_ - Add stale_lru_entries field to CacheStatisticsSnapshot and copy in GetStatistics() Timestamp double-set fix (M-15): - Remove redundant metadata.created_at / last_accessed assignment in CacheManager::Insert; QueryCache::Insert is the sole authoritative setter Schema-drift guard (M-16): - Add kCacheStatsFieldVersion constant and kExpectedCacheStatisticsSnapshotSize static_assert in query_cache.h to force reviewers to mirror field changes across CacheStatistics, CacheStatisticsSnapshot, and GetStatistics() Testing: - EvictionCallbackFiresWithoutHoldingCacheLock: re-entrant Lookup from inside eviction callback must not deadlock (H-M3 regression) - BatchEvictionCallbackInvokedOncePerBulkOp: Clear fires batch callback once with all keys; per-key callback suppressed (H-M7) - BulkPathFallsBackToPerKeyCallbackWhenBatchUnset: backward-compat check (H-M7) - EvictionCallbackCanCallGetMetadataWithoutDeadlock: GetMetadata from callback (H-M3) - StaleLruCounterStartsAtZeroAndStaysZeroUnderNormalOps: counter baseline (H-M6) - MemoryUsageIncludesSharedPtrControlBlockOverhead: control block accounting (H-M1) - UnregisterCacheEntriesRemovesBatch: batch/empty/duplicate key handling (H-M7) - FilterColumnsChangedRestrictedToTargetTable: O(k) scope correctness (H-M2) - All 197/197 cache tests pass (including SLOW)
Normalize StructuredLog call sites across the server, cache, and utils layers as part of the Phase 4A logging and error-handling unification: New constant header: - Add src/server/log_field_names.h with canonical field-name constants in namespace mygramdb::server::log_fields (kFieldFilepath, kFieldFd, kFieldClientIp, kFieldGtid, kFieldError, kFieldErrorCode, etc.) - Normalizes path/filepath -> kFieldFilepath, client_fd/connection_fd -> kFieldFd (int64_t), remote_addr -> kFieldClientIp across call sites Event name standardization (60+ sites): - Replace generic server_error / server_warning catch-all events with dedicated <module>_<verb>_<outcome> event names in: connection_acceptor (accept_failed, socket_bind_failed, unix_socket_*), dump_handler (dump_save_failed, dump_load_starting, dump_verify_*), admin_handler (config_help_failed, config_show_failed/warning), debug_handler (optimize_rejected), snapshot_scheduler, sync_operation_manager (sync_failed, sync_replication_start_failed, wait_all_sync_complete_timeout), http_server, tcp_server (tcp_server_start_failed, rate_limit_exceeded), thread_pool (thread_pool_non_graceful_shutdown, worker_thread_exception), rate_limiter, network_utils (invalid_cidr_entry) StructuredLog migration in cache layer: - Replace spdlog::warn() calls in invalidation_queue.cpp with StructuredLog events cache_invalidation_queue_enqueue_after_stop and cache_invalidation_queue_overflow Expected<void, Error> chain for IBinlogReader::Start(): - Replace bool return + GetLastError() pattern with Expected<void, Error> at 4 call sites in dump_handler.cpp (DUMP SAVE and DUMP LOAD restart paths) and replication_handler.cpp (REPLICATION START user request); errors now surface via FieldError() for consistent structured output Testing: - 3011/3012 tests pass (existing flake StartOnOccupiedPortReturnsError) - No new lint warnings beyond pre-existing baseline
…t boilerplate
Nine nearly-identical setsockopt() blocks in connection_acceptor.cpp
(each with an inline errno snapshot and a StructuredLog WARN) are
replaced by calls to a new socket_utils::TrySetSockOpt helper. The
two fatal listening-socket sites (SO_REUSEADDR, SO_KEEPALIVE on the
listener) remain inline with a comment explaining the deliberate split:
they require ERROR-level logs and an early return false, which the
non-fatal helper is not designed for.
Key changes:
- New module src/server/socket_utils.{h,cpp} in mygramdb_server
library; exposes a pointer+len overload and an int convenience overload
- connection_acceptor.cpp: -43 LOC; 9 per-client option sites converted
(SO_KEEPALIVE, TCP_KEEPIDLE/INTVL/CNT, macOS TCP_KEEPALIVE,
SO_NOSIGPIPE, SO_RCVBUF, SO_SNDBUF, TCP_NODELAY)
- Helper always includes the fd in the warning, a small operability
improvement over the previous inline calls that omitted it
- New tests/server/socket_utils_test.cpp: 5 cases covering int overload
success (with getsockopt verification), pointer overload success,
closed fd, invalid fd, and unsupported option on a Unix socket;
all 3017 fast-suite tests pass
Adds a move-only RAII helper around the process-wide replication-pause
counter (Phase 4 M-6) and refactors the three call sites in
snapshot_scheduler and dump_handler to use it. The wrapper guarantees
that an Acquire() is always paired with a counter decrement on scope
exit, eliminating the manual RequestPause + ScopeGuard{ReleasePause}
boilerplate and closing a leaked-counter risk in DUMP SAVE: that path
held the increment across the WriteDump call and only decremented
inline at the bottom of the worker, with no fallback if a future
refactor introduced an early return between the two points.
Key changes:
- New Scope class in replication_pause_counter.h: Acquire() returns the
first_pauser bool, Release() returns the last_releaser bool, dtor
drops a held-but-unreleased counter as a safety net (does NOT call
binlog Start; the explicit-release path owns that side-effect)
- Move-only ownership; double-Acquire / double-Release / Release-without-
Acquire are no-ops to defend against scope-misuse bugs
- snapshot_scheduler.cpp: scope replaces inline Request/ScopeGuard pair
- dump_handler.cpp DUMP SAVE: scope guards the pause across WriteDump
- dump_handler.cpp DUMP LOAD: scope guards both the success path and
the existing fallback ScopeGuard, which now Release()s the scope
cooperatively (the success branch's explicit Release marks the scope
spent so the fallback lambda's Release is a harmless no-op)
- New tests/server/replication_pause_counter_test.cpp: 10 cases covering
Acquire/Release ordering, dtor safety net, double-Acquire/Release
no-ops, move-construction ownership transfer, and the multi-scope
first/last-pauser pattern; full fast suite stays at 100% (3027/3027)
Adds a generic background-thread helper that runs a callback at a fixed interval until Stop() is called, and migrates two of MygramDB's hand- rolled std::condition_variable + std::thread + atomic<bool> trios onto it (Phase 4 M-8). Both migrated sites previously carried the same boilerplate inline: mutex_+cv_+stop flag, wait_for(predicate=stop), unlock-around-task, lock again, repeat. The shared helper: - centralises the "stop is fast" guarantee — Stop() notifies the cv so a worker sleeping on a long interval wakes within microseconds rather than waiting the full interval before the join() returns, - catches exceptions thrown by the callback and logs a structured warn (event=periodic_worker_task_failed) instead of letting them fall out of the worker thread and call std::terminate, and - emits start/stop/failure events under uniform names tagged with the worker's name so operators can correlate across modules. Migrated sites: - src/server/rate_limiter.cpp (SweeperLoop -> PeriodicWorker) - src/cache/query_cache.cpp (RefreshLRUWorker -> PeriodicWorker) Deferred sites and rationale (intentionally not part of this commit): - src/server/snapshot_scheduler.cpp: has additional outer state (next_save_time) and a Stop-ordering contract with start_stop_mutex_; retrofitting needs more invasive changes than the value delivered. - src/cache/invalidation_queue.cpp: not a fixed-interval periodic — it is a delay-aware queue drainer whose wait duration depends on queue contents. PeriodicWorker would be a poor fit. Tests: - New tests/utils/periodic_worker_test.cpp: 9 cases covering invalid inputs (empty task, zero interval, double-start), idle-Stop no-op, destructor stops a running worker, multi-tick firing, fast-Stop with long interval (<500ms vs 5s), throwing-callback resilience, and restart-after-stop. Marked SLOW (sleep-based timing). - Existing rate_limiter / query_cache fast + SLOW suites pass unchanged: 3027/3027 fast + 33/33 SLOW (rate_limiter + periodic).
…ease Phase 4 H-D1 unifies the four call sites that previously open-coded the "compare_exchange_strong(false, true) then bind an AtomicFlagResetGuard" pattern. The new utils::OperationGuard::TryAcquire() factory bundles both steps so that: - the test-and-set and the release happen as one logical operation, closing the well-known TOCTOU window where someone splits the compare_exchange and the guard binding (the exact bug class CR-2 fixed in DUMP LOAD), and - the new sites read identically across handlers, so a future reviewer can spot a missing "in-progress" guard at a glance. Two release semantics are exposed: - Release(): clear the flag and disengage. Use when this scope is the sole owner (DUMP LOAD success, OPTIMIZE, automatic snapshot). - Dismiss(): disengage WITHOUT clearing. Use when ownership transfers to a different release path — specifically, the DUMP SAVE async path hands the flag to the worker thread, whose own RAII reset clears it at the end. A unit test pins this contract; the initial roll-out of this commit collapsed both into Release() and broke the DUMP SAVE concurrency test (two threads slipped through), which is why the Dismiss() distinction is now load-bearing. Migrated sites: - src/server/handlers/dump_handler.cpp HandleDumpSave (uses Dismiss()) - src/server/handlers/dump_handler.cpp HandleDumpLoad (uses Release()) - src/server/handlers/debug_handler.cpp OPTIMIZE - src/server/snapshot_scheduler.cpp TakeSnapshot Tests: tests/utils/flag_guard_test.cpp grows by 11 OperationGuard cases covering engage/disengage paths, scope-exit release, explicit Release + Dismiss semantics, double-release safety, move-construction ownership transfer, and the disengaged-default-construction case. Full fast suite: 3038/3038 (was 3027/3027 before this batch).
…icWorker Both fixes close windows where a Stop()-side thread could observe or race against state written by a worker-side thread without holding the same lock. DumpProgress::StartWorker (server_types.h, dump_handler.cpp): - HandleDumpSave previously wrote worker_thread directly from the handler thread with no lock held. JoinWorker() (called by TcpServer::Stop() and ~DumpProgress) reads worker_thread under DumpProgress::mutex. A Stop() arriving concurrently with the HandleDumpSave assignment was therefore a data race on the unique_ptr by the C++ memory model. - New StartWorker(std::function<void()>) centralises the assignment inside DumpProgress::mutex. An assert guards the pre-condition that any prior worker has already been drained via JoinWorker(), making misuse loud rather than silently leaking a thread. PeriodicWorker post-unlock recheck (periodic_worker.cpp): - Loop() released mutex_ before calling task_(). A Stop() arriving in those few instructions would set should_stop_ and notify, but the worker had already committed to invoking task_() once more — one extra execution after shutdown was logically requested. - Added a should_stop_.load(acquire) check immediately after the unlock. Stop() cannot have set should_stop_ while the worker held the mutex, so the only race window is between the unlock and this load. The break is safe: unique_lock tracks ownership state, so the destructor will not double-unlock. Regression test (periodic_worker_test.cpp): - 200-iteration stress loop: sets a stop_requested flag immediately before Stop(), and asserts that no callback invocation observes the flag. With the original code, callbacks occasionally raced past the in-lock check and then fired inside the now-unlocked window; the recheck prevents that.
Four files had line-length and indentation violations detected by the CI clang-format check introduced after commit af39bea. No functional or logical changes. Affected files: - src/server/snapshot_scheduler.cpp: reformat ScopeGuard lambda capture and StructuredLog chained calls to fit within column limit - src/utils/periodic_worker.cpp: reformat StructuredLog chains in Start() validation paths; collapse a short chain in Loop() that was already under the limit back to a single line - tests/server/socket_utils_test.cpp: join split TrySetSockOpt call onto one line - tests/utils/periodic_worker_test.cpp: reflow two ASSERT_TRUE(worker .Start(...)) call sites to fit the column limit
Extend the JSON search endpoint with three new query options and harden
the build system for macOS and cleaner module boundaries.
HTTP search enhancements (src/server/http_server.cpp):
- ParseSortFromJson: accept {"sort": {"column": "...", "order": "ASC|DESC"}}
with safe column-name validation (IsSafeJsonColumnName, EqualsAsciiIgnoreCase)
- ParseFuzzyFromJson: accept {"fuzzy": 1|2} for edit-distance search
- ParseHighlightFromJson: accept {"highlight": {"open_tag", "close_tag",
"snippet_length", "max_fragments"}} with range validation
- BuildHighlightTerms: expand query terms through synonym dictionary and
normalize before passing to query::Highlighter::Generate
- HandleSearch: attach per-document "highlight" snippet to JSON results;
returns HTTP 400 when highlight is requested but text storage is disabled
- Fix filters type guard: previously accepted non-object values silently;
now returns HTTP 400 with "Field 'filters' must be an object"
Startup transactional safety (src/app/server_orchestrator.cpp,
src/server/tcp_server.cpp):
- ServerOrchestrator::Start is now fully transactional: HTTP server failure
stops both TcpServer and BinlogReader, leaving no background workers running
- TcpServer::Start stops reactor and acceptor on their own failure paths
- shutdown_in_progress_ is reset to false at the top of Start so restarts
after a previous Stop do not see a stale flag
Build system (CMakeLists.txt, Makefile, src/CMakeLists.txt,
src/config/CMakeLists.txt, src/server/CMakeLists.txt):
- Extract RuntimeVariableManager into a dedicated mygramdb_runtime_config
static library to break the circular dependency between mygramdb_config
and mygramdb_cache; consumers updated accordingly
- Remove duplicate top-level MySQL detection block; consolidate under
USE_MYSQL guard with mysql-client@8.4 preferred, fallback to mysql-client
- Change missing-MySQL from WARNING to FATAL_ERROR with -DUSE_MYSQL=OFF hint
- Makefile: replace $$(nproc) with portable NPROC variable (falls back to
sysctl -n hw.ncpu on macOS, then 4)
- Remove redundant mygramdb_server link from mygramdb executable
CMakeLists.txt cleanup across tests:
- runtime_variable_manager_test and runtime_variable_manager_mysql_test
link mygramdb_runtime_config instead of mygramdb_config + mygramdb_cache
- Remove redundant transitive link targets from tests/app, tests/index,
tests/loader, tests/server test executables
Tests (tests/server/http_server_search_test.cpp):
- SearchSupportsJsonSort: verifies ASC sort on a filter column
- SearchSupportsJsonFuzzy: verifies edit-distance=1 matches "machime" -> "machine"
- SearchSupportsJsonHighlight: verifies <strong> tags appear in snippet output
- SearchRejectsInvalidJsonFiltersType: verifies HTTP 400 for array-typed filters
…check Follow-up to the JSON search API additions. Two independent robustness fixes in ParseHighlightFromJson and IsSafeJsonColumnName. Highlight tag length cap (src/server/http_server.cpp): - Add kMaxHighlightTagLength = 256 constant - Reject open_tag / close_tag values longer than 256 bytes with HTTP 400 and a clear error message; prevents response-size amplification that could be triggered by a crafted request with a very long tag string Column-name predicate cleanup (IsSafeJsonColumnName): - Replace c == '$' with u == '$' (unsigned char) for consistency with surrounding comparisons and to avoid any signed-char UB hazard - Remove redundant std::isspace / std::iscntrl guards; all characters they would reject are already excluded by the ascii_safe whitelist Test (tests/server/http_server_search_test.cpp): - SearchRejectsOversizedHighlightTags: sends a 257-byte open_tag, expects HTTP 400 with an error mentioning "open_tag" and "at most 256 bytes"
- MYSQL_PORT: 13306 -> 23306 in conftest.py and docker-compose.yml - MYGRAMDB_HTTP_PORT: 18080 -> 20080 in conftest.py - MySQL and HTTP port updated in mygramdb-test.yaml to match Affected files: - e2e/conftest.py - e2e/docker/docker-compose.yml - e2e/docker/mygramdb-test.yaml
Add CHANGELOG section, release index entry, and full release notes for v1.6.1, a maintenance release covering lifecycle and concurrency hardening, HTTP search API extensions (sort/fuzzy/highlight), and a mygram-cli rewrite on top of MygramClient.
bm25_scorer.cpp lives in mygramdb_index and calls DocumentStore::GetNormalizedTextBatch from mygramdb_storage. GCC's left-to-right static-library scan resolved storage symbols before index was processed, leaving the cross-reference unresolvable and producing an undefined reference to the cxx11-ABI symbol on Linux CI. Listing mygramdb_index explicitly before mygramdb_storage restores the working link order; macOS clang tolerates the omission and now emits only a harmless duplicate-library warning.
binlog_event_processor.cpp directly calls cache::CacheManager::Invalidate and ClearTable, but mygramdb_mysql had no explicit link dependency on mygramdb_cache. GCC's left-to-right static-library scan resolved mygramdb_cache (pulled in transitively via mygramdb_server PUBLIC link) before reaching mygramdb_mysql, leaving binlog_event_processor.o's cache-symbol references unresolvable and causing binlog_reader_core_test and binlog_reader_events_test to fail on Linux CI with undefined reference errors. Adding the explicit dependency lets CMake order the libraries correctly; macOS clang had accepted the omission due to its more permissive symbol resolution.
…terministic Replace the 50µs sleep in ConcurrentPauseReleaseHasExactlyOneFirstAndLast with an atomic barrier that holds all 32 threads at the pause→release boundary until every thread has called RequestPause(), guaranteeing the counter traces 0→N→0 and eliminating false "multiple first/last" failures on coverage and sanitizer runners. Replace the hardcoded port 18091 in StartOnOccupiedPortReturnsError with port 0 so the OS assigns a free ephemeral port, then read it back via getsockname() to prevent bind() collisions with other tests running in parallel ctest sessions.
The previous assertion of exactly one success per round fails on coverage/sanitizer runners where the ~10ms in-memory dump completes before slow racing threads reach the compare_exchange, producing legitimate sequential 0->1->0->1->0 transitions and multiple successes. The test now verifies the true atomic invariant — all threads see a valid response, at least one wins per round, and the success count stays within bounds — and adds a Doxygen comment explaining why the stricter bound cannot be asserted without a worker-completion hook in the fixture.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release v1.6.1 — sizable maintenance release focused on lifecycle and concurrency hardening, HTTP search API extensions, and client/CLI correctness.
Highlights:
sort,fuzzy, andhighlightoptions aligned with the TCP protocolmygram-clinow delegates toMygramClient; eight independent client correctness bugs fixedmygramdb_indexlink forbm25_scorer_test; declaredmygramdb_cachedependency onmygramdb_mysql(fixed Linux GCC link-order issues)40 commits, 161 files changed, +17,382 / -3,621 LOC.
See docs/releases/v1.6.1.md for full details.
Test plan