Chili Palmer
2feecae70d
Some checks failed
Dependency security audit / rustsec (push) Has been cancelled
92 lines
3.7 KiB
Markdown
92 lines
3.7 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, error sources, and
|
|
matchable `gix` variants wherever available. Classify TLS only when a structured
|
|
source identifies it. When `gix` has erased the underlying status or cause,
|
|
return a generic error with a bounded diagnostic instead of guessing a specific
|
|
class from rendered text.
|
|
|
|
Acceptance: tests classify direct HTTP 401/403 and structured connection,
|
|
timeout, and TLS failures without depending on English error text. A `gix`
|
|
failure without a structured cause remains generic and its diagnostic is
|
|
bounded.
|
|
|
|
## 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. 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.
|