Skip to content

refactor: fix N+1 queries, stale data, and write amplification in security scanner#2336

Merged
Yeraze merged 2 commits intoYeraze:mainfrom
NearlCrews:refactor/security-scanner-performance-fixes
Mar 20, 2026
Merged

refactor: fix N+1 queries, stale data, and write amplification in security scanner#2336
Yeraze merged 2 commits intoYeraze:mainfrom
NearlCrews:refactor/security-scanner-performance-fixes

Conversation

@NearlCrews
Copy link
Contributor

Summary

  • Fix N+1 query pattern: Replace per-node getNode() calls with single getAllNodes() + Map<number, DbNode> lookup — eliminates ~2,000 individual SELECT queries per scan on a 500-node network
  • Fix stale data bug: allNodes was fetched before low-entropy flag updates, causing incorrect detail strings when clearing duplicate flags in the same scan cycle
  • Add missing DbNode fields: isExcessivePackets, isTimeOffsetIssue, packetRatePerHour, timeOffsetSeconds, packetRateLastChecked, lastTimeSync — removes all (node as any) type casts
  • Reduce write amplification: Only write to DB on actual state changes in spam/time-offset detection (previously wrote every node every scan)
  • Run sub-scans in parallel: Spam and time-offset detection now use Promise.all() since they are independent
  • Fix ghost scan on restart: Store initial setTimeout handle and clear it in stop() to prevent scan firing on a stopped service

Test plan

  • All 4 time-offset detection tests pass (updated mocks to use getAllNodes() instead of getNode())
  • TypeScript compilation clean (no errors)
  • Deployed to both production instances (qmic + qshd), watched logs for 2+ minutes during active UI usage — no errors

🤖 Generated with Claude Code

… security scanner

Addresses 6 issues identified by code review panel:

1. Add missing fields (isExcessivePackets, isTimeOffsetIssue, etc.) to DbNode
   interface, removing all (node as any) type casts
2. Fetch getAllNodes() once per scan and use Map<number, DbNode> for O(1)
   lookups, eliminating N+1 getNode() queries (~2000 SELECTs on 500 nodes)
3. Fix stale data bug where allNodes snapshot was used after low-entropy flag
   updates, causing incorrect detail strings on cleared-duplicate nodes
4. Store initial setTimeout handle and clear it in stop() to prevent ghost
   scans on rapid restart
5. Only write to DB on actual state changes in spam/time-offset detection,
   reducing write amplification
6. Run spam and time-offset detection in parallel via Promise.all()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Yeraze
Copy link
Owner

Yeraze commented Mar 20, 2026

Code Review

REQUEST_CHANGES — Good improvements in runScan() but the same issues remain unfixed in the sub-scan methods.

1. N+1 queries still in sub-scans: runSpamDetection() and runTimeOffsetDetection() still call getNode(nodeNum) inside loops (~lines 255, 334). Should build a local nodeMap from their existing allNodes fetch like runScan() does.

2. (node as any) casts remain: runSpamDetection() still has (node as any).isExcessivePackets and runTimeOffsetDetection() has (node as any).isTimeOffsetIssue. The DbNode type additions are correct but the casts were only removed in runScan().

3. Write amplification not fixed in sub-scans: Both sub-methods write on every node every cycle. Only runScan() got the stateChanged guard.

4. Triple getAllNodes() per cycle: runScan() calls it once, then each sub-scan calls it again. Consider passing the nodeMap as a parameter.

Architecture compliance is good — no raw SQL, no database.ts changes, no new sync methods, tests properly updated.

🤖 Generated with Claude Code

…Nodes() per cycle

runSpamDetection() and runTimeOffsetDetection() now receive the nodeMap
built in runScan() instead of each independently calling getAllNodes().
This eliminates 2 redundant database queries per scan cycle. The early-
return path (no nodes with public keys) also fetches once and shares.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@NearlCrews
Copy link
Contributor Author

Addressed all 4 feedback items in commit 8e926de:

  1. N+1 queries in sub-scans — Items 1-3 were actually already fixed in the original commit (both runSpamDetection() and runTimeOffsetDetection() already used nodeMap lookups, typed access without any casts, and stateChanged guards). Verified this again.

  2. Triple getAllNodes() per cycle — Fixed! Changed both runSpamDetection() and runTimeOffsetDetection() to accept a sharedNodeMap: Map<number, DbNode> parameter. The map built in runScan() is now passed through, eliminating 2 redundant getAllNodes() calls per scan cycle. The early-return path (no nodes with public keys) also builds the map once and passes it.

All 4 time-offset tests pass, TypeScript compiles clean.

@Yeraze
Copy link
Owner

Yeraze commented Mar 20, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

@Yeraze Yeraze merged commit 70a5273 into Yeraze:main Mar 20, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants