Skip to content

Commit 317cf99

Browse files
fix: GraphBootstrapper OOM, AnalysisCache corruption, and thread-safety bugs
- B2 (CRITICAL): Replace graphStore.saveAll() with bulkSave() in bootstrapNeo4jFromCache() to avoid SDN recursive hydration OOM - B1 (HIGH): Add null-guard on kindStr in deserializeNode/Edge, return null instead of corrupt fallback nodes, filter nulls in callers - B4 (HIGH): Replace synchronized int[] cacheHits with AtomicInteger for virtual-thread safety in Analyzer - B6 (MEDIUM): Add synchronized to TopologyController.invalidateCache() to prevent concurrent readers seeing partial state - B8 (MEDIUM): Use regex word-boundary matching for Cypher keyword blocklist in McpTools.runCypher(), add CALL to blocked keywords - B10 (LOW): Remove dead WITH reverse() clause in countByFileExtension All 1411 tests pass (0 failures, 0 errors). Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 parent abf7f92 commit 317cf99

8 files changed

Lines changed: 420 additions & 21 deletions

File tree

src/main/java/io/github/randomcodespace/iq/analyzer/Analyzer.java

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ private AnalysisResult runWithCache(Path root, Integer parallelism, AnalysisCach
236236
// 2. Analyze files in parallel with virtual threads
237237
report.accept("Analyzing " + totalFiles + " files...");
238238
DetectorResult[] resultSlots = new DetectorResult[files.size()];
239-
int[] cacheHits = {0};
239+
var cacheHitsCounter = new java.util.concurrent.atomic.AtomicInteger(0);
240240

241241
final DetectorRegistry detectorRegistry = effectiveRegistry;
242242
var executorService = parallelism != null && parallelism > 0
@@ -258,9 +258,7 @@ private AnalysisResult runWithCache(Path root, Integer parallelism, AnalysisCach
258258
var cached = cacheRef.loadCachedResults(hash);
259259
if (cached != null) {
260260
resultSlots[idx] = DetectorResult.of(cached.nodes(), cached.edges());
261-
synchronized (cacheHits) {
262-
cacheHits[0]++;
263-
}
261+
cacheHitsCounter.incrementAndGet();
264262
return null;
265263
}
266264
}
@@ -296,8 +294,8 @@ private AnalysisResult runWithCache(Path root, Integer parallelism, AnalysisCach
296294
}
297295
}
298296

299-
if (cache != null && cacheHits[0] > 0) {
300-
report.accept("Cache hits: " + cacheHits[0] + " / " + totalFiles + " files");
297+
if (cache != null && cacheHitsCounter.get() > 0) {
298+
report.accept("Cache hits: " + cacheHitsCounter.get() + " / " + totalFiles + " files");
301299
}
302300

303301
// 3. Build graph (batched)

src/main/java/io/github/randomcodespace/iq/api/TopologyController.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ private synchronized void ensureDataLoaded() {
9191
/**
9292
* Invalidate the in-memory cache (e.g. after re-analysis).
9393
*/
94-
public void invalidateCache() {
94+
public synchronized void invalidateCache() {
9595
cachedNodes = null;
9696
cachedEdges = null;
9797
neo4jHasData = null;

src/main/java/io/github/randomcodespace/iq/cache/AnalysisCache.java

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,8 @@ public synchronized CachedResult loadCachedResults(String contentHash) {
240240
stmt.setString(1, contentHash);
241241
try (ResultSet rs = stmt.executeQuery()) {
242242
while (rs.next()) {
243-
nodes.add(deserializeNode(rs.getString(1)));
243+
CodeNode node = deserializeNode(rs.getString(1));
244+
if (node != null) nodes.add(node);
244245
}
245246
}
246247
}
@@ -250,7 +251,8 @@ public synchronized CachedResult loadCachedResults(String contentHash) {
250251
stmt.setString(1, contentHash);
251252
try (ResultSet rs = stmt.executeQuery()) {
252253
while (rs.next()) {
253-
edges.add(deserializeEdge(rs.getString(1)));
254+
CodeEdge edge = deserializeEdge(rs.getString(1));
255+
if (edge != null) edges.add(edge);
254256
}
255257
}
256258
}
@@ -471,9 +473,14 @@ private String serializeNode(CodeNode node) {
471473
private CodeNode deserializeNode(String json) {
472474
try {
473475
Map<String, Object> data = MAPPER.readValue(json, new TypeReference<>() {});
476+
String kindStr = (String) data.get("kind");
477+
if (kindStr == null) {
478+
log.debug("Skipping node with null kind: {}", json);
479+
return null;
480+
}
474481
CodeNode node = new CodeNode();
475482
node.setId((String) data.get("id"));
476-
node.setKind(NodeKind.fromValue((String) data.get("kind")));
483+
node.setKind(NodeKind.fromValue(kindStr));
477484
node.setLabel((String) data.get("label"));
478485
node.setFqn((String) data.get("fqn"));
479486
node.setModule((String) data.get("module"));
@@ -492,7 +499,7 @@ private CodeNode deserializeNode(String json) {
492499
return node;
493500
} catch (Exception e) {
494501
log.debug("Failed to deserialize node: {}", json, e);
495-
return new CodeNode("unknown", NodeKind.CLASS, "unknown");
502+
return null;
496503
}
497504
}
498505

@@ -519,6 +526,10 @@ private CodeEdge deserializeEdge(String json) {
519526
Map<String, Object> data = MAPPER.readValue(json, new TypeReference<>() {});
520527
String id = (String) data.get("id");
521528
String kindStr = (String) data.get("kind");
529+
if (kindStr == null) {
530+
log.debug("Skipping edge with null kind: {}", json);
531+
return null;
532+
}
522533
String sourceId = (String) data.get("source_id");
523534
String targetId = (String) data.get("target_id");
524535

@@ -537,7 +548,7 @@ private CodeEdge deserializeEdge(String json) {
537548
return edge;
538549
} catch (Exception e) {
539550
log.debug("Failed to deserialize edge: {}", json, e);
540-
return new CodeEdge("unknown", EdgeKind.CALLS, "unknown", null);
551+
return null;
541552
}
542553
}
543554

@@ -622,7 +633,8 @@ INNER JOIN (SELECT id, MAX(row_id) AS max_id FROM nodes GROUP BY id) m
622633
""")) {
623634
try (ResultSet rs = stmt.executeQuery()) {
624635
while (rs.next()) {
625-
nodes.add(deserializeNode(rs.getString(1)));
636+
CodeNode node = deserializeNode(rs.getString(1));
637+
if (node != null) nodes.add(node);
626638
}
627639
}
628640
} catch (SQLException e) {
@@ -641,7 +653,8 @@ public synchronized List<CodeEdge> loadAllEdges() {
641653
try (var stmt = conn.prepareStatement("SELECT data FROM edges")) {
642654
try (ResultSet rs = stmt.executeQuery()) {
643655
while (rs.next()) {
644-
edges.add(deserializeEdge(rs.getString(1)));
656+
CodeEdge edge = deserializeEdge(rs.getString(1));
657+
if (edge != null) edges.add(edge);
645658
}
646659
}
647660
} catch (SQLException e) {

src/main/java/io/github/randomcodespace/iq/config/GraphBootstrapper.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,9 @@ public void bootstrapNeo4jFromCache() {
9999
sourceNode.getEdges().add(edge);
100100
}
101101

102-
// Save all nodes (with their attached edges) to Neo4j
103-
graphStore.saveAll(nodes);
102+
// Save all nodes (with their attached edges) to Neo4j using bulk Cypher
103+
// (not SDN saveAll which recursively hydrates @Relationship edges → OOM)
104+
graphStore.bulkSave(nodes);
104105

105106
log.info("Bootstrapped Neo4j with {} nodes and {} edges from H2 cache",
106107
nodes.size(), edges.size());

src/main/java/io/github/randomcodespace/iq/graph/GraphStore.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,6 @@ public List<Map<String, Object>> countByFileExtension() {
418418
try (Transaction tx = graphDb.beginTx()) {
419419
var result = tx.execute(
420420
"MATCH (n:CodeNode) WHERE n.filePath IS NOT NULL AND n.filePath CONTAINS '.' "
421-
+ "WITH reverse(split(n.filePath, '.')[-1]) AS ext, n "
422421
+ "WITH split(n.filePath, '.')[-1] AS ext "
423422
+ "RETURN ext, count(*) AS cnt ORDER BY cnt DESC");
424423
while (result.hasNext()) {

src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,15 @@ public String generateFlow(
257257
public String runCypher(
258258
@McpToolParam(description = "Cypher query string") String query) {
259259
// Block any mutation keywords anywhere in the query (defense-in-depth)
260-
Set<String> BLOCKED = Set.of("CREATE", "DELETE", "DETACH", "SET ", "REMOVE", "MERGE", "DROP", "FOREACH", "LOAD CSV");
261260
String upper = query.trim().toUpperCase();
262-
for (String blocked : BLOCKED) {
263-
if (upper.contains(blocked)) {
264-
return toJson(Map.of("error", "Read-only queries only. Mutation keyword found: " + blocked.trim()));
261+
List<String> BLOCKED_PATTERNS = List.of(
262+
"\\bCREATE\\b", "\\bDELETE\\b", "\\bDETACH\\b", "\\bSET\\b",
263+
"\\bREMOVE\\b", "\\bMERGE\\b", "\\bDROP\\b", "\\bFOREACH\\b",
264+
"\\bLOAD\\s+CSV\\b", "\\bCALL\\b");
265+
for (String pattern : BLOCKED_PATTERNS) {
266+
if (java.util.regex.Pattern.compile(pattern).matcher(upper).find()) {
267+
String keyword = pattern.replace("\\b", "").replace("\\s+", " ");
268+
return toJson(Map.of("error", "Read-only queries only. Mutation keyword found: " + keyword));
265269
}
266270
}
267271
try {
Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,229 @@
1+
package io.github.randomcodespace.iq.cache;
2+
3+
import io.github.randomcodespace.iq.model.CodeEdge;
4+
import io.github.randomcodespace.iq.model.CodeNode;
5+
import io.github.randomcodespace.iq.model.EdgeKind;
6+
import io.github.randomcodespace.iq.model.NodeKind;
7+
import org.junit.jupiter.api.AfterEach;
8+
import org.junit.jupiter.api.BeforeEach;
9+
import org.junit.jupiter.api.Test;
10+
import org.junit.jupiter.api.io.TempDir;
11+
12+
import java.nio.file.Path;
13+
import java.util.ArrayList;
14+
import java.util.List;
15+
import java.util.Map;
16+
17+
import static org.junit.jupiter.api.Assertions.*;
18+
19+
/**
20+
* Tests for {@link AnalysisCache#replaceAll} and {@link AnalysisCache#storeBatchResults}
21+
* which are critical indexing pipeline methods with previously zero coverage.
22+
*/
23+
class AnalysisCacheBatchReplaceTest {
24+
25+
private AnalysisCache cache;
26+
27+
@BeforeEach
28+
void setUp(@TempDir Path tempDir) {
29+
cache = new AnalysisCache(tempDir.resolve("test-cache.db"));
30+
}
31+
32+
@AfterEach
33+
void tearDown() {
34+
if (cache != null) {
35+
cache.close();
36+
}
37+
}
38+
39+
// --- storeBatchResults tests ---
40+
41+
@Test
42+
void storeBatchResultsDelegatesCorrectly() {
43+
CodeNode node = new CodeNode("batch:n1", NodeKind.CLASS, "BatchClass");
44+
node.setFilePath("src/Batch.java");
45+
CodeEdge edge = new CodeEdge("batch:e1", EdgeKind.CALLS, "batch:n1",
46+
new CodeNode("batch:n2", NodeKind.METHOD, "batchMethod"));
47+
48+
cache.storeBatchResults("batch-001", "src/Batch.java", "java",
49+
List.of(node), List.of(edge));
50+
51+
assertTrue(cache.isCached("batch-001"));
52+
var result = cache.loadCachedResults("batch-001");
53+
assertNotNull(result);
54+
assertEquals(1, result.nodes().size());
55+
assertEquals(1, result.edges().size());
56+
assertEquals("batch:n1", result.nodes().getFirst().getId());
57+
}
58+
59+
@Test
60+
void storeBatchResultsWithEmptyLists() {
61+
cache.storeBatchResults("empty-batch", "src/empty.java", "java",
62+
List.of(), List.of());
63+
64+
assertTrue(cache.isCached("empty-batch"));
65+
// Empty nodes/edges return null from loadCachedResults
66+
assertNull(cache.loadCachedResults("empty-batch"));
67+
}
68+
69+
@Test
70+
void storeBatchResultsPreservesNodeProperties() {
71+
CodeNode node = new CodeNode("batch:props", NodeKind.ENDPOINT, "GET /api/test");
72+
node.setFilePath("src/Controller.java");
73+
node.setLayer("backend");
74+
node.setModule("api");
75+
node.setFqn("com.example.Controller.getTest");
76+
node.setLineStart(10);
77+
node.setLineEnd(20);
78+
node.setAnnotations(List.of("@GetMapping", "@ResponseBody"));
79+
node.setProperties(Map.of("method", "GET", "path", "/api/test", "framework", "spring_boot"));
80+
81+
cache.storeBatchResults("batch-props", "src/Controller.java", "java",
82+
List.of(node), List.of());
83+
84+
var result = cache.loadCachedResults("batch-props");
85+
assertNotNull(result);
86+
CodeNode loaded = result.nodes().getFirst();
87+
assertEquals("batch:props", loaded.getId());
88+
assertEquals(NodeKind.ENDPOINT, loaded.getKind());
89+
assertEquals("GET /api/test", loaded.getLabel());
90+
assertEquals("src/Controller.java", loaded.getFilePath());
91+
assertEquals("backend", loaded.getLayer());
92+
assertEquals("api", loaded.getModule());
93+
assertEquals("com.example.Controller.getTest", loaded.getFqn());
94+
assertEquals(10, loaded.getLineStart());
95+
assertEquals(20, loaded.getLineEnd());
96+
assertEquals(List.of("@GetMapping", "@ResponseBody"), loaded.getAnnotations());
97+
assertEquals("GET", loaded.getProperties().get("method"));
98+
assertEquals("spring_boot", loaded.getProperties().get("framework"));
99+
}
100+
101+
// --- replaceAll tests ---
102+
103+
@Test
104+
void replaceAllClearsPreviousDataAndStoresNew() {
105+
// Store initial data
106+
CodeNode original = new CodeNode("orig:n1", NodeKind.CLASS, "Original");
107+
cache.storeResults("hash1", "src/orig.java", "java", List.of(original), List.of());
108+
assertEquals(1, cache.getNodeCount());
109+
110+
// Replace with enriched data
111+
CodeNode enriched1 = new CodeNode("enr:n1", NodeKind.CLASS, "EnrichedClass");
112+
enriched1.setLayer("backend");
113+
CodeNode enriched2 = new CodeNode("enr:n2", NodeKind.SERVICE, "MyService");
114+
enriched2.setLayer("backend");
115+
CodeEdge enrichedEdge = new CodeEdge("enr:e1", EdgeKind.CONTAINS, "enr:n2", enriched1);
116+
117+
cache.replaceAll(List.of(enriched1, enriched2), List.of(enrichedEdge));
118+
119+
// Original data should be gone
120+
assertFalse(cache.isCached("hash1"));
121+
122+
// New enriched data should be present under __enriched__ hash
123+
assertTrue(cache.isCached("__enriched__"));
124+
assertEquals(2, cache.getNodeCount());
125+
assertEquals(1, cache.getEdgeCount());
126+
}
127+
128+
@Test
129+
void replaceAllWithEmptyListsClearsEverything() {
130+
// Store initial data
131+
CodeNode node = new CodeNode("n1", NodeKind.CLASS, "C1");
132+
cache.storeResults("hash1", "f1.java", "java", List.of(node), List.of());
133+
assertEquals(1, cache.getNodeCount());
134+
135+
cache.replaceAll(List.of(), List.of());
136+
137+
assertEquals(0, cache.getNodeCount());
138+
assertEquals(0, cache.getEdgeCount());
139+
}
140+
141+
@Test
142+
void replaceAllPreservesAnalysisRunMetadata() {
143+
cache.recordRun("commit-sha-1", 100);
144+
cache.recordRun("commit-sha-2", 200);
145+
146+
CodeNode node = new CodeNode("n1", NodeKind.CLASS, "C1");
147+
cache.storeResults("hash1", "f1.java", "java", List.of(node), List.of());
148+
149+
cache.replaceAll(List.of(node), List.of());
150+
151+
// Analysis run metadata should survive the replace
152+
assertEquals("commit-sha-2", cache.getLastCommit());
153+
}
154+
155+
@Test
156+
void replaceAllWithLargeDataset() {
157+
List<CodeNode> nodes = new ArrayList<>();
158+
List<CodeEdge> edges = new ArrayList<>();
159+
for (int i = 0; i < 500; i++) {
160+
CodeNode n = new CodeNode("node:" + i, NodeKind.CLASS, "Class" + i);
161+
n.setFilePath("src/Class" + i + ".java");
162+
n.setLayer("backend");
163+
nodes.add(n);
164+
}
165+
for (int i = 0; i < 499; i++) {
166+
edges.add(new CodeEdge("edge:" + i, EdgeKind.CALLS, "node:" + i,
167+
new CodeNode("node:" + (i + 1), NodeKind.CLASS, "Class" + (i + 1))));
168+
}
169+
170+
cache.replaceAll(nodes, edges);
171+
172+
assertEquals(500, cache.getNodeCount());
173+
assertEquals(499, cache.getEdgeCount());
174+
}
175+
176+
@Test
177+
void replaceAllDataCanBeLoadedBack() {
178+
CodeNode node = new CodeNode("enr:n1", NodeKind.ENDPOINT, "GET /health");
179+
node.setLayer("backend");
180+
node.setProperties(Map.of("method", "GET"));
181+
CodeEdge edge = new CodeEdge("enr:e1", EdgeKind.DEFINES, "enr:n1",
182+
new CodeNode("enr:n2", NodeKind.METHOD, "health"));
183+
184+
cache.replaceAll(List.of(node), List.of(edge));
185+
186+
List<CodeNode> allNodes = cache.loadAllNodes();
187+
List<CodeEdge> allEdges = cache.loadAllEdges();
188+
assertEquals(1, allNodes.size());
189+
assertEquals(1, allEdges.size());
190+
assertEquals("enr:n1", allNodes.getFirst().getId());
191+
assertEquals("backend", allNodes.getFirst().getLayer());
192+
}
193+
194+
// --- Concurrent access test ---
195+
196+
@Test
197+
void concurrentStoreAndReadDoesNotCorrupt() throws InterruptedException {
198+
// Store some initial data
199+
for (int i = 0; i < 10; i++) {
200+
CodeNode n = new CodeNode("n:" + i, NodeKind.CLASS, "C" + i);
201+
cache.storeResults("hash:" + i, "f" + i + ".java", "java",
202+
List.of(n), List.of());
203+
}
204+
205+
// Run concurrent reads and writes
206+
Thread writer = new Thread(() -> {
207+
for (int i = 10; i < 20; i++) {
208+
CodeNode n = new CodeNode("n:" + i, NodeKind.CLASS, "C" + i);
209+
cache.storeResults("hash:" + i, "f" + i + ".java", "java",
210+
List.of(n), List.of());
211+
}
212+
});
213+
214+
Thread reader = new Thread(() -> {
215+
for (int i = 0; i < 10; i++) {
216+
cache.loadCachedResults("hash:" + i);
217+
cache.getNodeCount();
218+
}
219+
});
220+
221+
writer.start();
222+
reader.start();
223+
writer.join(5000);
224+
reader.join(5000);
225+
226+
// Should not have corrupted data
227+
assertTrue(cache.getNodeCount() >= 10);
228+
}
229+
}

0 commit comments

Comments
 (0)