Skip to content

fix(sqlite-vfs): use delete range for truncate cleanup#4636

Open
NathanFlurry wants to merge 1 commit into04-12-fix_sqlite-native_restore_kv_error_hookfrom
04-12-fix_sqlite-vfs_use_delete_range_for_truncate_cleanup
Open

fix(sqlite-vfs): use delete range for truncate cleanup#4636
NathanFlurry wants to merge 1 commit into04-12-fix_sqlite-native_restore_kv_error_hookfrom
04-12-fix_sqlite-vfs_use_delete_range_for_truncate_cleanup

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • 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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my 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

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 13, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

PR Review: fix(sqlite-vfs): use delete range for truncate cleanup

This is a clean, targeted optimization that replaces manual chunk enumeration with a range delete in the SQLite VFS truncate path. Both implementations maintain parity per the CLAUDE.md requirement.

What the change does

Before: For truncation, the code built a list of individual chunk keys from last_chunk_to_keep + 1 to last_existing_chunk, then deleted them in batches of KV_MAX_BATCH_KEYS.

After: A single kv_delete_range / deleteRange call from the first chunk to delete up to get_chunk_key_range_end(file_tag) (the exclusive upper bound for all chunks of this file).

Correctness

Range boundary is safe. get_chunk_key_range_end(file_tag) returns [SQLITE_PREFIX, SCHEMA_VERSION, CHUNK_PREFIX, file_tag + 1], which is lexicographically strictly greater than any 8-byte chunk key for file_tag and strictly less than any key for file_tag + 1. No keys from adjacent file tags can be deleted.

Semantically broader than old code, but intentionally so. The old code was bounded by last_existing_chunk (derived from file.size). The new code deletes everything from the truncation point through the end of the file tag's key namespace. This is actually more robust: it cleans up any orphaned chunks beyond what file.size tracks (e.g., from a prior crash). This matches the behavior already used in delete_file.

Zero-size truncation works correctly. When size == 0, last_chunk_to_keep == -1, so the start key is get_chunk_key(file_tag, 0) — all chunks are deleted. In Rust, the cast (-1i64 + 1) as u32 == 0u32 is correct.

Guard condition is correct. The last_chunk_to_keep < last_existing_chunk guard (added in both files) prevents a spurious delete call when no chunks need removal.

Minor observations

Removed comment in TypeScript. The old code had // Delete chunks beyond the new size which explained the loop's purpose. The new code is simple enough to be self-evident, but a brief comment on the deleteRange call (similar to the Rust doc comment on get_chunk_key_range_end) could be helpful for future readers. Optional callout, not a blocker.

Pre-existing overflow concern (not introduced here). In Rust, get_chunk_key_range_end does file_tag + 1 on a u8. If file_tag == 255, this overflows in debug mode. This same function is already used in delete_file, so it's pre-existing and out of scope for this PR.

No test changes. Correctness of range delete vs. batch delete is implementation-level; the observable behavior is identical. Existing truncation tests should cover this. No new tests needed solely for this refactor.

Summary

Straightforward improvement: fewer allocations, fewer round-trips for large files, parity maintained between Rust and TypeScript. The logic is correct and the approach is consistent with how delete_file already works. Looks good to me.

@NathanFlurry NathanFlurry marked this pull request as ready for review April 13, 2026 05:16
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_restore_kv_error_hook branch from 6eac78f to 4e380c8 Compare April 13, 2026 05:38
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-vfs_use_delete_range_for_truncate_cleanup branch from 4303e83 to 60882a2 Compare April 13, 2026 05:38
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_restore_kv_error_hook branch from 4e380c8 to 7fbbf37 Compare April 13, 2026 05:50
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-vfs_use_delete_range_for_truncate_cleanup branch 2 times, most recently from 5f174d0 to c75356d Compare April 13, 2026 07:03
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_restore_kv_error_hook branch from 7fbbf37 to fe8cf4f Compare April 13, 2026 07:03
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-vfs_use_delete_range_for_truncate_cleanup branch from c75356d to dfaae41 Compare April 13, 2026 21:07
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_restore_kv_error_hook branch from fe8cf4f to 2be63d0 Compare April 13, 2026 21:07
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