robustness patch for the partition system #45
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. New Module: src/safe_io.rs Created a reusable module providing: - atomic_write_json() - Write JSON atomically (temp file + fsync + rename) - atomic_write() - Write bytes atomically - FileLock - RAII file locking wrapper using fs2 This establishes the foundation for Issue #15 (unified safe file operations). 2. Issue #4: Config Validation (partition.rs:132-172) Added minimum value guards to StorageConfig methods: - max_entries() → .max(1) - prevents infinite loops - max_tokens() → .max(1) - prevents infinite loops - bytes_per_token() → .max(1) - prevents divide-by-zero - max_age_seconds() → .max(60) - minimum 1 minute prevents excessive rotation 3. Issue #3: Atomic Manifest Writes (partition.rs:439-445) Replaced direct file write in save_manifest() with safe_io::atomic_write_json(). This prevents manifest corruption from crashes during write operations. 4. Issue #2: File Locking (partition.rs:362-368, 489-491, 577-578) Added file locking to partition write operations: - Added lock_path() helper returning .transcript.lock - append_entry() now acquires exclusive lock before writing - rotate() now acquires exclusive lock before rotating 5. Issue #1: Performance Caching (partition.rs, state/mod.rs) Made ActiveState public with Clone, added caching support: - PartitionManager::load_with_cached_state() - load with optional cached state - PartitionManager::active_state() - getter for caching - AppState.active_state_cache: RefCell<HashMap<String, ActiveState>> - per-context cache - Updated append_to_transcript() to use/update cache - Cache invalidation in clear_context(), clear_context_by_name(), finalize_compaction(), and destroy_context()
new tests added: 9 in src/partition.rs: - test_active_state_getter — verifies active_state() returns correct counts - test_load_with_cached_state_uses_cache — cached state is used instead of file scan - test_load_with_cached_state_none_scans_file — none cache falls back to file scan - test_load_with_cached_state_invalidates_stale_cache — stale cache (missing file) resets to default - test_active_state_reset_after_rotation — state resets to 0 after rotation - test_manifest_atomic_write_no_tmp_left — no .tmp file remains after manifest write - test_lock_file_created_on_append — .transcript.lock created during append - test_lock_file_created_on_rotate — .transcript.lock created during rotate - test_storage_config_minimum_guards — zero config values clamped to minimums 6 in src/state/mod.rs: - test_append_to_transcript_caches_state — cache populated after append - test_append_to_transcript_updates_cache_incrementally — cache increments correctly - test_clear_context_invalidates_cache — cache removed after clear - test_clear_context_by_name_invalidates_cache — cache removed after clear by name - test_destroy_context_invalidates_cache — cache removed after destroy - test_finalize_compaction_invalidates_cache — cache removed after compaction bug fix discovered while testing: moved cache invalidation in clear_context() and clear_context_by_name() to happen after save_current_context()/save_context(), not before. the save calls internally write new-context anchors via append_to_transcript(), which re-populated the cache after invalidation. moving invalidation last ensures it sticks. supporting changes: - added ActiveState::entry_count() getter (#[cfg(test)]) so state tests can assert on cached counts - fixed clippy MSRV warning: changed self.file.unlock() to FileExt::unlock(&self.file) in FileLock::Drop to use the fs2 trait method instead of the stdlib method (stable since 1.89, but project MSRV is 1.88)
summary - fixes #15 - adds tests - fixes bugs uncovered src/safe_io.rs - added atomic_write_text() - convenience wrapper for string content - fixed concurrent writer race in atomic_write() - temp files now use unique PID^thread_hash suffix instead of a shared .tmp extension, preventing writers from clobbering each other's temp files - added 4 new tests: test_atomic_write_text, test_atomic_write_text_roundtrip, test_concurrent_atomic_writes_produce_valid_files, test_concurrent_locked_appends_produce_valid_lines src/context.rs - ContextState::save() - replaced manual OpenOptions write with atomic_write_json() (critical state.json) - improved save() signature - &PathBuf → &Path (clippy fix) - added 2 tests: test_context_state_save_atomic, test_context_state_save_concurrent_writes src/state/mod.rs (7 functions migrated) - write_context_entries() - atomic full rewrite of context.jsonl - save_context_meta() - atomic context_meta.json - save_context() summary - atomic summary.md - save_todos() - atomic todos.md - save_goals() - atomic goals.md - save_local_config() - atomic local.toml - set_system_prompt_for() - atomic system_prompt.md - legacy migration summary write - atomic summary.md - removed unused BufWriter import src/tools/builtin.rs - update_reflection - atomic reflection.md - removed unused std::fs import src/partition.rs - build_bloom_filter() - atomic .bloom files src/cache.rs - cache_output() - replaced manual temp+rename (which lacked fsync) with atomic_write_text() for content and atomic_write_json() for metadata src/lock.rs - ContextLock::touch() - atomic heartbeat writes (prevents false stale detection) src/inbox.rs - nigrated from manual fs2::FileExt locking to FileLock RAII wrapper in both append_to_inbox() and load_and_clear_current_inbox(), guaranteeing unlock on drop even on panics/early returns Left intentionally unchanged - mark_context_dirty() - writes empty marker file, idempotent, losing it is harmless - append-only operations - inherently safe - Debug logging - best-effort, append-only - deletions/renames - no partial-write risk
hello clippy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1. New Module: src/safe_io.rs
Created a reusable module providing:
This establishes the foundation for Issue #15 (unified safe file operations).
2. Config Validation (partition.rs:132-172)
Added minimum value guards to StorageConfig methods:
3. Atomic Manifest Writes (partition.rs:439-445)
Replaced direct file write in save_manifest() with safe_io::atomic_write_json(). This prevents
manifest corruption from crashes during write operations.
4. File Locking (partition.rs:362-368, 489-491, 577-578)
Added file locking to partition write operations:
5. Performance Caching (partition.rs, state/mod.rs)
Made ActiveState public with Clone, added caching support: