[Refactor] Production readiness: thread-safety, performance, and testing - #24
Merged
Conversation
- Fix DFS/BFS thread-safety and add SearchContext improvements - Remove deprecated vertex fields, add thread-safety documentation - Add 34 production tests (scaling, timing, determinism, concurrency) Test count: 268 -> 302 (all passing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive production readiness improvements for the libgraph search library, focusing on thread-safety, performance optimization, and extensive testing to prepare the codebase for deployment in robotics navigation systems.
Key Changes:
- Fixed critical thread-safety issues in DFS (timestamp counter moved to SearchContext) and BFS (replaced priority_queue with std::queue for true O(1) FIFO)
- Removed 48 bytes of deprecated search-related fields per vertex, improving memory efficiency and thread safety
- Added 34 new production-grade tests covering performance scaling, timing bounds, determinism, and concurrent stress scenarios (test count: 268 → 302)
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
include/graph/search/dfs.hpp | Moved timestamp counter from strategy class to SearchContext for thread-safety |
include/graph/search/bfs.hpp | Replaced priority_queue with std::queue for true BFS O(1) operations |
include/graph/search/search_context.hpp | Added context-level attributes, start vertex tracking, and configurable pre-allocation |
include/graph/search/search_strategy.hpp | Updated interface to pass SearchContext to InitializeVertex/RelaxVertex methods |
include/graph/search/search_algorithm.hpp | Added thread-safety documentation and start vertex tracking |
include/graph/search/astar.hpp, dijkstra.hpp | Updated to new SearchStrategy interface with thread-safety docs |
include/graph/vertex.hpp | Removed deprecated search fields (is_checked, g_cost, etc.) |
include/graph/graph.hpp | Removed deprecated ResetAllVertices() method |
include/graph/impl/graph_impl.hpp | Added atomicity to AddUndirectedEdge, fixed const_cast in AddEdgeWithResult |
include/graph/impl/vertex_impl.hpp | Removed ClearVertexSearchInfo() implementation |
tests/unit_test/timing_bounds_test.cpp | New: 8 tests for latency distribution and adversarial graph scenarios |
tests/unit_test/performance_scaling_test.cpp | New: 9 tests for construction and search scaling up to 100K vertices |
tests/unit_test/determinism_test.cpp | New: 9 tests for 1000-run determinism validation across algorithms |
tests/unit_test/concurrent_stress_test.cpp | New: 8 tests for concurrent searches with up to 200 threads |
tests/unit_test/dfs_comprehensive_test.cpp | Added 3 tests for concurrent DFS and context attribute management |
tests/unit_test/bfs_comprehensive_test.cpp | Added 3 tests for queue-based BFS correctness and scaling |
tests/unit_test/search_context_comprehensive_test.cpp | Added 3 tests for start vertex tracking and pre-allocation |
tests/unit_test/graph_mod_test.cpp | Added test for AddUndirectedEdge atomicity |
tests/unit_test/pq_with_graph_test.cpp | Updated to use element values instead of deprecated vertex g_cost |
tests/unit_test/vertex_independent_test.cpp | Removed test for deprecated VertexSearchInfoManagement |
tests/CMakeLists.txt | Added 4 new production test files to build |
README.md | Enhanced thread-safety documentation with explicit model |
TODO.md | Updated with production readiness completion status |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Core fixes: - Fix iterator invalidation: Edge and vertices_from now use stable Vertex* pointers instead of iterators that invalidate on unordered_map rehash - Deterministic tie-breaking: priority queue comparators break ties by vertex_id for reproducible search results across runs - Debug assertions: validate non-negative, finite costs in debug builds (SFINAE-based for C++11 compatibility) New features: - PathResult<State, Cost>: rich search result with path, total_cost, nodes_expanded via SearchAlgorithm::SearchWithResult() - SearchLimits: early termination via max_expansions or timeout_ms via SearchAlgorithm::SearchWithLimits() - MultiGoalResult: find path to nearest goal via SearchMultiGoal() with goal_vertex_id and goal_index in result - Nodes expanded counter: context.GetNodesExpanded() for diagnostics Tests: - 14 new tests for production features - All 317 tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add ASAN, UBSAN, TSAN sanitizer options to CMakeLists.txt - Add sanitizer CI jobs (ASAN+UBSAN, TSAN separately - mutually exclusive) - Fix null pointer dereference in A* multi-goal search (UBSAN detected) - Added is_multi_goal_search_ flag to SearchContext - A* strategy now checks flag before dereferencing goal iterator - Update README.md with production features section - Create CHANGELOG.md with unreleased changes - Update test count to 317 in README 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tests that intentionally trigger data races to document unsafe patterns (ConcurrentEdgeAdditions, MixedReadWriteOperations) are now skipped when running under TSAN since TSAN will detect the races and abort. The RUNNING_UNDER_TSAN macro detects both GCC/Clang TSAN instrumentation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Test count: 268 -> 302 (all passing)
🤖 Generated with Claude Code