[WEB-2706] fix: Fix issue with SQLite transactions #5934
Conversation
- Close DB handles on reload - Fix GET_ISSUES tracking
WalkthroughThe changes involve several modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
web/core/local-db/utils/load-issues.ts (3)
Line range hint 52-57: Fix nested transactions in updateIssue
The updateIssue function creates nested transactions by calling deleteIssueFromLocal and addIssue, which each have their own transaction blocks. This can lead to unexpected behavior with SQLite's transaction handling.
Consider refactoring to use a single transaction:
export const updateIssue = async (issue: TIssue & { is_local_update: number }) => {
if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return;
const issue_id = issue.id;
- // delete the issue and its meta data
- await deleteIssueFromLocal(issue_id);
- await addIssue(issue);
+ await persistence.db.exec("BEGIN;");
+ try {
+ const deleteQuery = `DELETE from issues where id='${issue_id}'`;
+ const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`;
+ await persistence.db.exec(deleteQuery);
+ await persistence.db.exec(deleteMetaQuery);
+ await stageIssueInserts(issue);
+ await persistence.db.exec("COMMIT;");
+ } catch (error) {
+ await persistence.db.exec("ROLLBACK;");
+ throw error;
+ }
};Line range hint 13-17: Add error handling to transaction blocks
Multiple functions use transactions without proper error handling, which could leave the database in an inconsistent state if an error occurs.
Consider wrapping all transaction blocks with try-catch and ROLLBACK. Example pattern:
export const addIssuesBulk = async (issues: any, batchSize = 100) => {
if (!rootStore.user.localDBEnabled || !persistence.db) return;
if (!issues.length) return;
const insertStart = performance.now();
- await persistence.db.exec("BEGIN;");
-
- for (let i = 0; i < issues.length; i += batchSize) {
- const batch = issues.slice(i, i + batchSize);
- // ... processing
- }
- await persistence.db.exec("COMMIT;");
+ try {
+ await persistence.db.exec("BEGIN;");
+ for (let i = 0; i < issues.length; i += batchSize) {
+ const batch = issues.slice(i, i + batchSize);
+ // ... processing
+ }
+ await persistence.db.exec("COMMIT;");
+ } catch (error) {
+ await persistence.db.exec("ROLLBACK;");
+ throw error;
+ }
};Apply similar error handling to addIssue, deleteIssueFromLocal, and stageIssueInserts functions.
Also applies to: 23-36, 42-49, 89-127
SQL Injection vulnerabilities found in multiple locations
Several instances of unsafe SQL query construction using string interpolation were identified:
-
In
load-issues.ts:const query = 'INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});'DELETE from issue_meta where issue_id='${issue_id}'DELETE from issues where id='${issue_id}'delete from issue_meta where issue_id='${issue_id}'
-
In
storage.sqlite.ts:DELETE FROM options where key='${key}'
-
In
load-workspace.ts:INSERT OR REPLACE INTO ${table} (${columns}) VALUES (${values});
While some queries in the codebase use proper parameterization with bind parameters (e.g., the issue_meta insertions), many critical queries are vulnerable to SQL injection. The code should be updated to consistently use parameterized queries with bind parameters throughout.
Example fix for the issues table insert:
const placeholders = Array(Object.values(sanitizedIssue).length).fill('?').join(',');
const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${placeholders});`;
await persistence.db.exec({
sql: query,
bind: Object.values(sanitizedIssue)
});🔗 Analysis chain
Line range hint 89-127: Verify SQL injection protection in stageIssueInserts
The function constructs SQL queries by directly interpolating values, which could be vulnerable to SQL injection.
Let's check for other instances of unsafe SQL construction:
Consider using parameterized queries consistently:
- const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});`;
- await persistence.db.exec(query);
+ const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${placeholders});`;
+ await persistence.db.exec({
+ sql: query,
+ bind: Object.values(sanitizedIssue)
+ });🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct string interpolation in SQL queries
rg -g '*.ts' "INSERT.*\$\{|DELETE.*\$\{|UPDATE.*\$\{|SELECT.*\$\{"
Length of output: 64
Script:
#!/bin/bash
# Let's try a different approach to find SQL queries with string interpolation
# Search for SQL keywords followed by any string interpolation
rg -g '*.ts' -A 2 -B 2 '`.*(?:INSERT|DELETE|UPDATE|SELECT).*\$\{.*`|`.*\$\{.*(?:INSERT|DELETE|UPDATE|SELECT).*`'
# Also search specifically in the load-issues.ts file
cat "web/core/local-db/utils/load-issues.ts"
Length of output: 7774
web/core/local-db/utils/load-workspace.ts (2)
Line range hint 117-124: Fix typo in variable name
There's a typographical error in the variable name "memebers" which should be "members".
Apply this fix:
- const [labels, modules, cycles, states, estimates, memebers] = await Promise.all(promises);
+ const [labels, modules, cycles, states, estimates, members] = await Promise.all(promises);
await persistence.db.exec("BEGIN;");
await batchInserts(labels, "labels", labelSchema);
await batchInserts(modules, "modules", moduleSchema);
await batchInserts(cycles, "cycles", cycleSchema);
await batchInserts(states, "states", stateSchema);
await batchInserts(estimates, "estimate_points", estimatePointSchema);
- await batchInserts(memebers, "members", memberSchema);
+ await batchInserts(members, "members", memberSchema);Line range hint 117-126: Add error handling for transaction
The transaction lacks error handling which could leave the database in an inconsistent state if any batch operation fails.
Consider wrapping the transaction in a try-catch block:
const [labels, modules, cycles, states, estimates, members] = await Promise.all(promises);
const start = performance.now();
+ try {
await persistence.db.exec("BEGIN;");
await batchInserts(labels, "labels", labelSchema);
await batchInserts(modules, "modules", moduleSchema);
await batchInserts(cycles, "cycles", cycleSchema);
await batchInserts(states, "states", stateSchema);
await batchInserts(estimates, "estimate_points", estimatePointSchema);
await batchInserts(members, "members", memberSchema);
await persistence.db.exec("COMMIT");
+ } catch (error) {
+ await persistence.db.exec("ROLLBACK");
+ throw error;
+ }
const end = performance.now();web/core/local-db/storage.sqlite.ts (1)
306-308: Simplify arrow function
The arrow function block can be simplified for better readability.
- [issuesRaw, count] = await startSpan({ name: "GET_ISSUES" }, async () => {
- return await Promise.all([runQuery(query), runQuery(countQuery)]);
- });
+ [issuesRaw, count] = await startSpan({ name: "GET_ISSUES" }, async () =>
+ Promise.all([runQuery(query), runQuery(countQuery)])
+ );🧰 Tools
🪛 GitHub Check: lint-web
[failure] 306-306:
Unexpected block statement surrounding arrow body; move the returned value immediately after the =>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/core/local-db/storage.sqlite.ts (2 hunks)
- web/core/local-db/utils/load-issues.ts (1 hunks)
- web/core/local-db/utils/load-workspace.ts (1 hunks)
- web/core/services/issue/issue.service.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-web
web/core/local-db/storage.sqlite.ts
[failure] 306-306:
Unexpected block statement surrounding arrow body; move the returned value immediately after the =>
🔇 Additional comments (5)
web/core/local-db/utils/load-issues.ts (1)
20-20: LGTM: Efficient handling of empty arrays
The early return for empty arrays prevents unnecessary transaction overhead.
web/core/local-db/utils/load-workspace.ts (1)
118-118: LGTM: Simplified transaction syntax
The change to use BEGIN; instead of BEGIN TRANSACTION; is correct and follows SQLite's standard syntax.
web/core/services/issue/issue.service.ts (1)
74-75: LGTM! Simplified performance tracking.
The removal of Sentry's startSpan wrapper improves code clarity while maintaining functionality. This change aligns with the PR objective to fix GET_ISSUES measurement.
Let's verify that performance tracking is properly handled in the persistence layer:
✅ Verification successful
Performance tracking properly moved to persistence layer
The removal of Sentry's startSpan from issue.service.ts is justified as the persistence layer (storage.sqlite.ts) now handles performance tracking using the native Performance API, as evidenced by:
- Performance measurement starts at line
const start = performance.now(); - Performance measurement ends at line
const end = performance.now(); - The tracking is implemented at the database query level, which is more accurate for measuring actual data retrieval time
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify performance tracking in persistence layer
# Expected: Find performance tracking implementation in storage layer
# Check for Sentry span usage in storage implementation
rg -A 5 "getIssues.*startSpan" "web/core/local-db/"
Length of output: 51
Script:
#!/bin/bash
# Let's broaden our search to find the persistence layer implementation
# First, find where getIssues is implemented
ast-grep --pattern 'async getIssues($$$) { $$ }'
# Also check for any performance tracking related imports or usage
rg -l "startSpan|performance|measure" "web/core/"
Length of output: 380
Script:
#!/bin/bash
# Let's check the implementation in storage.sqlite.ts and load-issues.ts which seem most relevant
rg -A 10 "getIssues" "web/core/local-db/storage.sqlite.ts" "web/core/local-db/utils/load-issues.ts"
# Also check for any performance measurement implementation in these files
rg -A 5 "performance\.mark|performance\.measure" "web/core/local-db/storage.sqlite.ts" "web/core/local-db/utils/load-issues.ts"
Length of output: 4000
web/core/local-db/storage.sqlite.ts (2)
Line range hint 306-314: Improved error handling and performance tracking
The changes properly handle errors by falling back to server requests and include comprehensive performance tracking with Sentry spans.
🧰 Tools
🪛 GitHub Check: lint-web
[failure] 306-306:
Unexpected block statement surrounding arrow body; move the returned value immediately after the =>
Line range hint 1-1: Verify transaction management implementation
The PR objective mentions fixing "transaction within transaction" issue, but the transaction management code isn't visible in this file. Please ensure the fix is implemented in the worker code.
Let's verify the transaction implementation:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/core/local-db/worker/db.ts (2)
Line range hint
114-127: Enhance connection cleanup for worker termination scenarios.While the
close()method exists, consider adding automatic cleanup for unexpected worker termination scenarios to prevent resource leaks. This aligns with the PR objective to "Close DB handles on reload".Add a cleanup handler:
+ constructor() { + // Ensure DB is closed when worker is terminated + self.addEventListener('beforeunload', async () => { + await this.close(); + }); + }
Transaction management needs improvement for reliability and error handling
The verification confirms the need for a robust transaction manager. Current implementation shows several issues:
- No ROLLBACK handling in application code despite transactions being used extensively
- Complex promise-based queue system in
db.tsthat only handles "BEGIN;" string literal- Multiple transaction blocks in different utilities without proper error handling
- No nested transaction support despite SQLite's SAVEPOINT capability being available
- Transactions are used for bulk operations but lack proper cleanup on failure
Key locations requiring attention:
web/core/local-db/worker/db.ts: Core transaction handling logicweb/core/local-db/utils/load-issues.ts: Multiple transaction blocks without error handlingweb/core/local-db/utils/load-workspace.ts: Bulk operation transaction without safety netsweb/core/local-db/utils/tables.ts: Table creation transaction without rollback capability🔗 Analysis chain
Line range hint
65-108: Consider implementing a robust transaction manager.The current transaction handling using a promise queue is complex and fragile. Consider implementing a dedicated transaction manager class that:
- Properly tracks transaction states
- Handles nested transactions using SQLite's SAVEPOINT
- Provides clear transaction boundaries and rollback capabilities
Let's verify the current transaction usage patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for different transaction start patterns that might be incompatible echo "Checking transaction patterns in the codebase:" rg -i "begin\s*(transaction)?\s*;" --type ts # Check for potential nested transactions echo -e "\nChecking for potential nested transactions:" ast-grep --pattern 'BEGIN' -l ts | xargs -I {} rg -A 10 'BEGIN' {} | grep -i 'begin'Length of output: 867
Script:
#!/bin/bash # Let's analyze the transaction patterns more comprehensively # Check for COMMIT patterns echo "Checking COMMIT patterns:" rg -i "commit\s*;" --type ts # Check for ROLLBACK patterns echo -e "\nChecking ROLLBACK patterns:" rg -i "rollback\s*;" --type ts # Check the context around BEGIN statements to understand the transaction flow echo -e "\nAnalyzing transaction flow context:" for file in $(rg -l "BEGIN;" --type ts); do echo -e "\nFile: $file" rg -B 5 -A 15 "BEGIN;" "$file" done # Check if there are any SAVEPOINT usages echo -e "\nChecking for SAVEPOINT usage:" rg -i "savepoint" --type tsLength of output: 5929
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/core/local-db/utils/load-workspace.ts(1 hunks)web/core/local-db/worker/db.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/local-db/utils/load-workspace.ts
🔇 Additional comments (1)
web/core/local-db/worker/db.ts (1)
39-44: LGTM: Proper database initialization parameters.
The explicit parameters for open_v2 are correct and align with best practices:
OPEN_READWRITE | OPEN_CREATEensures proper access mode- VFS name "plane" matches the registered virtual file system
| // @todo this will fail if the transaction is started any other way | ||
| // eg: BEGIN, OR BEGIN TRANSACTION |
There was a problem hiding this comment.
Critical: Transaction syntax limitations could cause failures.
The code only handles "BEGIN;" but not other valid SQLite transaction start commands like "BEGIN TRANSACTION;" or "BEGIN". This limitation could lead to transaction deadlocks or failures, especially when transactions are nested.
Consider updating the condition to handle all valid transaction start syntaxes:
- if (props === "BEGIN;") {
+ if (typeof props === "string" &&
+ (props === "BEGIN;" ||
+ props === "BEGIN" ||
+ props === "BEGIN TRANSACTION;")) {Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor