fix(crates): tighten data integrity, cleanup, and diagnostic contracts - #159
Merged
Conversation
forhappy
force-pushed
the
codex/crate-quality-review
branch
from
September 7, 2026 16:10
c808a15 to
0f268b2
Compare
forhappy
marked this pull request as ready for review
September 7, 2026 23:03
forhappy
force-pushed
the
codex/crate-quality-review
branch
from
September 7, 2026 23:52
715f91e to
d1a9a17
Compare
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
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.
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
GIT_DIRenvironment paths in shared Git discovery and the feature-gated CLI config resolver instead of discarding non-Unicode overrides.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_objectnow returns the existingEvictStatsinstead ofu64; 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
Documentation
CLI build metadata now tracks Git-resolved HEAD/current-branch paths. This avoids repeated worktree rebuilds caused by nonexistent
.git/HEADpaths 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.
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
b8b81728944completed 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.d0022ec2279is 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.b8b81728944passed. It covers the published mutation-cleanup and read-worker batches; the newer lookup/abandonment commits need fresh CI.b6755c4b503passed 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.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.