From 6804429c9fbc4afabe6361a93837d1071e2bd930 Mon Sep 17 00:00:00 2001 From: Andrew Phillips Date: Sat, 9 Aug 2025 23:22:13 -0300 Subject: [PATCH] docs: update PLAN.md with refined issue descriptions and solutions Co-authored-by: aider (openai/andrew/openrouter/anthropic/claude-sonnet-4) --- PLAN.md | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/PLAN.md b/PLAN.md index 53148e0..87e10ff 100644 --- a/PLAN.md +++ b/PLAN.md @@ -17,8 +17,8 @@ ### 3. Concurrency Issues **Files affected:** `src/modes/diff.rs`, `src/meta_plugin/digest.rs` **Functions affected:** `mode_diff()`, meta plugin `update()` methods -**Problem example:** Multiple threads accessing shared meta plugin state without synchronization -**Fix example:** Use Arc> for shared state or ensure thread-local plugin instances +**Problem example:** In `mode_diff()`, if writer threads panic, resources may not be cleaned up properly: `writer_thread_a.join()` only propagates panic but doesn't ensure file descriptors are closed +**Fix example:** Use RAII guards or ensure cleanup in panic handlers: `let _fd_guard = FileDescriptorGuard::new(fd_write);` ## Design Problems @@ -31,8 +31,15 @@ ### 5. Plugin Architecture Flaws **Files affected:** `src/meta_plugin.rs`, `src/meta_plugin/digest.rs`, `src/meta_plugin/program.rs` **Functions affected:** `MetaPlugin::create()`, `MetaPlugin::update()`, `MetaPlugin::finalize()` -**Problem example:** `create()` returns dummy writer that's never used, inconsistent with actual usage pattern -**Fix example:** Remove `create()` method and rely only on `update()`/`finalize()` pattern +**Problem example:** +- `create()` returns dummy writer that's never used, inconsistent with actual usage pattern +- `MetaPluginProgram::finalize()` spawns new process instead of reusing existing one +- No validation that meta plugins produce valid output formats +- Plugin errors are silently ignored in save operations +**Fix example:** +- Remove `create()` method and rely only on `update()`/`finalize()` pattern +- Reuse single process per plugin instance for better performance +- Add output validation and proper error propagation ### 6. Security Concerns **Files affected:** `src/main.rs`, `src/modes/get.rs`, `src/modes/delete.rs` @@ -45,14 +52,14 @@ ### 7. Inefficient Operations **Files affected:** `src/modes/save.rs`, `src/compression_engine.rs` **Functions affected:** `mode_save()`, `CompressionEngine::size()` -**Problem example:** Fixed 4KB buffer `let mut buffer = [0; libc::BUFSIZ as usize];` may be too small -**Fix example:** Use adaptive buffer sizing or larger default buffers (64KB+) +**Problem example:** Fixed BUFSIZ buffer (typically 8KB) may not be optimal for all scenarios, especially large files or fast storage +**Fix example:** Use adaptive buffer sizing based on file size or storage characteristics, or allow configuration via environment variable ### 8. I/O Problems -**Files affected:** `src/modes/save.rs`, `src/modes/get.rs`, `src/compression_engine.rs` -**Functions affected:** `mode_save()`, `mode_get()`, `CompressionEngine::cat()` -**Problem example:** All I/O operations are blocking with no progress indication -**Fix example:** Add progress callbacks or async I/O with tokio +**Files affected:** `src/meta_plugin/program.rs`, `src/compression_engine/program.rs` +**Functions affected:** `MetaPluginProgram::finalize()`, `CompressionEngineProgram::open()`, `CompressionEngineProgram::create()` +**Problem example:** Meta plugin processes can block indefinitely if they hang or produce large output without proper timeouts +**Fix example:** Add timeouts to process operations and non-blocking I/O for meta plugins: `process.wait_timeout(Duration::from_secs(30))` ## Code Quality Issues @@ -65,19 +72,17 @@ ### 10. Code Organization **Files affected:** `src/modes/save.rs`, `src/modes/diff.rs` **Functions affected:** `mode_save()`, `mode_diff()` -**Problem example:** `mode_save()` function is 150+ lines doing I/O, meta processing, and database operations -**Fix example:** Split into smaller functions: `setup_compression()`, `process_meta_plugins()`, `save_to_database()` +**Problem example:** Large functions doing multiple responsibilities +**Fix example:** Split into smaller functions: +- `src/modes/save.rs: mode_save()` → `setup_compression_and_plugins()`, `process_input_stream()`, `finalize_meta_plugins()`, `save_item_to_database()` +- `src/modes/diff.rs: mode_diff()` → `validate_diff_args()`, `setup_diff_pipes()`, `spawn_writer_threads()`, `execute_diff_command()`, `handle_diff_output()` +- `src/modes/diff.rs: write_item_to_pipe()` → `open_item_reader()`, `copy_item_data()` ## Specific Bug Risks ### 11. Race Conditions **Files affected:** `src/db.rs`, `src/modes/save.rs` **Functions affected:** `insert_item()`, `mode_save()` -**Problem example:** Multiple processes could create items with same auto-increment ID simultaneously -**Fix example:** Use database-level unique constraints and handle conflicts gracefully +**Problem example:** While SQLite uses file locking, multiple processes could still create items with same auto-increment ID if they begin transactions simultaneously before the lock is acquired, leading to potential file overwrites when using `item_id.to_string()` as filename +**Fix example:** Use database-level unique constraints and handle conflicts gracefully, or use UUIDs for filenames instead of auto-increment IDs -### 12. Data Integrity -**Files affected:** `src/modes/save.rs`, `src/modes/update.rs` -**Functions affected:** `mode_save()`, `mode_update()` -**Problem example:** Database entry created before file is fully written, crash could leave orphaned entries -**Fix example:** Write to temporary file first, then move to final location and update database atomically