Skip to content

insipx/db integrity 2 corrupt classification - #4030

Open
insipx wants to merge 1 commit into
insipx/db-integrity-1-corefrom
insipx/db-integrity-2-corrupt-classification
Open

insipx/db integrity 2 corrupt classification#4030
insipx wants to merge 1 commit into
insipx/db-integrity-1-corefrom
insipx/db-integrity-2-corrupt-classification

Conversation

@insipx

@insipx insipx commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Stacked on #4029. A corrupted-but-correctly-keyed encrypted DB previously failed client-open as SqlCipherKeyIncorrect ("PRAGMA key or salt has incorrect value"), indistinguishable from a wrong key. Corruption-shaped validate failures now surface as the new non-retryable PlatformStorageError::DatabaseCorrupt; wrong-key behavior is unchanged, and the comment documents why the two remain ambiguous in general under plaintext headers. Includes a deterministic corruption regression test (truncate + byte-flip, 10/10) and a retryability assertion.

🤖 Generated with Claude Code

https://claude.ai/code/session_01V1AXHNsnQmHW174i5rRUEb

Note

Add DatabaseCorrupt variant to PlatformStorageError and detect corruption in EncryptedConnection::validate

  • Adds DatabaseCorrupt(String) to the PlatformStorageError enum and returns false from is_retryable() for this variant, classifying corruption as non-retryable.
  • Updates EncryptedConnection::validate in sqlcipher_connection.rs to inspect the PRAGMA/key validation error message; if it contains "malformed" or "corrupt" the error surfaces as DatabaseCorrupt, otherwise it remains SqlCipherKeyIncorrect.
  • Adds tests covering both the non-retryable classification and the corruption-vs-wrong-key distinction.
  • Behavioral Change: callers that previously saw SqlCipherKeyIncorrect for a corrupt database will now see DatabaseCorrupt; retry logic will no longer attempt retries on corruption errors.

Macroscope summarized 9859ebb.

@claude

claude Bot commented Aug 22, 2026

Copy link
Copy Markdown

Claude finished @insipx's task —— View job


Code Review (2026-08-27 15:47 UTC)

This PR effectively distinguishes database corruption from incorrect encryption keys. The implementation is clean and well-tested. A few observations:

Error Message Inspection (sqlcipher_connection.rs:274)

The corruption detection relies on string matching (contains("malformed") || contains("corrupt")). While pragmatic, this is fragile to SQLite/SQLCipher message changes. The comment on line 277-279 correctly acknowledges the inherent ambiguity under plaintext headers, documenting why both errors remain non-retryable.

Minor concern: The to_lowercase() conversion on line 273 could theoretically cause issues if SQLCipher uses locale-specific casing (unlikely, but worth noting). ASCII-specific matching might be more robust.

Test Coverage

The test at line 477 (corrupt_db_reports_corrupt_not_wrong_key) is excellent—deterministic corruption via truncate + byte-flip. This addresses the 1 missing line in codecov (likely a defensive error branch).

Retryability Classification

The is_retryable() implementation (native.rs:245) correctly marks DatabaseCorrupt as non-retryable. The test at line 1004 verifies this.

Performance & Security

No concerns. This is an error path change with no runtime overhead on the happy path.

Minor Nit

sqlcipher_connection.rs:272: The error log includes the full error from batch_execute. If the error message ever contained sensitive data (unlikely for a PRAGMA failure, but worth considering), this could leak into logs.


Summary: The implementation is sound. The string-matching detection is pragmatic given SQLite's error reporting. The comprehensive regression test and retryability assertion provide good coverage. No blocking issues.


@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from a57d7c8 to 4d80216 Compare August 22, 2026 14:56
@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from 4d80216 to 6143d45 Compare August 22, 2026 15:03
@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (insipx/db-integrity-1-core@3b41bb0). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...pted_store/database/native/sqlcipher_connection.rs 97.50% 1 Missing ⚠️
Additional details and impacted files
@@                      Coverage Diff                      @@
##             insipx/db-integrity-1-core    #4030   +/-   ##
=============================================================
  Coverage                              ?   85.96%           
=============================================================
  Files                                 ?      421           
  Lines                                 ?    68854           
  Branches                              ?        0           
=============================================================
  Hits                                  ?    59193           
  Misses                                ?     9661           
  Partials                              ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from 6143d45 to f2f9934 Compare August 22, 2026 15:28
@insipx
insipx marked this pull request as ready for review August 22, 2026 20:43
@insipx
insipx requested a review from a team as a code owner August 22, 2026 20:43
@macroscopeapp

macroscopeapp Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 991b609

Macroscope's review found this PR approvable — This is a small, self-contained fix that distinguishes corrupted encrypted databases from incorrect keys while preserving existing retry behavior. It changes only an error path, adds targeted regression coverage, and is fully within the author’s owned storage code.

No code changes detected at 9859ebb. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from f2f9934 to 991b609 Compare August 25, 2026 14:13
@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from 991b609 to bfc4447 Compare August 25, 2026 14:36
@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from bfc4447 to ddeebbc Compare August 27, 2026 14:37
@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from ddeebbc to 44e1945 Compare August 27, 2026 15:12
@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from 44e1945 to affbd83 Compare August 27, 2026 15:38
…lCipherKeyIncorrect

EncryptedConnection::validate mapped every key-test failure to
SqlCipherKeyIncorrect, so a corrupted-but-correctly-keyed database was
indistinguishable from a wrong key at client open. Corruption-shaped
failures (malformed/corrupt) now surface as the new non-retryable
PlatformStorageError::DatabaseCorrupt. Wrong-key behavior is unchanged;
the accompanying comment documents why the two remain ambiguous in
general (plaintext-header setup makes wrong-key failure shapes
data-dependent).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@insipx
insipx force-pushed the insipx/db-integrity-2-corrupt-classification branch from affbd83 to 9859ebb Compare August 27, 2026 15:46
@blacksmith-sh

This comment has been minimized.

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