Fix for lib mrci#81
Closed
gabsow wants to merge 22 commits into
Closed
Conversation
Initial additions of the needed types and functions. (cherry picked from commit 95fc9cc)
(cherry picked from commit 37ea653)
(cherry picked from commit 5f0f0a9)
(cherry picked from commit c66873c)
(cherry picked from commit da3eeff)
…in steps (cherry picked from commit b252924)
… under internal commands (cherry picked from commit 597d0ac)
…s still have bugs (cherry picked from commit 58a15ab)
(cherry picked from commit f635c48)
(cherry picked from commit 7c5792a)
(cherry picked from commit d2cd1df)
(cherry picked from commit e64de66)
(cherry picked from commit f2e283d)
Avoid newer setuptools behavior in Linux and macOS workflows.
…o no need for the MR_ExecutionCtxSetDone() anymore
Commit 886d9ab introduced an early-return when MR_ExecutionInvokeCallback returns false (NULL callback). On non-initiator shards the done callback is never set, so both MR_RunExecution and MR_ExecutionTimedOutInternal bailed before completing termination—skipping NOTIFY_DONE and MR_FreeExecution respectively. This caused resource leaks that made Redis exit uncleanly under Valgrind. Restore master's behavior: invoke the callback (no-op if NULL), set it to NULL, and always continue the full termination flow. Co-authored-by: Cursor <cursoragent@cursor.com>
gabsow
force-pushed
the
fixForLibMRCI
branch
from
7f0d744 to
eeecfff
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // All nodes (shards), including myself, have answered | ||
| MR_ExecutionAddTask(e, MR_RunExecution, NULL); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused nodeIndex parameter in API function
Low Severity
The nodeIndex parameter of MR_SetInternalCommandResults is declared in the public API, passed by the caller (MR_OnDataResponseArrived passes node->index), but never referenced in the function body. The Node.index field was also added specifically for this purpose but provides no value currently. This dead parameter clutters the API and may indicate an incomplete implementation for correlating results with specific nodes.
Additional Locations (1)
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.
Note
High Risk
Touches cluster inter-node messaging and execution lifecycle, adding a new internal-command path that runs commands on the main thread and aggregates hiredis replies; bugs here could impact cluster stability or correctness.
Overview
Adds cluster-wide internal command executions to LibMR. Executions can now be built from an empty builder using
MR_CreateEmptyExecutionBuilder()and populated withMR_ExecutionBuilderInternalCommand()steps, distributed using aFUNCTION_ID_INTERNALflag, executed on recipients’ main thread (MR_ClusterExecuteInternalCommands), and aggregated via RESP-array replies parsed in the event loop (MR_SetInternalCommandResults) before completion.Refactors cluster messaging to distinguish status vs data responses, includes sending internal messages to
selfwhen broadcasting, assigns per-node indices for result routing, and centralizes/auth-retriesAUTHon connect/hello failures. Also fixes multiple API/typo issues and naming cleanup (e.g.,MR_ExecutionBuilderBuildAccumulate,MR_ClusterGetSlotByKey,MR_RecordInitialize,nReceived,array_trim, and Rust FFI binding update).Written by Cursor Bugbot for commit eeecfff. This will update automatically on new commits. Configure here.