Skip to content

fix(crates): tighten data integrity, cleanup, and diagnostic contracts - #159

Merged
forhappy merged 126 commits into
mainfrom
codex/crate-quality-review
Sep 8, 2026
Merged

fix(crates): tighten data integrity, cleanup, and diagnostic contracts#159
forhappy merged 126 commits into
mainfrom
codex/crate-quality-review

Conversation

@forhappy

@forhappy forhappy commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

This crate-quality pass addresses data-integrity and lifecycle gaps: malformed reads could be accepted, some cleanup paths could return while workers still owned resources, metadata readers applied different validation rules, and credential initialization could overwrite or partially publish an encryption key.

The PR also consolidates duplicated logic and improves crate documentation. Implementation scope is finalized for review. Completed focused checks and outstanding CI/native qualification are distinguished below; the evidence ledger preserves the detailed scope of each result.

Data integrity and shared contracts

  • Share Git delta instruction decoding and preserve ordered matches and duplicate counts in large term comparisons. Consolidate term report assembly and make nested report equality reflexive.
  • Unify Xorb decoded-length/hash checks across readers, enforce the builder’s decoded offset limit, and reject excess LZ4/BG4 output before accumulating it. Preserve raw zero-copy reads.
  • Resolve warm local cache ranges against the opened cache file size; support bounded, offset, and suffix requests after origin removal. Clarify synthetic HEAD metadata and verify conditional/versioned reads bypass cached data.
  • Preserve NUL-delimited worktree field contents, including carriage returns in valid Unix paths.
  • Validate storage response ranges and declared body lengths through EOF. Route both file-download APIs through the same implementation and preserve late transport errors.
  • Bind LFS verification receipts and served streams to object identity. Invalidate unsafe verifier-1 receipts; the first verification after upload hashes stored bytes. Verified streaming requires a strong ETag or object version; single-read downloads remain supported without one.
  • Align metadata diagnostic history selection with writer tie-breaking by generation and placement ID within a committed batch. This diagnostic scan is not the production dedup proof path.
  • Preserve native GIT_DIR environment paths in shared Git discovery and the feature-gated CLI config resolver instead of discarding non-Unicode overrides.
  • Reject malformed hex before byte slicing across metadata, workflow, HTTP, cache-server, and Keychain inputs. Preserve typed pointer and LFS error causes.
  • Use checked timestamp formatting across shared contracts, CLI output, imports, and VFS; reject unsupported clock values instead of silently emitting invalid timestamps.

Resource ownership and cleanup

  • Serialize concurrent shared-reader close calls through checkpoint cleanup; a second caller can no longer return while the first is still closing SlateDB.

  • Apply the four-part LFS upload admission bound to full parts and the final partial part. Failed queued parts stop tail admission and release retained futures.

  • Preserve typed malformed-lock JSON causes and optional provider throttling causes through storage, CLI, read diagnostics, and push conversions. Local admission errors remain source-free; retry hints, CRAB-E0002, and wire summaries are unchanged.

  • Fail aborted ZIP traversal through the HTTP body after cleanup; qualify archive and LFS HTTP/1 response failures with loopback clients.

  • Reject invalid multipart cleanup clocks before selecting abandoned uploads, preserving the typed clock error through fsck. Reject unrepresentable push-lock expiry and renewal deadlines before storage access.

  • Require scheduler ownership before workflow orphan cleanup, and preserve malformed sidecar-like names instead of deleting user files/directories.

  • Centralize multipart completion/abort handling and preserve the original failure when cleanup also fails.

  • Validate term-resolution concurrency in the shared constructor and resolved CLI configuration. Concurrent batches share admission; each batch bounds retained worker tasks and preserves input-order strict errors and typed task-error sources. Both diff entry points propagate the fallible constructor.

  • Share metadata lookup ownership between terms and hydration. A slow first canonical miss no longer blocks unrelated index hits. Point and batch methods share initialization; close waits for active reads without requiring unused clones to disappear.

  • Cancel abandoned term-batch admission waiters through operation child tokens without cancelling the caller or sibling batches. A per-batch cleanup task waits for admission and tracked workers, then closes the reader even if the caller drops the batch or cleanup await. Await completion before runtime shutdown.

  • Drain term-resolution workers before closing shared lookup state. Preserve operation-versus-cleanup error precedence across write maintenance paths.

  • Give VFS queue workers and read-window prefetch one hydration-service owner. Serialize admission with shutdown and await admitted work, including blocking cache writes, through daemon, coordinator, NFS, and CLI cleanup. Keep timed-out waits resumable. Close the notification-subscription race that could strand hydration waiters. Coordinator, daemon, and interactive NFS owners retain and await refresh tasks after cancellation, including admitted blocking work. Native backend requests remain a separate gap.

  • Tie NFS read-pool pins to entry lifetimes so a late drop after invalidation cannot unpin and evict an active replacement.

  • Bound NFS control connection setup, writes, and response reads with one deadline and one shared TCP/Unix JSON exchange. Timeout drops the request-owned socket without retrying uncertain mutations.

  • Give each FUSE IPC exchange ownership of its buffered connection until a valid response completes. Bound writes and reads together; close the connection on cancellation, timeout, or transport/parse errors so late responses cannot be reused by a later call.

  • Leave stale coordinator socket cleanup to the daemon lock holder. IPC clients spawn/retry only for missing or refused connections and preserve other typed connection errors.

  • Preserve cache-server metadata and byte/eviction accounting when payload removal fails. Indexed eviction and corruption cleanup share one unlink policy with a typed filesystem cause; admin eviction returns HTTP 500 instead of false success.

  • Return actual object count and bytes from cache removal. Empty entries count once; already-absent candidates count zero. CacheStore::remove_object now returns the existing EvictStats instead of u64; all workspace callers are updated and the HTTP schema is unchanged.

  • Run periodic cache eviction on a blocking worker, one batch at a time. Shutdown signals the loop and joins admitted work instead of aborting its owner and detaching a disk mutation.

  • Give admin eviction and staged upload/origin publication a service-owned blocking worker with bounded admission. Shutdown drains admitted work, including cleanup of unclaimed temporary-file results. Origin responses retain staged bytes when authoritative publication rejects the write.

  • Own the TLS shutdown signal alongside serving instead of detaching a waiter; register signals before cache startup and preserve typed bind errors.

  • Return ordinary cache-service command errors for JSON/text write failures, including final newline and flush, and for fallible Tokio runtime initialization. Use one JSON writer for stdout and evidence files; retain generated onboarding bundles when printing their result fails.

  • Validate cache-server origin construction before cache initialization or maintenance startup. Invalid configuration previously created cache state and detached an eviction task. Correct the eviction handle's shutdown documentation.

  • Distinguish staging recovery I/O failures from absence. Reject metadata remote-index writes before either index is modified when the database is unopened.

Workflow and authentication

  • Make scheduler lock acquisition async so contention yields to the lock holder and cancellation drops the waiter. All four production CLI paths await the canonical API.
  • Validate retry policies during parsing and simplify scheduler lock outcomes using the dependency's boolean contention result.
  • Serialize inline and YAML cache-only replay with execution. YAML replay uses recorded lockfile hashes, skips commands and hooks, and reports misses as exit 3 instead of executing stages. Share cache materialization while preserving single-stage and workflow JSON output.
  • Anchor cached file, stdout, and directory materialization to the supplied repository root, including verified local fallback and overwrite protection.
  • Share schema and requested-ID decoding across all five experiment metadata readers. Remote readers also share canonical-hash validation. Preserve JSON causes through CLI integrity classification and retry handling.
  • Make local live-set collection fail on incomplete or invalid metadata instead of returning a partial root set. This exported helper has no production GC caller today.
  • Remove credentials from diagnostic Debug output and consolidate bounded unary auth-store retries.
  • Classify token reads under the existing cache lock. Create Keychain entries without replacing a concurrent winner, and publish complete key files without overwriting an existing key. Preserve Unix key-file permissions.

Documentation

  • Clarify crate boundaries, examples, feature slices, shutdown ownership, and persistence contracts. Correct the read README’s superseded source-loss warning against the existing repair and completed protocol regressions.
  • Replace visible rustdoc harness syntax in 14 README examples with explicit functions.
  • Give the HTTP server, cache, and remote-Git crates short READMEs linked to detailed references; document request validation, retry reservation, and handler versus response-body ownership.
  • Maintain the per-crate agent guides and sibling CLAUDE symlinks introduced for all 21 crates in docs(crates): add agent guides for all 21 shared crates #158.

CLI build metadata now tracks Git-resolved HEAD/current-branch paths. This avoids repeated worktree rebuilds caused by nonexistent .git/HEAD paths and unrelated checkpoint refs, while retaining commit invalidation.

The coverage and evidence ledger records caller/callee and sibling analysis, dependency contracts, regressions, commands, and remaining work for every crate.

Validation

These checks were run for their affected batches; this table is not a claim that every test was rerun on the latest head. Detailed results and feature combinations are in the ledger.

Surface Focused evidence
Final throttling diagnostics Baseline source-loss regression fails before the fix; storage mapping/retry, CLI/read/push source chains, diagnostic parity, retry timing and push summary checks pass; strict storage lint/docs and CLI build pass
LFS upload admission and lock diagnostics 36 object-store tests, upload success/failure boundary regressions, four lock tests, strict Clippy/rustdoc and CLI build pass for their affected batches
Concurrent shared-reader close Real checkpoint-write regression fails before the fix; 24 metadata lookup tests, downstream abandoned-batch cleanup, strict lint/docs and CLI build pass
Abandoned reader cleanup A real managed-checkpoint regression fails before the fix; all three APIs pass abandonment during worker execution and cleanup; eight term tests, stored-index/shard integration, strict lint/docs and CLI build pass
Shared lookup ownership Paused-origin regression fails before the fix; 23 metadata lookup tests, seven term-resolution tests, cross-path stored-index/shard integration, minimal metadata check, strict lint/docs and CLI build pass
Read admission and worker bounds Dropped-batch waiter release and parent isolation across all three APIs; invalid-capacity regressions; 137 configuration tests; 1,000-input ownership checks across all three APIs; completion-order and typed JoinError checks; strict lint/docs and CLI builds
Request mutation cleanup 94 cache-store/handler tests for unclaimed-result cleanup; real HTTP/SQLite contention regression; cancellation, staged recovery, strict lint/docs and binary builds
Background eviction Executor-starvation regression fails before the fix; 44 store, seven evictor, four server-owner and two HTTP eviction tests plus strict Clippy/rustdoc and binary build pass
Eviction counts Empty-object regression fails before the fix; 43 store, six evictor, four HTTP admin tests, strict lint/docs and binary build pass; cache-store downstream compilation passes
Cache eviction failures Regression reports false success before the fix; 42 store, six evictor, three HTTP admin tests, strict Clippy/rustdoc and binary build pass
Coordinator startup Two regressions fail before the fix; 26 IPC client tests, lock-owner cleanup test, running-coordinator CLI integration, and strict fuse+nfs lint/docs pass; default CLI build passes
FUSE IPC ownership Stalled-write and cancellation regressions fail before the fix; five focused client tests and strict fuse+nfs Clippy/rustdoc pass; CLI coordinator-ping integration and default-feature CLI build pass
NFS control deadlines Two stalled-write regressions fail before the fix; 19 control and ten mount-control tests pass; strict all-target Clippy and rustdoc pass; no-default-features NFS CLI build passes (CLI warnings remain)
Xorb decoding 21 parser tests; regressions fail before the fixes; strict Clippy/rustdoc and cache-store corruption/provenance checks
Cache ranges and authority Local/remote feature regressions, exact-range and recency siblings, conditional/version bypass tests, strict Clippy/rustdoc
Multipart clock handling 12 journal tests; six CLI consumer/repair checks; errors retain their typed cause
Lease deadline arithmetic 30 lock tests, cancellation/draining coverage, strict Clippy and rustdoc; 11 local heartbeat tests pass; one provider case remains ignored
Git build metadata 20 Cargo/native-Git fixture observations; full CLI build followed by an unchanged run without recompilation
Storage and LFS Multipart failure matrix; malformed stream/range regressions; receipt and stream replacement races; local-store and cross-crate integration tests
Workflow replay Missing-record and recorded-state regressions, selected-stage and hook isolation, flag conflicts, native inline/YAML lock contention, and exact-byte JSON/JSONL replay after input deletion
Workflow cleanup Five shared sweep tests, a real-lock CLI regression, and a built-CLI cross-process smoke; original code deletes an active holder’s sidecar before reporting lock timeout
Workflow metadata 44 experiment, live-set, CLI identity/error, and retry checks; old collector fails the new malformed-metadata regression
Token cache Latest batch: 24 tests covering encryption, lookup/deletion, concurrent publication, Keychain winner selection, non-overwrite, and permissions
Key-file write failure Isolated Unix subprocess forces a real write failure; original writer leaves the final path present, fixed writer does not
VFS and read lifecycle 116 focused hydration/pipeline/daemon/coordinator/NFS tests; strict all-target Clippy; separate NFS/FUSE checks; strict rustdoc; CLI build and all three foreground preparation tests pass
Refresh completion 91 focused coordinator/daemon/refresh/NFS tests, including blocking-work regressions that fail with the old teardown; strict Clippy/docs, backend checks, and CLI build
Scheduler waiting 14 lock tests, starvation regression failing before the fix, inline/YAML CLI contention tests, strict Clippy/docs, and CLI build
TLS signal ownership Occupied-loopback bind regression fails before the fix; four server tests cover TLS/mTLS startup and immediate shutdown, 16 preflight tests, strict Clippy/docs, and binary build
NFS lease identity Regression fails before the fix; nine pool tests, nine NFS read/readdir/readlink tests, generation invalidation test, strict Clippy/docs; CLI build result recorded in ledger
Cache CLI output and runtime Five binary tests and 41 CLI tests pass; real broken-pipe and descriptor-exhaustion regressions fail on the old code. Strict all-target Clippy and binary build pass
Cache startup Invalid-origin regression fails before the fix with one retained task and a created cache root; all three server tests pass after the fix. 16 preflight tests, strict all-target Clippy, and strict rustdoc pass
Metadata candidate ordering Six remote-index tests, strict Clippy/rustdoc, and auth-server publication consumer pass
Shared contracts and output Pointer/delta/comparison, timestamp, staging recovery, metadata index, and output-error tests
HTTP response framing Normal/cancelled LFS and completed/aborted archive bodies through real HTTP/1 clients; archive regression rejects the original partial-success behavior
Git directory overrides Both regressions fail before the fix; six shared tests with/without facade, six ref-resolution tests, seven feature-gated config-resolver tests, and three CLI adapter tests pass. Strict shared Clippy/docs and feature-enabled CLI build pass; real non-UTF8 directory comparison is Linux-only
Git worktree parsing Eight tests with and without facade; native Git reproduces trailing-carriage-return truncation before the fix
Diff reports 49 complete before/after report comparisons; 29 library tests including nested NaN equality
Documentation Verbatim README example compilation, selected doctests, and strict workflow rustdoc

The latest lookup batch passes strict combined all-target Clippy, strict rustdoc, minimal metadata compilation, and the CLI binary build. The macOS debug build retains the recorded unwind-section linker warning. The CLI is not claimed to be warning-free. Formatting and diff checks pass.

CI and remaining qualification

  • The previous published head b8b81728944 completed with 31 successful and 11 skipped PR checks, including Rust quality and tests. Fresh checks are required for the newly published lookup and cleanup changes.
  • Added bounded Windows process-lifetime sampling around the mount command, recording process identity and timing only. The static smoke-script checker passes; native run on d0022ec2279 is in progress. Its feature gate, Linux RustFS/Xet job, and native Linux/macOS jobs have passed; Windows remains in progress. The Windows hang remains unqualified.
  • The cache-service run on b8b81728944 passed. It covers the published mutation-cleanup and read-worker batches; the newer lookup/abandonment commits need fresh CI.
  • Native NFS run on b6755c4b503 passed both Linux jobs and macOS; Windows reached its one-hour deadline and retained-evidence verification failed because its smoke report is missing. The retained macOS report verifies its exact commit and required artifacts. Its uncached fixture records 31 lease hits, one miss, and 32 MiB of NFS responses for 8 MiB of user reads (4.0× amplification). This proves fixture lease reuse, not efficient-read or historical-throughput parity.
  • The earlier Windows run reached its deadline without a complete report; retained-evidence verification failed. On the newer run, mount readiness appears but the post-native-command phase does not. The native-command/Tee pipeline remains blocked before mounted-path probing; process exit versus inherited output handles still needs evidence. No Windows hang fix or pass is claimed.
  • The earlier repository-browser job failed a release-tooltip contrast assertion. Its cause remains unverified; the gate is not claimed green.
  • Full workspace/native/cloud qualification, sibling-owner abandoned-future cleanup and runtime shutdown, aggregate resource bounds, whole-backend shutdown, auth key-source policy, non-Unix token locking, live identity-provider/Keychain integration, and power-loss durability remain tracked work. Keychain fixtures simulate commands and do not access host credentials.

This PR is ready for review with its implementation scope frozen. Fresh CI must qualify the final head before merge. The ledger records historical evidence and follow-up opportunities; native Windows and the other qualification limits above remain explicit.

@forhappy forhappy changed the title fix(crates): improve error sources, credential diagnostics, and diff correctness fix(crates): strengthen diagnostics, diff correctness, and retry validation Sep 7, 2026
@forhappy forhappy changed the title fix(crates): strengthen diagnostics, diff correctness, and retry validation fix(crates): tighten data integrity, cleanup, and diagnostic contracts Sep 7, 2026
@forhappy
forhappy force-pushed the codex/crate-quality-review branch from c808a15 to 0f268b2 Compare September 7, 2026 16:10
@forhappy
forhappy marked this pull request as ready for review September 7, 2026 23:03
@forhappy
forhappy force-pushed the codex/crate-quality-review branch from 715f91e to d1a9a17 Compare September 7, 2026 23:52
@forhappy
forhappy merged commit f63e03f into main Sep 8, 2026
42 checks passed
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