102 lines
4.2 KiB
Markdown
102 lines
4.2 KiB
Markdown
# Implementation Findings
|
|
|
|
Confirmed, high-impact work only. Keep changes inside `crates/storage` unless a
|
|
finding explicitly names another crate. Prefer the smallest shared fix and add
|
|
one focused regression test per behavior change.
|
|
|
|
## 1. Preserve operation and rollback failures
|
|
|
|
Rollback code can replace the original failure with a restoration failure:
|
|
|
|
- `Repository::rollback_created` in `crates/storage/src/repository.rs`
|
|
- `TreeMutator::remove` and `TreeMutator::transfer` in
|
|
`crates/storage/src/mutation.rs`
|
|
|
|
Return an error containing both failures, following the existing
|
|
`WriteError::RollbackFailed { operation, rollback }` pattern. Cover failures
|
|
from reopening/syncing directories and from all source/destination restoration
|
|
steps. Never report successful rollback if any restoration step failed.
|
|
|
|
Acceptance: focused tests prove that the original operation error and rollback
|
|
error are both retained.
|
|
|
|
## 2. Use one snapshot per tree mutation
|
|
|
|
Directory remove/move/copy currently scan the repository up to five times via
|
|
`snapshot`, `reject_unmanaged`, `source_entries`, `source_policies`, and
|
|
`source_directories` in `crates/storage/src/mutation.rs`.
|
|
|
|
Take one snapshot at operation start and pass it to those helpers. All
|
|
resolution, validation, and source selection for the transaction must use that
|
|
same snapshot.
|
|
|
|
Acceptance: existing mutation tests pass and a focused test or instrumentation
|
|
proves one snapshot is taken for a directory operation.
|
|
|
|
## 3. Remove string-based HTTPS Git error classification
|
|
|
|
`map_reqwest_error` and `fetch_embedded_https` in
|
|
`crates/storage/src/git.rs` classify authentication, TLS, and network failures
|
|
using substrings from third-party error messages.
|
|
|
|
Use structured `reqwest` status/connect/timeout information and matchable `gix`
|
|
error variants wherever available. Keep a bounded textual diagnostic only as a
|
|
fallback; it must not drive a more specific classification unless the library
|
|
exposes no structured alternative.
|
|
|
|
Acceptance: tests classify HTTP 401/403, TLS failure, connection/timeout, and an
|
|
unknown protocol failure without depending on English error text.
|
|
|
|
## 4. Make config replacement durable
|
|
|
|
`Config::persist` and `persist_new` in `crates/storage/src/config.rs` sync the
|
|
file but not the parent directory after replacement/creation.
|
|
|
|
Sync the parent directory after the atomic rename, reusing the repository's
|
|
existing durability pattern where practical. Return a distinct durability error
|
|
if the rename succeeds but the directory sync fails.
|
|
|
|
Acceptance: focused tests cover the post-rename sync failure boundary. Do not
|
|
add config locking unless concurrent writers are an explicit supported use case.
|
|
|
|
## 5. Do not hold secret-store state across backend I/O
|
|
|
|
`SecretStore` in `crates/storage/src/secret_store.rs` holds its mutex while
|
|
calling potentially blocking backend operations (`unlock`, `lock`, `create`,
|
|
`retrieve`, `replace`, and `delete`).
|
|
|
|
Reduce the lock scope around backend calls while preserving atomic lock-state
|
|
and cache behavior. Do not weaken biometric/keychain protection or allow a
|
|
secret operation to complete as unlocked after a concurrent `lock()`.
|
|
|
|
Acceptance: concurrency tests cover `lock()` racing with at least retrieval and
|
|
mutation, and prove no stale secret is inserted into the cache.
|
|
|
|
## 6. Reap completed TUI task handles
|
|
|
|
`AsyncExecutor::submit` in `apps/tui/src/runtime.rs` retains every `JoinHandle`
|
|
until executor drop.
|
|
|
|
Remove and join finished handles during normal executor activity. Keep the
|
|
current thread-per-task design; do not add a thread pool without measured need.
|
|
|
|
Acceptance: a focused test submits and completes repeated tasks and verifies
|
|
the retained handle count does not grow without bound.
|
|
|
|
## Required checks
|
|
|
|
```sh
|
|
cargo fmt --all -- --check
|
|
RUSTFLAGS="-D warnings" cargo check --workspace --all-targets
|
|
cargo clippy --workspace --all-targets -- -D warnings
|
|
cargo test --workspace
|
|
```
|
|
|
|
## Explicitly deferred
|
|
|
|
Do not spend this pass on `thiserror`, broad error-enum redesign, Git options
|
|
structs, frontend file splitting, generic generation tracking, public struct
|
|
fields, UniFFI mirror removal, Git performance caching, config locking, or a
|
|
thread pool. Revisit only with a concrete bug, compatibility need, or measured
|
|
performance problem.
|