fix(cluster): build a NodeInfo before calling NodeRegistry.register in RegisterNode - #3524
Open
AmirF194 wants to merge 1 commit into
Open
Conversation
…n RegisterNode ClusterServiceImpl.RegisterNode called NodeRegistry.register() with NodeInfo's own constructor keywords (name=, url=, capacity=, labels=, cell_ids=), but register() takes one positional NodeInfo. Every gRPC node registration raised TypeError. Build the NodeInfo first, matching the pattern POST /cluster/nodes and ClusterAuthGate.register_node already use. Fixes sipyourdrink-ltd#3500
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
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.
What
ClusterServiceImpl.RegisterNode(the gRPC entry point forbernstein.v1.ClusterService/RegisterNode) raisedTypeErroron every call.Why
NodeRegistry.register(node: NodeInfo) -> NodeInfotakes a single positionalNodeInfo. The gRPC handler instead called it withNodeInfo's own constructor keywords:TypeError: NodeRegistry.register() got an unexpected keyword argument 'name', unconditionally, for any node trying to join a cluster over gRPC. The two other call sites in the codebase (POST /cluster/nodesintask_cluster.py, andClusterAuthGate.register_nodeincluster_auth.py) already build aNodeInfofirst and pass it positionally;RegisterNodewas the one call site that skipped that step.How
Build the
NodeInfofrom the request fields, then pass it positionally, matching the two working call sites.Fixes #3500
Verification
TestClusterServiceImplRegisterNode::test_register_node_stores_a_real_node) fails on unmodifiedmainwith the exactTypeErrorabove, and passes on this branch, asserting the registered node's name, url, capacity fields, labels and cell_ids.uv run python scripts/run_tests.py -k grpc(13 tests) and-k cluster(13 files, 172 tests) pass. Did not run the full suite.ruff check src/,ruff format --check, andlint-importspass;pyrighton the changed file goes from 183 to 179 pre-existing errors (this file sits outsidepyrightconfig.strict.json's scope; the drop is the removed invalid-kwargs call).cluster_pb2stubs generated in this environment). The test callsClusterServiceImpl.RegisterNodedirectly with a fake request and a mockedcluster_pb2module.Checklist
uv run ruff check src/passesuv run pyright src/passes (pre-existing repo-wide backlog outsidepyrightconfig.strict.json, unrelated to this change)uv run python scripts/run_tests.py -xpasses (ran only-k grpcand-k cluster, see Verification)Documentation duty (every PR that touches a feature)
docs/operations/<area>.mdupdated (or N/A) - N/Adocs/api/schema regenerated if a public surface changed (or N/A) - N/A, no proto/schema changeuv run bernstein agents-md syncrun so AGENTS.md, CLAUDE.md,.goosehints,CONVENTIONS.md, and.cursor/rules/*.mdcreflect any new module (or N/A) - N/A, no new module