Remove dead code identified by coverage analysis - #8425
Open
Amaury Chamayou (achamayou) wants to merge 2 commits into
Open
Amaury Chamayou (achamayou) wants to merge 2 commits into
Amaury Chamayou (achamayou) wants to merge 2 commits into
Conversation
Coverage run 35761555333 flagged several blocks of framework code with no callers, all left behind by earlier removals: - InternalTablesAccess::remove_member, set_recovery_threshold, is_recovery_participant, is_recovery_owner and is_active_member date from the pre-JS C++ governance actions and have no remaining callers. - The HTTP/2 server streaming API (start_stream/send_data/close_stream, stream close callbacks, the Streaming response state) and the unused http2::ClientParser were added for gRPC server streaming (#4537), whose only consumer was removed with gRPC support (#6572). - NetworkTables::get_all_*_tables tuple builders fed the auto-generated wrapper endpoints of the classic governance API, removed in #7080. - Store::operator== and untyped::Map::operator== are debugging helpers with no users. NullTxHistory is only used by unit tests, so it moves out of src/node/history.h into src/kv/test/null_tx_history.h. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
September 22, 2026 21:46
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review comments remain, and all reviewers assessed it as approval-ready.
Review effort: Lite
Findings: None
What changed in this PR
Removes confirmed dead framework code and relocates test-only transaction history stubs.
Changes:
- Removes obsolete governance, HTTP/2 streaming, table-builder, and equality helpers.
- Simplifies HTTP/2 handling to the active unary server path.
- Moves
NullTxHistoryutilities into KV tests.
| File | Change |
|---|---|
src/service/network_tables.h |
Removes unused table builders. |
src/node/rpc/test/frontend_test.cpp |
Updates test includes. |
src/node/rpc/test/frontend_test_infra.h |
Uses relocated history stub. |
src/node/rpc/http2_session.h |
Removes streaming APIs. |
src/node/internal_tables_access.h |
Removes obsolete governance helpers. |
src/node/history.h |
Removes test-only stubs. |
src/kv/untyped_map.h |
Removes unused equality helper. |
src/kv/test/null_tx_history.h |
Adds relocated history stubs. |
src/kv/test/kv_test.cpp |
Updates test includes. |
src/kv/store.h |
Removes unused equality helper. |
src/http/http2_types.h |
Removes streaming state and callbacks. |
src/http/http2_parser.h |
Simplifies unary server handling. |
src/http/http2_callbacks.h |
Simplifies response callbacks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
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.
Motivation
The Coverage workflow reports 77.2% line coverage on
main. A line-level pass over the report shows that roughly 750 of the ~15,000 missed lines are framework code with no callers at all, left behind by earlier feature removals. Deleting it makes the coverage numbers reflect code that can actually run, and removes maintenance surface. This is the first PR in a series to raise coverage; the follow-ups add tests.Implementation summary
Deleted, with the removal that orphaned each item:
InternalTablesAccess::remove_member,set_recovery_threshold,is_recovery_participant,is_recovery_owner,is_active_member: pre-JS C++ governance actions, superseded by constitution JS (remove_member/set_recovery_thresholdactions), no C++ callers remain.ServerParser::start_stream/send_data/close_stream/set_on_stream_close_callback,HTTP2StreamResponderandHTTP2ServerSessionforwarders, theStreamingresponse state and per-stream close callback) and the unusedhttp2::ClientParser: added for gRPC server streaming (gRPC server streaming #4537), whose only consumer went with gRPC support (Remove experimental gRPC support #6572).Parsernow always creates a server session.read_outgoing_callbackandrespond()are flattened since only the unary path remains; behaviour on that path is unchanged.NetworkTables::get_all_*_tables()tuple builders: fed the auto-generated wrapper endpoints of the classic governance API, removed in Remove deprecated ("classic") governance API #7080.Store::operator==anduntyped::Map::operator==: debugging helpers with no users (the only==on stores in tests compareshistorical::State, which compares pointers).Relocated:
NullTxHistory/NullTxHistoryPendingTxare test-only stubs, so they move fromsrc/node/history.htosrc/kv/test/null_tx_history.hnext to the other KV test stubs.Net: -925/+193 lines. No public headers under
include/ccfare touched, so no changelog entry.Safety and compatibility
No runtime behaviour change: every deleted function had zero call sites (verified by repo-wide search, and the build links with no fallout). The one live path edited is HTTP/2 unary responses, where the removed branches were unreachable (
Streamingwas never set);e2e_logging_http2passes locally. No wire formats, KV tables or APIs change, so mixed-version operation and recovery are unaffected.Validation: Debug
-DCOVERAGE=ONbuild, all 61unittests,e2e_logging_http2, andcpp-format/copyright/ascii/todochecks pass locally.