refactor: wrap all db usage in try/finally for reliable db.close()#384
refactor: wrap all db usage in try/finally for reliable db.close()#384carlos-alm merged 2 commits intomainfrom
Conversation
Replace manual db.close() before each return with a single try/finally block across all query functions. This ensures the database connection is always released, even if an unexpected error occurs mid-execution. Extends the pattern from #383 (sequence.js) to the rest of the codebase: queries.js (16 functions), embedder.js (3), dataflow.js (3), cfg.js (1), flow.js (2), structure.js (3), owners.js (1), triage.js (1), communities.js (1), ast.js (1), complexity.js (1), branch-compare.js (2). Impact: 42 functions changed, 163 affected
|
Claude finished @carlos-alm's task —— View job Reviewing PR: Database try/finally refactoringTodo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: SQLite connection resource leaks when functions exit early due to exceptions or early returns. The old pattern manually called Example from queryNameData(): // OLD: Manual db.close() before each return (❌ resource leak risk)
if (nodes.length === 0) {
db.close(); // Must remember to close before every return
return { query: name, results: [] };
}
// ... more code that could throw ...
db.close(); // Final close, but unreachable if exception occurs
// NEW: Guaranteed cleanup with try/finally (✅ always closes)
try {
if (nodes.length === 0) {
return { query: name, results: [] }; // Early return OK
}
// ... code that might throw ...
} finally {
db.close(); // Always executes, even on exceptions
}Why This Approach: Standard resource management pattern. Ensures database connections are always released regardless of how the function exits (normal return, early return, or exception). Risk Assessment: LOW RISK - This is a defensive programming improvement that reduces resource leaks without changing functionality. Phase 2: Configuration Change Detection ✅No high-risk configuration changes detected:
Phase 3: Test Integrity ✅No test weakening detected:
Phase 4: Code Quality Review ✅Excellent refactoring implementation: ✅ Pattern Consistency: Every modified function follows identical
✅ No Functional Changes: Logic preserved, only resource management improved No issues found:
Root Cause AnalysisProblem Being Solved: SQLite connection resource leaks when functions exit via exceptions or early returns Critical ConcernsNone identified. This is a textbook example of proper resource management refactoring. Final Recommendation
|
Greptile SummaryThis PR applies a consistent Key points:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant QueryFn as Query Function
participant DB as SQLite DB
Caller->>QueryFn: invoke
QueryFn->>DB: openReadonlyOrFail()
Note over QueryFn: try block begins
alt Happy path
QueryFn->>DB: db.prepare().all()
DB-->>QueryFn: rows
QueryFn->>DB: finally: db.close()
QueryFn-->>Caller: return result
else Early return guard
QueryFn->>DB: finally: db.close()
QueryFn-->>Caller: return empty result
else Unexpected exception thrown
QueryFn->>DB: finally: db.close()
QueryFn-->>Caller: propagate error
end
Last reviewed commit: c1d2695 |
|
Claude finished @carlos-alm's task —— View job Reviewing PR: Database try/finally refactoringTodo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: SQLite connection resource leaks when functions exit early due to exceptions or early returns. The old pattern manually called Example from queryNameData(): // OLD: Manual db.close() before each return (❌ resource leak risk)
if (nodes.length === 0) {
db.close(); // Must remember to close before every return
return { query: name, results: [] };
}
// ... more code that could throw ...
db.close(); // Final close, but unreachable if exception occurs
// NEW: Guaranteed cleanup with try/finally (✅ always closes)
try {
if (nodes.length === 0) {
return { query: name, results: [] }; // Early return OK
}
// ... code that might throw ...
} finally {
db.close(); // Always executes, even on exceptions
}Why This Approach: Standard resource management pattern. Ensures database connections are always released regardless of how the function exits (normal return, early return, or exception). Risk Assessment: LOW RISK - This is a defensive programming improvement that reduces resource leaks without changing functionality. Phase 2: Configuration Change Detection ✅No high-risk configuration changes detected:
Phase 3: Test Integrity ✅No test weakening detected:
Phase 4: Code Quality Review ✅Excellent refactoring implementation: ✅ Pattern Consistency: Every modified function follows identical
✅ No Functional Changes: Logic preserved, only resource management improved No issues found:
Root Cause AnalysisProblem Being Solved: SQLite connection resource leaks when functions exit via exceptions or early returns Critical ConcernsNone identified. This is a textbook example of proper resource management refactoring. Final Recommendation
|
1 participant
Summary
db.close()before each return with a singletry/finallyblock across all query functionssequence.js) to the rest of the codebase35 functions refactored across 12 files:
queries.jsqueryNameData,impactAnalysisData,moduleMapData,fileDepsData,fnDepsData,fnImpactData,pathData,diffImpactData,listFunctionsData,statsData,contextData,childrenData,explainData,whereData,rolesData,exportsData)embedder.jssearchData,multiSearchData,ftsSearchData)dataflow.jsdataflowData,dataflowPathData,dataflowImpactData)cfg.jscfgData)flow.jslistEntryPointsData,flowData)structure.jsstructureData,hotspotsData,moduleBoundariesData)owners.jsownersData)triage.jstriageData)communities.jscommunitiesData)ast.jsastQueryData)complexity.jscomplexityData)branch-compare.jsloadSymbolsFromDb,loadCallersFromDb)Intentionally unchanged:
_prepareSearchinembedder.js— returns the opendbhandle to callershybridSearchDataquickcheckDbopen/close — trivial inline patterniterListFunctions,iterRoles,iterWhere) — already usetry/finallyTest plan
db.close()in modified files is inside afinallyblock (except the 3 intentional exceptions above)