Skip to content

Harden SQlite#367

Merged
dphuang2 merged 4 commits intomainfrom
dhuang/pro-431-replace-peeweesqlite-with-tinydb
Dec 13, 2025
Merged

Harden SQlite#367
dphuang2 merged 4 commits intomainfrom
dhuang/pro-431-replace-peeweesqlite-with-tinydb

Conversation

@dphuang2
Copy link
Collaborator

@dphuang2 dphuang2 commented Dec 13, 2025


name: Pull Request
about: Propose changes to the codebase
title: "Brief description of changes"
labels: ''
assignees: ''


Description

Please include a summary of the change and which issue is fixed or feature is implemented. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)
Implements # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Refactoring/Code cleanup
  • Build/CI/CD related changes
  • Other (please describe):

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

  • Test A
  • Test B

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • My code follows the style guidelines of this project (ran black ., isort ., flake8 .)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Screenshots (if applicable)

If applicable, add screenshots to help showcase your changes.

Additional context

Add any other context about the PR here.


Note

Applies hardened SQLite settings and safe checkpoint-and-copy flows, adds corruption detection with auto-repair and CLI recovery, and introduces comprehensive concurrency/corruption tests.

  • SQLite Hardening:
    • Introduce SQLITE_HARDENED_PRAGMAS and apply WAL, busy timeouts, and related pragmas across databases.
    • Add corruption handling: check_and_repair_database, DatabaseCorruptedError, and _backup_and_remove_database.
    • Ensure directories exist; create tables with safe=True.
  • Agent/Resources:
    • SQLResource: use hardened connections and _checkpoint_and_copy_database for fork/checkpoint/restore to flush WAL before copying.
  • Event Bus & Dataset Logger:
    • SqliteEventBusDatabase and SqliteEvaluationRowStore: initialize with hardened pragmas, auto-repair on init, and safe table creation.
  • CLI:
    • logs_command: detect SQLite corruption, prompt for auto-fix (backup/reset), and retry startup; improve error handling.
  • Tests:
    • Add tests/test_sqlite_hardening.py covering pragmas, integrity checks, backup/removal, auto-repair on init, and concurrent reads/writes.
  • Config:
    • pyproject.toml: include tests in Pyright analysis; adjust excludes.

Written by Cursor Bugbot for commit f05a1ff. This will update automatically on new commits. Configure here.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: WAL mode not checkpointed before database file copy

The new code applies PRAGMA journal_mode=WAL on every connection via _apply_hardened_pragmas, but the fork, checkpoint, and restore methods use shutil.copyfile to copy only the main database file. In WAL mode, uncommitted transactions live in the separate -wal file. Copying just the main file without first running PRAGMA wal_checkpoint(TRUNCATE) or copying the WAL/SHM files can result in incomplete or inconsistent database copies, leading to data loss or corruption.

eval_protocol/agent/resources/sql_resource.py#L136-L137

shutil.copyfile(str(self._db_path), str(forked_resource._db_path))

eval_protocol/agent/resources/sql_resource.py#L150-L151

checkpoint_path = self._temp_dir / checkpoint_name
shutil.copyfile(str(self._db_path), str(checkpoint_path))

eval_protocol/agent/resources/sql_resource.py#L172-L173

shutil.copyfile(str(checkpoint_path), str(self._db_path))

Fix in Cursor Fix in Web


Dylan Huang added 2 commits December 13, 2025 01:29
…ors and improve corruption detection. Add tests to ensure valid databases are not deleted on non-corruption DatabaseErrors.
This update introduces a new function, _checkpoint_and_copy_database, which ensures that all data in the Write-Ahead Logging (WAL) file is flushed to the main database file before copying. This function is now utilized in the SQLResource class for database forking, checkpointing, and restoring operations, enhancing data integrity during these processes.
return True
raise DatabaseCorruptedError(db_path, e)
# For other DatabaseErrors (locks, busy, etc.), re-raise without deleting
raise
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Database connection not closed on exception

In check_and_repair_database, when a DatabaseError is raised after test_db.connect() succeeds (during execute_sql or fetchone), the test_db connection is never closed. The test_db.close() call at line 56 only executes in the happy path. If an exception occurs, the exception handler at lines 67-82 catches DatabaseError but doesn't close the connection, causing a resource leak. The try block lacks a finally clause to ensure cleanup.

Fix in Cursor Fix in Web

@dphuang2 dphuang2 merged commit c2f8e24 into main Dec 13, 2025
9 of 10 checks passed
@dphuang2 dphuang2 deleted the dhuang/pro-431-replace-peeweesqlite-with-tinydb branch December 13, 2025 10:02
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.

1 participant